Re: cryptodisk enabled returns to rescue prompt

[Jason Kushmaul <jasonkushmaul@gmail.com>]

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

Hello,

I'm new to the list.  I've been working on this myself for personal
reasons.  I think this should be viewed as an accessibility issue, not as a
convenience.  Those that have motor impairments have a very difficult time
booting their machines without a reboot, or rescue.

Please see my patch attached which adds documentation, configuration, and
implementation.  The patch is against master
(4e7b5bb3be69633ed860cb74b0ef2c84a839523d) but I can change that if you
like.
I tested this in a virtualbox.

If a more formal request is needed, I prepared this before finding this
existing post:

**************************************
FEATURE
**************************************
Add LUKS full disk encryption passphrase retry config and logic, providing
accessibility to people with motor impairments, Parkinson's, etc.

**************************************
JUSTIFICATION
**************************************
As of master (4e7b5bb3be69633ed860cb74b0ef2c84a839523d), I've found no
other tickets mentioning this.

When cryptodisk attempts to recover the key, it asks for the passphrase,
just once.  You are required to reboot, or know how to recover grub
yourself manually.

Many people enjoy the confidence of encrypting their full disk, including
/boot. However, for those who may be plagued with motor impairment, shaking
of hands, twitches in the fingers as they type, one would have severe
barriers to enjoying that same level of security due to bneing required to
type a passphrase once, and getting it right without having to reboot again.

I know there is a concern for security.  This configuration would default
to 1 attempt as it is today, and those who chose, may choose any amount
they like up to 256.  Defaulting to 1 will maintain exactly the same
behavior for users upgrading.

**************************************
STEPS TO REPRODUCE
**************************************
Steps:
* Setup
* Observation

Setup:
* Encrypt the full disk using luks so that the /boot is contained in luks
disk.
* Use a passphrase 32 characters long with an equal distribution of
[0-9a-zA-Z] and specials.
* Boot and wait for passphrase prompt.

Observation:
Enter the incorrect password and hit enter.  You are not asked to retry, or
allowed to configure it before install  of grub on the full disk crypto
setup.  You must then type the full blown steps to ask again, or simply
CTL-ALT-DEL and wait 45 more seconds...

With the patches, one can configure with a "-t" and a number of retry
attempts.  They will see the same prompt, see a notification about key
recovery in progress, and if incorrect, another message stating such, but
then be prompted again on failure.

**************************************
SUMMARY
**************************************
Those with motor impairments have a barrier preventing them from enjoying
 LUKS full disk encryption with strong passphrases.  Causing them a need to
reboot until correct.

This is easy to reproduce, but a little more difficult to realize how
people with impaired motor function would struggle.

The changes in this patch offer a configurable way to increase the number
of attempts from 1, to any number <= 256, but maintains the default
behavior as all users expect, which is just 1 attempt.

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

[-- Attachment #2: master_luks_key_recover_with_attempts.patch --]
[-- Type: text/x-patch, Size: 11642 bytes --]

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, 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;
 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)
 {
   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");
-
+  
+  /* 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) 
+          {
+              max_attempts = (unsigned) tmpl;
+          }
+      }
+  }
+  
   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"),
 			      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;
+static grub_err_t
+luks_recover_key (grub_disk_t source,
+                  grub_cryptodisk_t dev)
+{
+    grub_err_t err;
+    struct grub_luks_phdr header;
+    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
+    if (err)
+        return err;
+    
+    if (!max_attempts || max_attempts > 256)
+      max_attempts = 1;
+    err = GRUB_ERR_FILE_NOT_FOUND;
+    for (unsigned attempt = 0; attempt < max_attempts; attempt++)
+    {
+        grub_errno = GRUB_ERR_NONE;
+        err = luks_recover_key_attempt(source, dev, header);
+        if (err != GRUB_ERR_ACCESS_DENIED)
+            break;
+    }
+    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);
+      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"));
+        }
+    }
 
   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"));
+      }
+  }
+
   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"));
+      }
+  }
+
   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;
+          }
+
       if (grub_strncmp (ptr, "GRUB_ENABLE_CRYPTODISK=",
 			sizeof ("GRUB_ENABLE_CRYPTODISK=") - 1) == 0)
 	{
diff --git a/util/grub-install.c b/util/grub-install.c
index 8a55ad4b8..081fcc446 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -509,7 +509,7 @@ have_bootdev (enum grub_install_plat pl)
 }
 
 static void
-probe_cryptodisk_uuid (grub_disk_t disk)
+probe_cryptodisk_uuid (grub_disk_t disk, unsigned attempts)
 {
   grub_disk_memberlist_t list = NULL, tmp;
 
@@ -520,7 +520,7 @@ probe_cryptodisk_uuid (grub_disk_t disk)
     }
   while (list)
     {
-      probe_cryptodisk_uuid (list->disk);
+      probe_cryptodisk_uuid (list->disk, attempts);
       tmp = list->next;
       free (list);
       list = tmp;
@@ -532,8 +532,8 @@ probe_cryptodisk_uuid (grub_disk_t disk)
 	load_cfg_f = grub_util_fopen (load_cfg, "wb");
       have_load_cfg = 1;
 
-      fprintf (load_cfg_f, "cryptomount -u %s\n",
-	      uuid);
+      fprintf (load_cfg_f, "cryptomount -u %s -t %u\n",
+               uuid, attempts);
     }
 }
 
@@ -1531,7 +1531,7 @@ main (int argc, char *argv[])
       if (config.is_cryptodisk_enabled)
 	{
 	  if (grub_dev->disk)
-	    probe_cryptodisk_uuid (grub_dev->disk);
+	    probe_cryptodisk_uuid (grub_dev->disk, config.cryptodisk_attempts);
 
 	  for (curdrive = grub_drives + 1; *curdrive; curdrive++)
 	    {
@@ -1539,7 +1539,7 @@ main (int argc, char *argv[])
 	      if (!dev)
 		continue;
 	      if (dev->disk)
-		probe_cryptodisk_uuid (dev->disk);
+		probe_cryptodisk_uuid (dev->disk, config.cryptodisk_attempts);
 	      grub_device_close (dev);
 	    }
 	}