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

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

[Forest]

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.

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;
+	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;

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Forest]

On Sun, 07 Apr 2024 14:52:32 -0700, Forest 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

As a new contributor, is there something I should do to ensure my patch
isn't overlooked?  I'm not sure what the development cadence here is like,
and since new patches to the same code have arrived in the weeks since I
submitted this one, I'm starting to worry that I might have to start all
over again on a new code base.

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Patrick Plenefisch]

[-- Attachment #1.1: Type: text/plain, Size: 808 bytes --]

On Mon, Apr 22, 2024 at 1:48 PM Forest <forestix@nom.one> wrote:

> On Sun, 07 Apr 2024 14:52:32 -0700, Forest 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
>
> As a new contributor, is there something I should do to ensure my patch
> isn't overlooked?  I'm not sure what the development cadence here is like,
> and since new patches to the same code have arrived in the weeks since I
> submitted this one, I'm starting to worry that I might have to start all
> over again on a new code base.
>
>
As another new contributor, I'm also curious about this. I've been waiting
since january & febuary, for any update on my patch, though

Patrick

[-- Attachment #1.2: Type: text/html, Size: 1160 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Glenn Washburn]

On Mon, 22 Apr 2024 10:45:29 -0700
Forest <forestix@nom.one> wrote:

> On Sun, 07 Apr 2024 14:52:32 -0700, Forest 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
> 
> As a new contributor, is there something I should do to ensure my patch
> isn't overlooked?  I'm not sure what the development cadence here is like,
> and since new patches to the same code have arrived in the weeks since I
> submitted this one, I'm starting to worry that I might have to start all
> over again on a new code base.

Asking about the status of the patch in a reply to the thread can help
ensure its not overlooked. Patches do get overlooked here and if the
author is not active in pursuing the patch, there's a good chance it
will be overlooked. The development cadence might be described as
glacial, but it also depends on various factors. In your case, I
suspect one factor is that I suspect Daniel is waiting for me to review
this v2. I tend to look at patches related to cryptomount. However, I
don't have the bandwidth to check in on the list more than a few times
a month and sometimes less. So yeah, glacial.

You shouldn't have to start all over, but yes you might need to rebase
on upcoming changes depending on when this gets in.

Glenn

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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;

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Forest]

On Sat, 27 Apr 2024 04:18:29 -0500, Glenn Washburn wrote:

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

Thanks. Will do.

>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;
>      }
>  }

This doesn't account for grub_strtoul() ignoring trailing text, so it
generates an error with a potentially stale/misleading grub_errno value.

My next rev will include your suggestion with a fix for that case.

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel