On 24/06/15 13:02, Andrei Borzenkov wrote: > On Wed, Jun 24, 2015 at 2:26 PM, John Lane <g...@jelmail.com> 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 ;) _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
