On Tue, Jun 23, 2015 at 8:27 PM, John Lane <g...@jelmail.com> wrote:
>>> - err = cr->recover_key (source, dev, hdr);
>>> + err = cr->recover_key (source, dev, hdr, key, keyfile_size);
>> You never clear key variable, so after the first call all subsequent
>> invocations will always use it for any device. Moving to proper
>> callback data would make such errors less likely.
> It is cleared when the arguments are processed, specifically
> "grub_cmd_cryptomount" sets "key = NULL".
Right, missed it, sorry.
>>> + keyfile_size = state[7].set ? grub_strtoul (state[7].arg, 0,
> 0) : \
>>> + GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
>>> +
This must check keyfile_size, otherwise it meand buffer overwrite.
>>> + keyfile = grub_file_open (state[4].arg);
>>> + if (!keyfile)
>>> + return grub_errno;
>>> +
>>> + if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
>>> + return grub_errno;
>>> +
>>> + keyfile_size = grub_file_read (keyfile, key, keyfile_size);
>>> + if (keyfile_size == (grub_size_t)-1)
>>> + return grub_errno;
>> If keyfile size is explicitly given, I expect that short read should be
>> considered an error. Otherwise what is the point of explicitly giving
>> size?
> A short read is accepted by the upstream "cryptsetup" tool. Its man page
> describes its "--keyfile-size" argument as "Read a maximum of value
> bytes from the key file. Default is to read the whole file up to the
> compiled-in maximum. The cryptomount command follows that rule.
It is not what my version of cryptsetup says.
>From a key file: It will be cropped to the size given by -s. If there
is insufficient key material in the key file, cryptsetup will quit
with an error.
Which is logical. If I state that I want to have N bytes in a key,
getting less is error.
>>> - if (!grub_password_get (passphrase, MAX_PASSPHRASE))
>>> + if (keyfile_bytes)
>>> {
>>> - grub_free (split_key);
>>> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
> supplied");
>>> + /* Use bytestring from key file as passphrase */
>>> + passphrase = keyfile_bytes;
>>> + passphrase_length = keyfile_bytes_size;
>>> + }
>>> + else
>> I'm not sure if this should be "else". I think normal behavior of user
>> space is to ask for password then. If keyfile fails we cannot continue
>> anyway.
> Not sure I follow you. This is saying that the key file data should be
> used if given ELSE ask the user for a passphrase.
What I mean - if user requested keyfile but keyfile could not be read,
we probably should fallback to interactive password.
I mostly think about scenario where keyfile is used to decrypt
/boot/grub; in this case we cannot continue if we cannot decrypt it
and we are in pre-normal environment where we cannot easily script. So
asking user for passphrase seems logical here.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
