Re: [PATCH 2/4] Cryptomount support key files

[Andrei Borzenkov]

On Tue, Jun 23, 2015 at 8:27 PM, John Lane <address@hidden> 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.