Re: [PATCH] cryptodisk: add luks_recover_key attempts option

[Daniel Kiper]

On Fri, Oct 04, 2019 at 09:11:09PM -0400, Jason Kushmaul wrote:
> Hello,
>
> Daniel or other reviewer,
>
> I was hoping to get a review for my accessibility patch offering motor
> impaired individuals more access to full disk encrypted disks, by
> allowing a configurable retry option.
>
> I've addressed the review items from before from Daniel
>
> From d135f9f6b7d1503f551e8cced9ffe43a30af31d3 Mon Sep 17 00:00:00 2001
> From: "Jason J. Kushmaul" <address@hidden>
> Date: Sat, 20 Jul 2019 17:03:01 -0400
> Subject: [PATCH] cryptodisk: add luks_recover_key attempts option
>
> Those with motor impairments have a barrier preventing
> them from enjoying LUKS full disk encryption with
> strong passphrases due to no retry behavior.
>
> This patch adds an accessibility configuration where
> one may configure an arbitrary number of attempts
> via the "-t" option.
>
> Existing users will observe the original behavior
> with a default of 1 attempt.
>
> Reported-by: Jason J. Kushmaul <address@hidden>
> Signed-off-by: Daniel Kiper <address@hidden>

Both lines should be replaced with

Signed-off-by: Jason J. Kushmaul <address@hidden>

> ---
>  docs/grub.texi                   | 10 +++++++--
>  grub-core/disk/cryptodisk.c      | 18 ++++++++++++++--
>  grub-core/disk/luks.c            | 36 +++++++++++++++++++++++++-------
>  grub-core/osdep/aros/config.c    |  2 ++
>  grub-core/osdep/unix/config.c    |  6 ++++--
>  grub-core/osdep/windows/config.c |  2 ++
>  include/grub/cryptodisk.h        |  1 +
>  include/grub/emu/config.h        |  1 +
>  include/grub/util/misc.h         |  2 ++
>  util/config.c                    | 29 +++++++++++++++++++++++++
>  util/grub-install.c              | 12 +++++------
>  11 files changed, 100 insertions(+), 19 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 3d50b16ba..01dc1114c 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1508,6 +1508,11 @@ check for encrypted disks and generate
> additional commands needed to access
>  them during boot.  Note that in this case unattended boot is not possible
>  because GRUB will wait for passphrase to unlock encrypted container.
>
> +@item GRUB_CRYPTODISK_ATTEMPTS
> +If set, @command{grub-install} will allow the provided number of attempts
> +on key recovery.  The default if not present or zero is 1 to match original
> +behavior.  Currently only support with LUKS cryptodisks.

Please replace "The default if not present or zero is 1 to match
original behavior. Currently only support with LUKS cryptodisks." with
"If not present or zero the default is 1. Currently only LUKS
cryptodisks support that option."

> +
>  @item GRUB_INIT_TUNE
>  Play a tune on the speaker when GRUB starts.  This is particularly useful
>  for users unable to see the screen.  The value of this option is passed
> @@ -4195,12 +4200,13 @@ Alias for @code{hashsum --hash crc32 arg
> @dots{}}. See command @command{hashsum}
>  @node cryptomount
>  @subsection cryptomount
>
> -@deffn Command cryptomount device|@option{-u} uuid|@option{-a}|@option{-b}
> +@deffn Command cryptomount [@option{-t} tries] device|@option{-u}
> uuid|@option{-a}|@option{-b}
>  Setup access to encrypted device. If necessary, passphrase
>  is requested interactively. Option @var{device} configures specific grub 
> device
>  (@pxref{Naming convention}); option @option{-u} @var{uuid} configures device
>  with specified @var{uuid}; option @option{-a} configures all detected 
> encrypted
> -devices; option @option{-b} configures all geli containers that have
> boot flag set.
> +devices; option @option{-b} configures all geli containers that have
> boot flag set;
> +option @option{-t}, LUKS only, configures passphrase retry attempts,
> defaulting to 1.
>
>  GRUB suports devices encrypted using LUKS and geli. Note that
> necessary modules (@var{luks} and @var{geli}) have to be loaded
> manually before this command can
>  be used.
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 5037768fc..a3f0fa44d 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -33,6 +33,7 @@
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> +unsigned long max_attempts;

AIUI you do not use max_attempts outside of this module. If yes then
static unsigned long max_attempts; here please. And you can drop
EXPORT_VAR() from include/grub/cryptodisk.h.

