Re: [PATCH] cryptodisk: add luks_recover_key attempts option

[Daniel Kiper]

On Sat, Nov 02, 2019 at 06:23:49PM -0400, Jason Kushmaul wrote:
> Hi Daniel,
>
> Please see revised patch addressing all of your comments in the
> attached patch file.
>
> Jason

> From 7a2b845f421d8605e139b2a7e41c2d7722c969c3 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.
>
> Signed-off-by: Jason J. Kushmaul <address@hidden>
> ---
>  docs/grub.texi                   |  9 ++++++--
>  grub-core/disk/cryptodisk.c      | 15 ++++++++++--
>  grub-core/disk/luks.c            | 39 ++++++++++++++++++++++++++------
>  grub-core/osdep/aros/config.c    |  4 ++++
>  grub-core/osdep/unix/config.c    |  9 ++++++--
>  grub-core/osdep/windows/config.c |  4 ++++
>  include/grub/emu/config.h        |  1 +
>  util/config.c                    | 18 +++++++++++++++
>  util/grub-install.c              | 12 +++++-----
>  9 files changed, 92 insertions(+), 19 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 3d50b16ba..9e6d7ad4e 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1508,6 +1508,10 @@ 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 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 +4199,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..2f3e0e851 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;

Does it really need to be a global variable? Could not you add this
to existing structs or pass as an argument to relevant function?

>  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,15 @@ 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)
> +    {
> +      if (state[3].arg)
> +     max_attempts = grub_strtoul (state[3].arg, NULL, 10);

Please do not ignore endptr and check for grub_strtoul() errors.
"man strtoul" is your friend.

> +    }
> +  max_attempts = max_attempts ? max_attempts : 1;
> +
>    have_it = 0;
>    if (state[0].set)
>      {
> @@ -1141,8 +1152,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_("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..60d6dc76b 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -33,6 +33,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>
>  #define LUKS_KEY_ENABLED  0x00AC71F3
>
> +extern unsigned long max_attempts;

Could not you find a batter way of passing this from one module to
another? If no please change its name to something less generic.

>  /* On disk LUKS header */
>  struct grub_luks_phdr
>  {
> @@ -308,10 +310,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 +324,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 +465,33 @@ 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;
> +
> +    max_attempts = max_attempts ? max_attempts : 1;

I think that this is redundant. You did it earlier.

> +    for (i = 0; i < max_attempts; i++)
> +      {
> +        /* When i > 0, the previous failed attempt will have
> +         * a grub_errno == GRUB_ERR_ACCESS_DENIED
> +         */
> +        grub_errno = GRUB_ERR_NONE;
> +        err = luks_recover_key_attempt(source, dev, header);
> +        /* Anything other than GRUB_ERR_ACCESS_DENIED is success, or 
> unrecoverable. */
> +        if (err != GRUB_ERR_ACCESS_DENIED && err != GRUB_ERR_BAD_ARGUMENT)
> +            return err;
> +      }
> +    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..48959287f 100644
> --- a/grub-core/osdep/aros/config.c
> +++ b/grub-core/osdep/aros/config.c
> @@ -74,6 +74,10 @@ grub_util_load_config (struct grub_util_config *cfg)
>    if (v && v[0] == 'y' && v[1] == '\0')
>      cfg->is_cryptodisk_enabled = 1;
>
> +  v = getenv("GRUB_CRYPTODISK_ATTEMPTS");
> +  if (v)
> +    cfg->cryptodisk_attempts = grub_strtoul(v, 0, 0);

"man strtoul". And I think that base should be 10.

>    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..d1c5ea1f8 100644
> --- a/grub-core/osdep/unix/config.c
> +++ b/grub-core/osdep/unix/config.c
> @@ -78,6 +78,10 @@ grub_util_load_config (struct grub_util_config *cfg)
>    if (v && v[0] == 'y' && v[1] == '\0')
>      cfg->is_cryptodisk_enabled = 1;
>
> +  v = getenv("GRUB_CRYPTODISK_ATTEMPTS");
> +  if (v)
> +    cfg->cryptodisk_attempts = grub_strtoul(v, 0, 0);

Ditto.

> +
>    v = getenv ("GRUB_DISTRIBUTOR");
>    if (v)
>      cfg->grub_distributor = xstrdup (v);
> @@ -105,8 +109,9 @@ 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_ENABLE_CRYPTODISK=%s\\nGRUB_CRYPTODISK_ATTEMPTS=%s\\nGRUB_DISTRIBUTOR=%s\\n\"
>  "
> +    "\"$GRUB_ENABLE_CRYPTODISK\" \"$GRUB_CRYPTODISK_ATTEMPTS\" 
> \"$GRUB_DISTRIBUTOR\"");
>
>    argv[2] = script;
>    argv[3] = '\0';
> diff --git a/grub-core/osdep/windows/config.c 
> b/grub-core/osdep/windows/config.c
> index 928ab1a49..9397bc8e3 100644
> --- a/grub-core/osdep/windows/config.c
> +++ b/grub-core/osdep/windows/config.c
> @@ -41,6 +41,10 @@ grub_util_load_config (struct grub_util_config *cfg)
>    if (v && v[0] == 'y' && v[1] == '\0')
>      cfg->is_cryptodisk_enabled = 1;
>
> +  v = getenv("GRUB_CRYPTODISK_ATTEMPTS");
> +  if (v)
> +    cfg->cryptodisk_attempts = grub_strtoul(v, 0, 0);

Ditto.

> +
>    v = getenv ("GRUB_DISTRIBUTOR");
>    if (v)
>      cfg->grub_distributor = xstrdup (v);
> 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/util/config.c b/util/config.c
> index ebcdd8f5e..572cb8a90 100644
> --- a/util/config.c
> +++ b/util/config.c
> @@ -23,6 +23,7 @@
>  #include <grub/emu/config.h>
>  #include <grub/util/misc.h>
>
> +

Please drop this change.

>  void
>  grub_util_parse_config (FILE *f, struct grub_util_config *cfg, int simple)
>  {
> @@ -42,6 +43,23 @@ grub_util_parse_config (FILE *f, struct grub_util_config 
> *cfg, int simple)
>           cfg->is_cryptodisk_enabled = 1;
>         continue;
>       }
> +      if (grub_strncmp (ptr, "GRUB_CRYPTODISK_ATTEMPTS=",
> +                     sizeof ("GRUB_CRYPTODISK_ATTEMPTS=") - 1) == 0)
> +     {
> +       ptr += sizeof ("GRUB_CRYPTODISK_ATTEMPTS=") - 1;
> +       if (grub_strlen(ptr) > 1)
> +         {
> +           cfg->cryptodisk_attempts = grub_strtoul(ptr, 0, 0);

Ditto. And please do not ignore endptr. Same above...

Additionally, LUKS2 patches are on their way and I want to see this
functionality in LUKS2 too. So, if you can wait until I will merge
them that would be nice.

Daniel