Re: [PATCH] cryptodisk: add luks_recover_key attempts

[Daniel Kiper]

Sorry for late reply but I was busy with release and other stuff.

On Sat, Jun 29, 2019 at 09:44:51PM -0400, Jason Kushmaul wrote:
> Add a configurable option for luks key recovery.  Existing
> users will continue to have the default of 1 attempt.
>
> Cryptodisk is not accessible to motor impaired individuals
> who may need the extra attempts without having to reboot or
> manually rescue only to be asked again.

I like the longer comment which you put in one of your earlier emails.
So, please move it here.

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

Missing SOB.

> ---
>  docs/grub.texi                   | 10 +++++++--
>  grub-core/disk/cryptodisk.c      | 24 +++++++++++++++++++---
>  grub-core/disk/luks.c            | 35 +++++++++++++++++++++++++-------
>  grub-core/osdep/aros/config.c    | 16 +++++++++++++++
>  grub-core/osdep/unix/config.c    | 20 ++++++++++++++++--
>  grub-core/osdep/windows/config.c | 16 +++++++++++++++
>  include/grub/emu/config.h        |  1 +
>  util/config.c                    | 14 +++++++++++++
>  util/grub-install.c              | 12 +++++------
>  9 files changed, 128 insertions(+), 20 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 308b25074..00df49fa1 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 outside of [1,256] is 1.
> +Currently only supported for LUKS.
> +
>  @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
> @@ -4194,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,

s/luks/LUKS/ Please be consistent with the rest of the documentation...

> 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..3b90e550e 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -41,6 +41,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}
>    };
>
> @@ -807,6 +808,7 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>
>  #endif
>
> +unsigned max_attempts;

Please move this before grub_cryptodisk_list definition.

>  static int check_boot, have_it;
>  static char *search_uuid;
>
> @@ -820,7 +822,7 @@ cryptodisk_close (grub_cryptodisk_t dev)
>  }
>
>  static grub_err_t
> -grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> +grub_cryptodisk_scan_device_real(const char *name, grub_disk_t source)

Please drop this change.

