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

[John Lane <grub@jelmail.com>]

On 24/06/15 07:59, Andrei Borzenkov wrote:
> On Tue, Jun 23, 2015 at 8:27 PM, John Lane <grub@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.
I'll add in a check for this.
>
>>>> +      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.
This is equivalent to the "-l" or "--keyfile-size" option, not the "-s"
or '--key-size' option.

It isn't the number of bytes in the key; it's the maximum number of
bytes that is read from the key file. For LUKS the key file contains a
passphrase which is then used to derive the key (pbkdf2 function). This
argument allows a passphrase length to be given.

My cryptsetup version is 1.6.6 and it says what I wrote above, which is
also reflected in the current upstream head:

https://gitlab.com/cryptsetup/cryptsetup/blob/master/man/cryptsetup.8#L635
> Which is logical. If I state that I want to have N bytes in a key,
> getting less is error.
I can see your logic but I was just following the convention set down by
cryptsetup.
I can make it error if that's preferred but it isn't what cryptsetup does.
>>>> -  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.
>

I'll change it so that it falls back to passphrase if it cannot open the
key file. This is in cryptodisk.c.
Should it also fall back to passphrase on other errors (seek and read) ?
The behaviour that I copied from elsewhere in the code was to exit with
an error when these things happen.

Here's the relevant (revised) snippet:

     keyfile = grub_file_open (state[4].arg);
      if (keyfile)
        {  

          if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
            return grub_errno;

          key = keyfile_buffer;
          keyfile_size = grub_file_read (keyfile, key, keyfile_size);
          if (keyfile_size == (grub_size_t)-1)
           return grub_errno;
        }  
      else
        grub_printf (N_("Unable to open key file %s\n"), state[4].arg);

You can see it errors on seek and read but will continue if the open
fails. "Continue" means return to the grub prompt.

Most commands seem to return to the prompt on error.