>  grub_cryptodisk_dev_t grub_cryptodisk_list;
>
>  static const struct grub_arg_option options[] =
> @@ -41,6 +42,7 @@ static const struct grub_arg_option options[] =
>      /* TRANSLATORS: It's still restricted to cryptodisks only.  */
>      {"all", 'a', 0, N_("Mount all."), 0, 0},
>      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> +    {"tries", 't', 0, N_("Max passphrase attempts."), 0, GRUB_KEY_ARG},
>      {0, 0, 0, 0, 0, 0}
>    };
>
> @@ -934,6 +936,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt,
> int argc, char **args)
>    if (argc < 1 && !state[1].set && !state[2].set)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>
> +  /* Default to original behavior of 1 attempt */
> +  max_attempts = 1;
> +  if (state[3].set)
> +    {
> +      const char *max_attempts_str = state[3].arg;

Please drop that variable and use state[3].arg directly.

> +      if (max_attempts_str)
> +        max_attempts = grub_strtoul (max_attempts_str, NULL, 10);
> +    }
> +
> +  if (max_attempts == 0)
> +    max_attempts = 1;

     max_attempts = max_attempts ? max_attempts : 1;

> +
>    have_it = 0;
>    if (state[0].set)
>      {
> @@ -1141,8 +1155,8 @@ GRUB_MOD_INIT (cryptodisk)
>  {
>    grub_disk_dev_register (&grub_cryptodisk_dev);
>    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> -                  N_("SOURCE|-u UUID|-a|-b"),
> -                  N_("Mount a crypto device."), options);
> +                              N_("[-t TRIES]SOURCE|-u UUID|-a|-b"),

I think that this would be better:
                                 N_("SOURCE|-u UUID|-a|-b [-t TRIES]"),

> +                              N_("Mount a crypto device."), options);
>    grub_procfs_register ("luks_script", &luks_script);
>  }
>
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 86c50c612..b7c9d72ec 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -308,10 +308,10 @@ 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)
> +luks_recover_key_attempt (grub_disk_t source,
> +                          grub_cryptodisk_t dev,
> +                          struct grub_luks_phdr header)
>  {
> -  struct grub_luks_phdr header;
>    grub_size_t keysize;
>    grub_uint8_t *split_key = NULL;
>    char passphrase[MAX_PASSPHRASE] = "";
> @@ -322,10 +322,6 @@ luks_recover_key (grub_disk_t source,
>    grub_size_t max_stripes = 1;
>    char *tmp;
>
> -  err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> -  if (err)
> -    return err;
> -
>    grub_puts_ (N_("Attempting to decrypt master key..."));
>    keysize = grub_be_to_cpu32 (header.keyBytes);
>    if (keysize > GRUB_CRYPTODISK_MAX_KEYLEN)
> @@ -467,6 +463,32 @@ luks_recover_key (grub_disk_t source,
>    return GRUB_ACCESS_DENIED;
>  }
>
> +static grub_err_t
> +luks_recover_key (grub_disk_t source,
> +                  grub_cryptodisk_t dev)
> +{
> +    grub_err_t err;
> +    struct grub_luks_phdr header;
> +    unsigned long i;
> +
> +    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> +    if (err)
> +        return err;
> +
> +    err = GRUB_ERR_FILE_NOT_FOUND;

This seems unneeded...

> +    for (i  = 0; i < max_attempts; i++)

Redundant space between "i" and "=".

> +      {
> +        /* clear last failed attempt which assigns
> +            grub_errno = GRUB_ERR_ACCESS_DENIED.
> +           if the last attempt fails, grub_errno is not reset. */

Multiline comments should look like:
/*
 *
 ...
 */

And please rephrase it.

> +        grub_errno = GRUB_ERR_NONE;
> +        err = luks_recover_key_attempt(source, dev, header);
> +        if (err != GRUB_ERR_ACCESS_DENIED)
> +            break;

You can do "return err" immediately from here.

> +      }
> +    return err;
> +}
> +
>  struct grub_cryptodisk_dev luks_crypto = {
>    .scan = configure_ciphers,
>    .recover_key = luks_recover_key
> diff --git a/grub-core/osdep/aros/config.c b/grub-core/osdep/aros/config.c
> index c82d0ea8e..2bd23951e 100644
> --- a/grub-core/osdep/aros/config.c
> +++ b/grub-core/osdep/aros/config.c
> @@ -74,6 +74,8 @@ grub_util_load_config (struct grub_util_config *cfg)
>    if (v && v[0] == 'y' && v[1] == '\0')
>      cfg->is_cryptodisk_enabled = 1;
>
> +  cfg->cryptodisk_attempts =
> grub_util_strtoul_with_default(getenv("GRUB_CRYPTODISK_ATTEMPTS"), 1);
> +
>    v = getenv ("GRUB_DISTRIBUTOR");
>    if (v)
>      cfg->grub_distributor = xstrdup (v);
> diff --git a/grub-core/osdep/unix/config.c b/grub-core/osdep/unix/config.c
> index 65effa9f3..00ca5c854 100644
> --- a/grub-core/osdep/unix/config.c
> +++ b/grub-core/osdep/unix/config.c
> @@ -78,6 +78,8 @@ grub_util_load_config (struct grub_util_config *cfg)
>    if (v && v[0] == 'y' && v[1] == '\0')
>      cfg->is_cryptodisk_enabled = 1;
>
> +  cfg->cryptodisk_attempts =
> grub_util_strtoul_with_default(getenv("GRUB_CRYPTODISK_ATTEMPTS"), 1);
> +
>    v = getenv ("GRUB_DISTRIBUTOR");
>    if (v)
>      cfg->grub_distributor = xstrdup (v);
> @@ -105,8 +107,8 @@ grub_util_load_config (struct grub_util_config *cfg)
>        *ptr++ = *iptr;
>      }
>
> -  strcpy (ptr, "'; printf
> \"GRUB_ENABLE_CRYPTODISK=%s\\nGRUB_DISTRIBUTOR=%s\\n\" "
> -      "\"$GRUB_ENABLE_CRYPTODISK\" \"$GRUB_DISTRIBUTOR\"");
> +  strcpy (ptr, "'; printf
> \"GRUB_CRYPTODISK_ATTEMPTS=%s\\nGRUB_ENABLE_CRYPTODISK=%s\\nGRUB_DISTRIBUTOR=%s\\n\"
> "
> +  "\"$GRUB_CRYPTODISK_ATTEMPTS\" \"$GRUB_ENABLE_CRYPTODISK\"
> \"$GRUB_DISTRIBUTOR\"");

May I ask you to put GRUB_CRYPTODISK_ATTEMPTS behind GRUB_ENABLE_CRYPTODISK?

>
>    argv[2] = script;
>    argv[3] = '\0';
> diff --git a/grub-core/osdep/windows/config.c 
> b/grub-core/osdep/windows/config.c
> index 928ab1a49..765e1b7fb 100644
> --- a/grub-core/osdep/windows/config.c
> +++ b/grub-core/osdep/windows/config.c
> @@ -41,6 +41,8 @@ grub_util_load_config (struct grub_util_config *cfg)
>    if (v && v[0] == 'y' && v[1] == '\0')
>      cfg->is_cryptodisk_enabled = 1;
>
> +  cfg->cryptodisk_attempts =
> grub_util_strtoul_with_default(getenv("GRUB_CRYPTODISK_ATTEMPTS"), 1);
> +
>    v = getenv ("GRUB_DISTRIBUTOR");
>    if (v)
>      cfg->grub_distributor = xstrdup (v);
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 32f564ae0..5ad759a70 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -112,6 +112,7 @@ struct grub_cryptodisk_dev
>  };
>  typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
>
> +extern unsigned long EXPORT_VAR (max_attempts);
>  extern grub_cryptodisk_dev_t EXPORT_VAR (grub_cryptodisk_list);
>
>  #ifndef GRUB_LST_GENERATOR
> diff --git a/include/grub/emu/config.h b/include/grub/emu/config.h
> index 875d5896c..7b5563378 100644
> --- a/include/grub/emu/config.h
> +++ b/include/grub/emu/config.h
> @@ -37,6 +37,7 @@ struct grub_util_config
>  {
>    int is_cryptodisk_enabled;
>    char *grub_distributor;
> +  unsigned long cryptodisk_attempts;
>  };
>
>  void
> diff --git a/include/grub/util/misc.h b/include/grub/util/misc.h
> index e9e0a6724..dd51f3956 100644
> --- a/include/grub/util/misc.h
> +++ b/include/grub/util/misc.h
> @@ -49,4 +49,6 @@ void grub_util_host_init (int *argc, char ***argv);
>
>  int grub_qsort_strcmp (const void *, const void *);
>
> +unsigned long grub_util_strtoul_with_default(const char *str,
> unsigned long def);
> +
>  #endif /* ! GRUB_UTIL_MISC_HEADER */
> diff --git a/util/config.c b/util/config.c
> index ebcdd8f5e..ed8b8357a 100644
> --- a/util/config.c
> +++ b/util/config.c
> @@ -23,6 +23,27 @@
>  #include <grub/emu/config.h>
>  #include <grub/util/misc.h>
>
> +unsigned long
> +grub_util_strtoul_with_default(const char *str, unsigned long def)
> +{
> +    unsigned long result = 0;
> +
> +    if (str)
> +      {
> +        /* avoid grub_errno=GRUB_ERR_BAD_NUMBER in grub_strtoul on empty */
> +        while (grub_isspace (*str))
> +            str++;
> +        if (*str != '\0')
> +            result = grub_strtoul(str, NULL, 10);

I think that you should check grub_errno here...

> +      }
> +
> +    if (result == 0)
> +        result = def;
> +
> +    return result;

You can replace if and return with
  return result ? result : def;

Daniel