Re: [PATCH 3/4] cryptodisk: Add options to cryptomount to support keyfiles

[Glenn Washburn]

On Thu, 12 May 2022 19:45:48 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, May 06, 2022 at 03:45:59AM -0500, Glenn Washburn wrote:
> > From: John Lane <john@lane.uk.net>
> >
> > Add the options --key-file, --keyfile-offset, and --keyfile-size to
> > cryptomount and code to put read the requested key file data and pass
> > via the cargs struct. Note, key file data is for all intents and purposes
> > equivalent to a password given to cryptomount. So there is no need to
> > enable support for key files in the various crypto backends (eg. LUKS1)
> > because the key data is passed just as if it were a password.
> >
> > Signed-off-by: John Lane <john@lane.uk.net>
> > GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
> > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> > development@efficientek.com: rebase and rework to use cryptomount arg 
> > passing,
> >   minor fixes, improve commit message
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 86 ++++++++++++++++++++++++++++++++++++-
> >  include/grub/cryptodisk.h   |  2 +
> >  include/grub/file.h         |  2 +
> >  3 files changed, 89 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 9f5dc7acb..19af4fa49 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -42,6 +42,9 @@ 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},
> >      {"password", 'p', 0, N_("Password to open volumes."), 0, 
> > ARG_TYPE_STRING},
> > +    {"key-file", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
> > +    {"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}
> >    };
> >
> > @@ -1172,6 +1175,85 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, 
> > int argc, char **args)
> >        cargs.key_len = grub_strlen (state[3].arg);
> >      }
> >
> > +  if (state[4].set) /* keyfile */
> > +    {
> > +      const char *p = NULL;
> > +      grub_file_t keyfile;
> > +      unsigned long long keyfile_offset = 0, keyfile_size = 0;
> > +
> > +      if (state[5].set) /* keyfile-offset */
> > +   {
> > +     keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
> 
> Hmmm... Could not you use grub_strtoul() instead of grub_strtoull()?

I was thinking the allowing the largest possible offset might be useful
to allow for using unallocated space at the end of large block devices.
This could still be done with smaller offsets, by using the blocklist
syntax to create a "file" that starts near the end and then use a much
smaller offset. Why don't you like it as is?

> > +
> > +     if (grub_errno != GRUB_ERR_NONE)
> > +       return grub_errno;
> 
> Could you explain why do you think this is needed? I think this check is
> redundant. "man strtoul" says:
> 
>   If endptr is not NULL, strtoul() stores the address of the first invalid
>   character in *endptr. If there were no digits at all, strtoul() stores the
>   original value of nptr in *endptr (and returns 0). In particular, if *nptr
>   is not '\0' but **endptr is '\0' on return, the entire string is valid.

Yep, just checked the implementation, you're right, this isn't needed.
I didn't realize that endptr would be set in the case of an overflow of
an otherwise valid number.

> > +     if (state[5].arg[0] == '\0' || *p != '\0')
> > +       return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +                          N_("non-numeric or invalid keyfile offset `%s'"),
> > +                          state[5].arg);
> > +   }
> > +
> > +      if (state[6].set) /* keyfile-size */
> > +   {
> > +     keyfile_size = grub_strtoul (state[6].arg, &p, 0);
> > +
> > +     if (state[6].arg[0] == '\0' || *p != '\0')
> > +       return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +                          N_("non-numeric or invalid keyfile size `%s'"),
> > +                          state[6].arg);
> > +
> > +     if (grub_errno != GRUB_ERR_NONE)
> > +       return grub_errno;
> 
> Ditto.
> 
> > +     if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
> > +       return grub_error (GRUB_ERR_OUT_OF_RANGE,
> > +                          N_("key file size exceeds maximum (%d)"),
> > +                          GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
> > +
> > +     if (keyfile_size == 0)
> > +       return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
> > +   }
> > +
> > +      keyfile = grub_file_open (state[4].arg,
> > +                           GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
> > +      if (keyfile == NULL)
> > +   return grub_errno;
> > +
> > +      if (keyfile_offset > keyfile->size)
> > +   {
> > +     keyfile_offset = keyfile->size;
> 
> I do not understand this keyfile_offset clamp. Could you enlighten me?
> In general I think you should fail here.

Hmm, yes, the clamp just makes the keyfile effectively zero bytes long,
but that makes little sense.

> > +     grub_dprintf ("cryptodisk","Keyfile offset, %llu, is greater than"
> > +                                "keyfile size, %" PRIuGRUB_UINT64_T "\n",
> > +                                keyfile_offset, keyfile->size);
> 
> Both keyfile_offset and keyfile->size are the same here.

This should've been before the clamp.

> > +   }
> > +
> > +      if (grub_file_seek (keyfile, (grub_off_t) keyfile_offset) == 
> > (grub_off_t) -1)
> > +   return grub_errno;
> > +
> > +      if (keyfile_size > 0)
> 
> I think "!= 0" would be more natural here.

Sounds good.

> > +   {
> > +     if (keyfile_size > (keyfile->size - keyfile_offset))
> > +       return grub_error (GRUB_ERR_FILE_READ_ERROR,
> > +                          N_("keyfile is too small: requested %llu bytes,"
> > +                             " but the file only has %" PRIuGRUB_UINT64_T
> > +                             " bytes"),
> > +                          keyfile_size,
> > +                          keyfile->size);
> 
> This error message is confusing. I think "keyfile->size" should be 
> "keyfile->size - keyfile_offset".

Good catch. I think it could be still confusing. I'll think of a better
way to say this.

> > +     cargs.key_len = keyfile_size;
> > +   }
> > +      else
> > +   cargs.key_len = keyfile->size - keyfile_offset;
> > +
> > +      cargs.key_data = grub_malloc (cargs.key_len);
> > +      if (cargs.key_data == NULL)
> > +   return GRUB_ERR_OUT_OF_MEMORY;
> > +
> > +      if (grub_file_read (keyfile, cargs.key_data, cargs.key_len) != 
> > (grub_ssize_t) cargs.key_len)
> > +   return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("reading key file")));
> 
> s/reading key file/cannot read key file/?

Sure.

Glenn