Re: [PATCH v2] cryptodisk: allow the user to retry failed passphrases

[Glenn Washburn]

Its also a good idea to always CC Daniel on patches. They may more easily
get dropped otherwise.

On Sun, 07 Apr 2024 14:52:32 -0700
Forest <forestix@nom.one> wrote:

> Changes since last rev:
> - replace some spaces with tabs
> - better explain the clearing of grub_errno
> - let an environment variable override the number of passphrase tries
> 
> Thanks to Glenn Washburn for reviewing.

The above will be the commit message for this change, which is not what
we want. The above are notes about this patch. To have text in a patch
that does not go into the commit message place the text after a line
containing only "---". There should probably be a body to the commit
message describing the change in more detail than is in the subject line.
See other commit messages for an idea. Single line commit messages are
generally only for very trivial commits. See further comments below.

> 
> Signed-off-by: Forest <forestix@nom.one>
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index a225f9a88..6ac603a32 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3277,6 +3277,7 @@ These variables have special meaning to GRUB.
>  * color_normal::
>  * config_directory::
>  * config_file::
> +* cryptodisk_passphrase_tries::
>  * debug::
>  * default::
>  * fallback::
> @@ -3441,6 +3442,14 @@ processed by commands @command{configfile} 
> (@pxref{configfile}) or @command{norm
>  (@pxref{normal}).  It is restored to the previous value when command 
> completes.
>  
>  
> +@node cryptodisk_passphrase_tries
> +@subsection cryptodisk_passphrase_tries
> +
> +When prompting the user for a cryptodisk passphrase, allow this many attempts
> +before giving up. The default is @samp{3}. (The user can give up early by
> +entering an empty passphrase.)
> +
> +
>  @node debug
>  @subsection debug
>  
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 2246af51b..3b2211123 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -17,6 +17,7 @@
>   */
>  
>  #include <grub/cryptodisk.h>
> +#include <grub/env.h>
>  #include <grub/mm.h>
>  #include <grub/misc.h>
>  #include <grub/dl.h>
> @@ -1063,6 +1064,8 @@ grub_cryptodisk_scan_device_real (const char *name,
>    grub_cryptodisk_dev_t cr;
>    struct cryptodisk_read_hook_ctx read_hook_data = {0};
>    int askpass = 0;
> +  const char *tries_env;
> +  grub_size_t tries;
>    char *part = NULL;
>  
>    dev = grub_cryptodisk_get_by_source_disk (source);
> @@ -1114,33 +1117,55 @@ grub_cryptodisk_scan_device_real (const char *name,
>      if (!dev)
>        continue;
>  
> -    if (!cargs->key_len)
> +    if (cargs->key_len)
> +      {
> +     ret = cr->recover_key (source, dev, cargs);
> +     if (ret != GRUB_ERR_NONE)
> +       goto error;
> +      }
> +    else
>        {
>       /* Get the passphrase from the user, if no key data. */
>       askpass = 1;
> -     part = grub_partition_get_name (source->partition);
> -     grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> -                  source->partition != NULL ? "," : "",
> -                  part != NULL ? part : N_("UNKNOWN"),
> -                  dev->uuid);
> -     grub_free (part);
> -
>       cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
>       if (cargs->key_data == NULL)
>         goto error_no_close;
>  
> -     if (!grub_password_get ((char *) cargs->key_data, 
> GRUB_CRYPTODISK_MAX_PASSPHRASE))
> +     tries_env = grub_env_get ("cryptodisk_passphrase_tries");
> +     tries = tries_env ? grub_strtoul (tries_env, 0, 0) : 3;

This does not account for tries_env not being a numeric value with no
garbage at the end or the empty string. I think the above line should be
replaced with:

if (tries_env != NULL && tries_env[0] != '\0')
  {
    const char *p = NULL;
    tries = grub_strtoul (tries_env, &p, 0);

    if (*p != '\0')
      {
        grub_error (grub_errno,
                    N_("non-numeric or invalid value for 
cryptodisk_passphrase_tries: `%s'"),
                    tries_env);
        goto error;
      }
  }

And the variable tries should be initialized to 3.

Glenn

> +     for (; tries > 0; tries--)
>         {
> -         grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
> -         goto error;
> +         part = grub_partition_get_name (source->partition);
> +         grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), 
> source->name,
> +                      source->partition != NULL ? "," : "",
> +                      part != NULL ? part : N_("UNKNOWN"),
> +                      dev->uuid);
> +         grub_free (part);
> +
> +         if (!grub_password_get ((char *) cargs->key_data, 
> GRUB_CRYPTODISK_MAX_PASSPHRASE))
> +           {
> +             grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
> +             goto error;
> +           }
> +         cargs->key_len = grub_strlen ((char *) cargs->key_data);
> +
> +         ret = cr->recover_key (source, dev, cargs);
> +         if (ret == GRUB_ERR_NONE)
> +           break;
> +         if (ret != GRUB_ERR_ACCESS_DENIED || tries == 1)
> +           goto error;
> +         grub_puts_ (N_("Invalid passphrase."));
> +
> +         /*
> +          * Since recover_key() calls a function that returns grub_errno,
> +          * a leftover error value from a previously rejected passphrase
> +          * will trigger a phantom failure. We therefore clear it before
> +          * trying a new passphrase.
> +          */
> +         grub_errno = GRUB_ERR_NONE;
>         }
> -     cargs->key_len = grub_strlen ((char *) cargs->key_data);
>        }
>  
> -    ret = cr->recover_key (source, dev, cargs);
> -    if (ret != GRUB_ERR_NONE)
> -      goto error;
> -
>      ret = grub_cryptodisk_insert (dev, name, source);
>      if (ret != GRUB_ERR_NONE)
>        goto error;