Re: [PATCH] cryptodisk: add luks_recover_key attempts

[Jason Kushmaul]

Hello,

I've updated the patch addressing the issues and just wanted to give
this a bump.

I wasn't sure about the longer commit message - if you had meant my
email with headers and sections outlined
with asterisks.  That seemed to verbose to me for that have been what
you meant.  If it was I will for sure update that.

I had to clear grub_errno because the operation that fails within the
attempt sets grub_errno - I tried to address just by adding a comment.

Thanks!

Jason

This patch can be reviewed on my gitlab too if anyone finds it helpful:
https://gitlab.com/jkushmaul/grub2/compare/master...luks_recover_key_with_attempts_squashed?view=inline

or inline:

>From d135f9f6b7d1503f551e8cced9ffe43a30af31d3 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.

Reported-by: Jason J. Kushmaul <address@hidden>
Signed-off-by: Daniel Kiper <address@hidden>
---
 docs/grub.texi                   | 10 +++++++--
 grub-core/disk/cryptodisk.c      | 18 ++++++++++++++--
 grub-core/disk/luks.c            | 36 +++++++++++++++++++++++++-------
 grub-core/osdep/aros/config.c    |  2 ++
 grub-core/osdep/unix/config.c    |  6 ++++--
 grub-core/osdep/windows/config.c |  2 ++
 include/grub/cryptodisk.h        |  1 +
 include/grub/emu/config.h        |  1 +
 include/grub/util/misc.h         |  2 ++
 util/config.c                    | 29 +++++++++++++++++++++++++
 util/grub-install.c              | 12 +++++------
 11 files changed, 100 insertions(+), 19 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 3d50b16ba..01dc1114c 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 zero is 1 to match original
+behavior.  Currently only support with LUKS cryptodisks.
+
 @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 +4200,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..a3f0fa44d 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;
 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,18 @@ 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)
+        max_attempts = grub_strtoul (max_attempts_str, NULL, 10);
+    }
+
+  if (max_attempts == 0)
+    max_attempts = 1;
+
   have_it = 0;
   if (state[0].set)
     {
@@ -1141,8 +1155,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_("[-t TRIES]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..b7c9d72ec 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,32 @@ 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;
+
+    err = GRUB_ERR_FILE_NOT_FOUND;
+    for (i  = 0; i < max_attempts; i++)
+      {
+        /* clear last failed attempt which assigns
+            grub_errno = GRUB_ERR_ACCESS_DENIED.
+           if the last attempt fails, grub_errno is not reset. */
+        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..2bd23951e 100644
--- a/grub-core/osdep/aros/config.c
+++ b/grub-core/osdep/aros/config.c
@@ -74,6 +74,8 @@ 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 =
grub_util_strtoul_with_default(getenv("GRUB_CRYPTODISK_ATTEMPTS"), 1);
+
   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..00ca5c854 100644
--- a/grub-core/osdep/unix/config.c
+++ b/grub-core/osdep/unix/config.c
@@ -78,6 +78,8 @@ 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 =
grub_util_strtoul_with_default(getenv("GRUB_CRYPTODISK_ATTEMPTS"), 1);
+
   v = getenv ("GRUB_DISTRIBUTOR");
   if (v)
     cfg->grub_distributor = xstrdup (v);
@@ -105,8 +107,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..765e1b7fb 100644
--- a/grub-core/osdep/windows/config.c
+++ b/grub-core/osdep/windows/config.c
@@ -41,6 +41,8 @@ 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 =
grub_util_strtoul_with_default(getenv("GRUB_CRYPTODISK_ATTEMPTS"), 1);
+
   v = getenv ("GRUB_DISTRIBUTOR");
   if (v)
     cfg->grub_distributor = xstrdup (v);
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 32f564ae0..5ad759a70 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -112,6 +112,7 @@ struct grub_cryptodisk_dev
 };
 typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;

+extern unsigned long EXPORT_VAR (max_attempts);
 extern grub_cryptodisk_dev_t EXPORT_VAR (grub_cryptodisk_list);

 #ifndef GRUB_LST_GENERATOR
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/include/grub/util/misc.h b/include/grub/util/misc.h
index e9e0a6724..dd51f3956 100644
--- a/include/grub/util/misc.h
+++ b/include/grub/util/misc.h
@@ -49,4 +49,6 @@ void grub_util_host_init (int *argc, char ***argv);

 int grub_qsort_strcmp (const void *, const void *);

+unsigned long grub_util_strtoul_with_default(const char *str,
unsigned long def);
+
 #endif /* ! GRUB_UTIL_MISC_HEADER */
diff --git a/util/config.c b/util/config.c
index ebcdd8f5e..ed8b8357a 100644
--- a/util/config.c
+++ b/util/config.c
@@ -23,6 +23,27 @@
 #include <grub/emu/config.h>
 #include <grub/util/misc.h>

+unsigned long
+grub_util_strtoul_with_default(const char *str, unsigned long def)
+{
+    unsigned long result = 0;
+
+    if (str)
+      {
+        /* avoid grub_errno=GRUB_ERR_BAD_NUMBER in grub_strtoul on empty */
+        while (grub_isspace (*str))
+            str++;
+        if (*str != '\0')
+            result = grub_strtoul(str, NULL, 10);
+      }
+
+    if (result == 0)
+        result = def;
+
+    return result;
+}
+
+
 void
 grub_util_parse_config (FILE *f, struct grub_util_config *cfg, int simple)
 {
@@ -32,6 +53,14 @@ 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;
+          cfg->cryptodisk_attempts = grub_util_strtoul_with_default(ptr, 1);
+          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);
     }
  }
-- 
2.21.0