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

[John Lane]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Comments inline. I'll resubmit the patch set with changes as per comments.


On 20/06/15 05:54, Andrei Borzenkov wrote:
> В Tue, 16 Jun 2015 10:11:13 +0100
> John Lane <address@hidden> пишет:
>
>> ---
>>  grub-core/disk/cryptodisk.c | 34 +++++++++++++++++++++++++++++++--
>>  grub-core/disk/geli.c       |  4 +++-
>>  grub-core/disk/luks.c       | 46
+++++++++++++++++++++++++++++++--------------
>>  include/grub/cryptodisk.h   |  5 ++++-
>>  4 files changed, 71 insertions(+), 18 deletions(-)
>>
>> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
>> index 65b3a01..fbe7db9 100644
>> --- a/grub-core/disk/cryptodisk.c
>> +++ b/grub-core/disk/cryptodisk.c
>> @@ -41,6 +41,10 @@ static const struct grub_arg_option options[] =
>>      {"all", 'a', 0, N_("Mount all."), 0, 0},
>>      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."),
0, 0},
>>      {"header", 'H', 0, N_("Read LUKS header from file"), 0,
ARG_TYPE_STRING},
>> +    {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
>> +    {"key-size", 'K', 0, N_("Set key size (bits)"), 0, ARG_TYPE_INT},
> It is unused
That's a mistake from when I split the patch in  two. The key size
argument is only used by plain mode.
(in LUKS mode it's obtained from the header). I'll re-do the patches
with that in the plain-mode one.
>
>> +    {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0,
ARG_TYPE_INT},
>> +    {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0,
ARG_TYPE_INT},
>>      {0, 0, 0, 0, 0, 0}
>>    };
>> 
>> @@ -805,6 +809,8 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>>  static int check_boot, have_it;
>>  static char *search_uuid;
>>  static grub_file_t hdr;
>> +static grub_uint8_t *key,
keyfile_buffer[GRUB_CRYPTODISK_MAX_KEYFILE_SIZE];
>> +static grub_size_t keyfile_size;
>> 
> Someone should really get rid of static variables and pass them as
> callback data.
I just followed the conventions used by the existing code. I am not
familiar enough with the code-base
to change how it does things.
>
>>  static void
>>  cryptodisk_close (grub_cryptodisk_t dev)
>> @@ -835,7 +841,7 @@ grub_cryptodisk_scan_device_real (const char
*name, grub_disk_t source)
>>      if (!dev)
>>        continue;
>>     
>> -    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".
I have tested multiple uses and it does work as expected.
>
>>      if (err)
>>      {
>>        cryptodisk_close (dev);
>> @@ -938,12 +944,36 @@ grub_cmd_cryptomount (grub_extcmd_context_t
ctxt, int argc, char **args)
>>        hdr = grub_file_open (state[3].arg);
>>        if (!hdr)
>>          return grub_errno;
>> -      grub_printf ("\nUsing detached header %s\n", state[3].arg);
> You remove line just added in previous patch? Why previous patch added
> it then?
I thought I'd removed that. Will re-do.
>
>>      }
>>    else
>>      hdr = NULL;
>> 
>>    have_it = 0;
>> +
>> +  if (state[4].set) /* Key file */
>> +    {
>> +      grub_file_t keyfile;
>> +      int keyfile_offset;
>> +
>> +      key = keyfile_buffer;
>> +      keyfile_offset = state[6].set ? grub_strtoul (state[6].arg, 0,
0) : 0;
>> +      keyfile_size = state[7].set ? grub_strtoul (state[7].arg, 0,
0) : \
>> +                                     GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
>> +
>> +      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.
>
>> +    }
>> +  else
>> +    key = NULL;
>> +
>>    if (state[0].set)
>>      {
>>        grub_cryptodisk_t dev;
>> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
>> index f4394eb..da6aa6a 100644
>> --- a/grub-core/disk/geli.c
>> +++ b/grub-core/disk/geli.c
>> @@ -401,7 +401,9 @@ configure_ciphers (grub_disk_t disk, const char
*check_uuid,
>> 
>>  static grub_err_t
>>  recover_key (grub_disk_t source, grub_cryptodisk_t dev,
>> -         grub_file_t hdr __attribute__ ((unused)) )
>> +         grub_file_t hdr __attribute__ ((unused)),
>> +         grub_uint8_t *key __attribute__ ((unused)),
>> +         grub_size_t keyfile_size __attribute__ ((unused)) )
>>  {
>>    grub_size_t keysize;
>>    grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
>> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
>> index 66e64c0..0d0db9d 100644
>> --- a/grub-core/disk/luks.c
>> +++ b/grub-core/disk/luks.c
>> @@ -136,6 +136,8 @@ configure_ciphers (grub_disk_t disk, const char
*check_uuid,
>>    ciphermode[sizeof (header.cipherMode)] = 0;
>>    grub_memcpy (hashspec, header.hashSpec, sizeof (header.hashSpec));
>>    hashspec[sizeof (header.hashSpec)] = 0;
>> +  grub_memcpy (uuid, header.uuid, sizeof (header.uuid));
>> +  uuid[sizeof (header.uuid)] = 0;
>> 
> How exactly this is related to keyfile support?
it isn't. it belongs in one of the other patches. will fix.
>
>
>>    ciph = grub_crypto_lookup_cipher_by_name (ciphername);
>>    if (!ciph)
>> @@ -322,12 +324,16 @@ configure_ciphers (grub_disk_t disk, const char
*check_uuid,
>>  static grub_err_t
>>  luks_recover_key (grub_disk_t source,
>>            grub_cryptodisk_t dev,
>> -              grub_file_t hdr)
>> +          grub_file_t hdr,
>> +          grub_uint8_t *keyfile_bytes,
>> +          grub_size_t keyfile_bytes_size)
>>  {
>>    struct grub_luks_phdr header;
>>    grub_size_t keysize;
>>    grub_uint8_t *split_key = NULL;
>> -  char passphrase[MAX_PASSPHRASE] = "";
>> +  char interactive_passphrase[MAX_PASSPHRASE] = "";
>> +  grub_uint8_t *passphrase;
>> +  grub_size_t passphrase_length;
>>    grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
>>    unsigned i;
>>    grub_size_t length;
>> @@ -364,18 +370,30 @@ luks_recover_key (grub_disk_t source,
>>    if (!split_key)
>>      return grub_errno;
>> 
>> -  /* Get the passphrase from the user.  */
>> -  tmp = NULL;
>> -  if (source->partition)
>> -    tmp = grub_partition_get_name (source->partition);
>> -  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
>> -           source->partition ? "," : "", tmp ? : "",
>> -           dev->uuid);
>> -  grub_free (tmp);
>> -  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.
>
>> +    {
>> +      /* Get the passphrase from the user.  */
>> +      tmp = NULL;
>> +      if (source->partition)
>> +        tmp = grub_partition_get_name (source->partition);
>> +      grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "),
source->name,
>> +            source->partition ? "," : "", tmp ? : "", dev->uuid);
>> +      grub_free (tmp);
>> +      if (!grub_password_get (interactive_passphrase, MAX_PASSPHRASE))
>> +        {
>> +          grub_free (split_key);
>> +          return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
supplied");
>> +        }
>> +
>> +      passphrase = (grub_uint8_t *)interactive_passphrase;
>> +      passphrase_length = grub_strlen (interactive_passphrase);
>> +
>>      }
>> 
>>    /* Try to recover master key from each active keyslot.  */
>> @@ -393,7 +411,7 @@ luks_recover_key (grub_disk_t source,
>> 
>>        /* Calculate the PBKDF2 of the user supplied passphrase.  */
>>        gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *)
passphrase,
>> -                     grub_strlen (passphrase),
>> +                     passphrase_length,
>>                       header.keyblock[i].passwordSalt,
>>                       sizeof (header.keyblock[i].passwordSalt),
>>                       grub_be_to_cpu32 (header.keyblock[i].
>> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
>> index 16dee3c..0299625 100644
>> --- a/include/grub/cryptodisk.h
>> +++ b/include/grub/cryptodisk.h
>> @@ -55,6 +55,8 @@ typedef enum
>>  #define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
>>  #define GRUB_CRYPTODISK_MAX_KEYLEN 128
>> 
>> +#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
>> +
> Why it is different from MAX_KEYLEN?
A keyfile can be bigger than a key. A offset into the keyfile gives the
start of the key.
The given value is what is implemented by cryptsetup (as reported by
"cryptsetup --help").
The value of MAX_KEYLEN is what existed in Grub prior to patching.
>
>>  struct grub_cryptodisk;
>> 
>>  typedef gcry_err_code_t
>> @@ -108,7 +110,8 @@ struct grub_cryptodisk_dev
>> 
>>    grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
>>                   int boot_only, grub_file_t hdr);
>> -  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t
dev, grub_file_t hdr);
>> +  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev,
>> +                grub_file_t hdr, grub_uint8_t *key, grub_size_t
keyfile_size);
>>  };
>>  typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
>> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJViZccAAoJEGnTYCRabxPG3FgP/3v62hWS/5fH9z4KGgRJvaCA
gJSkuy8HcLwBurH8xLqaAwEZe9NVEMeDsbtjS5jeNFiylhYp2kYa1PAgOGb0aAQS
I+i2Ek4soIgPQyC212JzpCot9GI+WUCXObQ/unXVaWrViqL3/DJRoo0Nhes1g0Gh
qvLYJjfBb3Ujl2Ldo9ANWIGATzUSc/wi8oXl/mGWUQ0Gz3hBL3VDKcsjYq3Y8eQD
JpSTr2dJrTKvY+3lnwTlQt4YSbKkOH+PvEdiB/jKGqkw0r7K3BQKGCiIpgeS8bbS
bhLn24HuGIWfCKdZlqvBsmNuB6elUucTUIYkbvLqwD7Q6d/2a/30AzsgOpBgmCR0
dw6EUc0loTBqmpeeChS0Z0+JLMaFzRk8Yxq/FjrASejOXVL3sXLXO/2YW2LyvNTk
PIHSuZ0u8juqwM12xI5eZ94pRq+LNyDvwPcdPqJHsiFyXYTn3JYlPAgD+4R0IHpV
NWWQMIxTR5RdLkVoxGtyAIsKYGcxjd9nY4A0mRvmLZYx8jBBMi0L7XITLpoRy9ZX
ib3a05pfSh6cM1dGHXPXnYjNXga2QBJ7MUrL3HdE48jLSbVd7LtBBpAFczBjrfyJ
Xw5tFvRUbHM5qFBdwCMiFJTIOiQxinBQQfVQ+U0zKH+OPfTC+2svtpNLFr4hGVPS
PblMr2bI0cZAv4zhD96j
=yRSR
-----END PGP SIGNATURE-----