>  {
>    grub_err_t err;
>    grub_cryptodisk_t dev;
> @@ -933,7 +935,23 @@ 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");
> -
> +

This looks strange. Could you fix it?

> +  /* Default to original behavior of 1 attempt */
> +  max_attempts = 1;
> +  if (state[3].set)
> +  {
> +      const char *max_attempts_str = state[3].arg;
> +      if (max_attempts_str)
> +      {
> +          char *end = NULL;
> +          long  tmpl = grub_strtol (max_attempts_str, &end, 10);
> +          if (tmpl > 0 && tmpl <= 256)

Why we need that limit? And I think that grub_strtoul() would be better here.

> +          {
> +              max_attempts = (unsigned) tmpl;
> +          }

You can drop the curly braces here.

> +      }
> +  }

Please fix coding style for ifs above. Curly braces are in wrong places and
number of spaces is wrong.

> +
>    have_it = 0;
>    if (state[0].set)
>      {
> @@ -1141,7 +1159,7 @@ 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_("[-t] SOURCE|-u UUID|-a|-b"),

s/[-t] /[-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..02b999f07 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,31 @@ luks_recover_key (grub_disk_t source,
>    return GRUB_ACCESS_DENIED;
>  }
>
> +
> +extern unsigned max_attempts;

Please move this to include/grub/cryptodisk.h.

> +static grub_err_t
> +luks_recover_key (grub_disk_t source,
> +                  grub_cryptodisk_t dev)
> +{
> +    grub_err_t err;
> +    struct grub_luks_phdr header;

Please add empty line here.

> +    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> +    if (err)
> +        return err;
> +
> +    if (!max_attempts || max_attempts > 256)
> +      max_attempts = 1;

This check, if at all, should be done in grub_cmd_cryptomount().

> +    err = GRUB_ERR_FILE_NOT_FOUND;
> +    for (unsigned attempt = 0; attempt < max_attempts; attempt++)

s/attempt/i/ and define it at the beginning of the function.

> +    {
> +        grub_errno = GRUB_ERR_NONE;

Why do you reset grub_errno here?

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

Wrong coding style for for().

> +    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..683823233 100644
> --- a/grub-core/osdep/aros/config.c
> +++ b/grub-core/osdep/aros/config.c
> @@ -73,6 +73,22 @@ grub_util_load_config (struct grub_util_config *cfg)
>    v = getenv ("GRUB_ENABLE_CRYPTODISK");
>    if (v && v[0] == 'y' && v[1] == '\0')
>      cfg->is_cryptodisk_enabled = 1;
> +
> +  cfg->cryptodisk_attempts = 1;
> +  v = getenv("GRUB_CRYPTODISK_ATTEMPTS");
> +  if (v)
> +    {
> +      char *end = NULL;
> +      long attempts = grub_strtol(v, &end, 10);

s/grub_strtol/grub_strtoul/ And wrong coding style for this
function call. In general try to mimic the coding style in
this file.

> +      if (attempts > 0 && attempts <= 256)

I do not see any valid reason for this limit.

> +        {
> +          cfg->cryptodisk_attempts = (unsigned) attempts;
> +        }

Redundant curly braces.

> +      else
> +        {
> +          grub_util_warn (_("grub_util_load_config:
> GRUB_CRYPTODISK_ATTEMPTS must be postive and less than 256"));
> +        }

Ditto.

> +    }
>
>    v = getenv ("GRUB_DISTRIBUTOR");
>    if (v)
> diff --git a/grub-core/osdep/unix/config.c b/grub-core/osdep/unix/config.c
> index 65effa9f3..3cf0f21c6 100644
> --- a/grub-core/osdep/unix/config.c
> +++ b/grub-core/osdep/unix/config.c
> @@ -78,6 +78,22 @@ 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 = 1;
> +  v = getenv("GRUB_CRYPTODISK_ATTEMPTS");
> +  if (v)
> +  {
> +      char *end = NULL;
> +      long attempts = grub_strtol(v, &end, 10);
> +      if (attempts > 0 && attempts <= 256)
> +      {
> +          cfg->cryptodisk_attempts = (unsigned) attempts;
> +      }
> +      else
> +      {
> +          grub_util_warn (_("grub_util_load_config:
> GRUB_CRYPTODISK_ATTEMPTS must be postive and less than 256"));
> +      }

Same as above...

> +  }
> +
>    v = getenv ("GRUB_DISTRIBUTOR");
>    if (v)
>      cfg->grub_distributor = xstrdup (v);
> @@ -105,8 +121,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\"");
>
>    argv[2] = script;
>    argv[3] = '\0';
> diff --git a/grub-core/osdep/windows/config.c
> b/grub-core/osdep/windows/config.c
> index 928ab1a49..2e3808f22 100644
> --- a/grub-core/osdep/windows/config.c
> +++ b/grub-core/osdep/windows/config.c
> @@ -41,6 +41,22 @@ 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 = 1;
> +  v = getenv("GRUB_CRYPTODISK_ATTEMPTS");
> +  if (v)
> +  {
> +      char *end = NULL;
> +      long attempts = grub_strtol(v, &end, 10);
> +      if (attempts > 0 && attempts <= 256)
> +      {
> +          cfg->cryptodisk_attempts = (unsigned) attempts;
> +      }
> +      else
> +      {
> +          grub_util_warn (_("grub_util_load_config:
> GRUB_CRYPTODISK_ATTEMPTS must be postive and less than 256"));
> +      }
> +  }

Again... And could not you create one function and use it in different places?

> +
>    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..fd4cb9e33 100644
> --- a/include/grub/emu/config.h
> +++ b/include/grub/emu/config.h
> @@ -36,6 +36,7 @@ grub_util_get_localedir (void);
>  struct grub_util_config
>  {
>    int is_cryptodisk_enabled;
> +  unsigned cryptodisk_attempts;
>    char *grub_distributor;
>  };
>
> diff --git a/util/config.c b/util/config.c
> index ebcdd8f5e..9d460712c 100644
> --- a/util/config.c
> +++ b/util/config.c
> @@ -32,6 +32,20 @@ grub_util_parse_config (FILE *f, struct grub_util_config
> *cfg, int simple)
>      {
>        const char *ptr;
>        for (ptr = buffer; *ptr && grub_isspace (*ptr); ptr++);
> +        if (grub_strncmp (ptr, "GRUB_CRYPTODISK_ATTEMPTS=",
> +            sizeof ("GRUB_CRYPTODISK_ATTEMPTS=") - 1) == 0)
> +          {
> +            ptr += sizeof ("GRUB_CRYPTODISK_ATTEMPTS=") - 1;
> +            char *end = NULL;
> +            long tmpl = strtol(ptr, &end, 10);
> +            if (!tmpl || tmpl > 256) {
> +                grub_util_warn (_("grub_util_parse_config:
> GRUB_CRYPTODISK_ATTEMPTS was out of range=%ld, defaulting to 1"), tmpl);
> +                cfg->cryptodisk_attempts = 1;
> +            }
> +            cfg->cryptodisk_attempts = tmpl;
> +            continue;
> +          }

Ditto again...

Daniel