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

[John Lane]

On 24/06/15 13:02, Andrei Borzenkov wrote:
> 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. 
OK, I've re-coded the keyfile bit to continue to passphrase on error,
and being unable to read a specified keyfile size is considered an error
now in addition to failure to open, seek or read.

revised patch forthcoming...
> 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.
How about making it so the user gets a number of attempts, so that a bad
passphrase results in the passphrase prompt again (a bit like when you
log in to a getty). The number of tries could be preset, say to 2. If a
keyfile is given then that uses up one try. A bad keyfile would then
result in a passphrase prompt.

The way the existing code is written would fit that model easily without
too much trouble.
>
>> 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.
I meant in the Grub code ;)