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

[Andrei Borzenkov]

On Wed, Jun 24, 2015 at 2:26 PM, John Lane <address@hidden> wrote:
>>>>> +
>>>>> +      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.
>

Ah, right. Cryptography must be confusing, otherwise it is not secret :)

> 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

I do not unterpret it your way.

>> 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.

Hmm ...

https://gitlab.com/cryptsetup/cryptsetup/blob/master/lib/utils_crypt.c#L446

if (!unlimited_read && i != keyfile_size_max) {
log_err(cd, _("Cannot read requested amount of data.\n"));
goto out_err;
}

So yes, it reads as much as it can - unless it is explicitly requested
to read specific amount of data.

>>>>> -  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) ?

I do not right now see reasons to differentiate. Either we could read
key or not. Moreover, it pobably should fallback to pasphrase even if
key file is present but verification failed. We may add --silent or
similar if some use case emerges.

> The behaviour that I copied from elsewhere in the code was to exit with
> an error when these things happen.
>

User using cryptsetup in full blown Unix shell has much better ways to
handle such errors; we do not.