On Sat, Nov 02, 2019 at 06:23:49PM -0400, Jason Kushmaul wrote: > Hi Daniel, > > Please see revised patch addressing all of your comments in the > attached patch file. > > Jason
> From 7a2b845f421d8605e139b2a7e41c2d7722c969c3 Mon Sep 17 00:00:00 2001 > From: "Jason J. Kushmaul" <[email protected]> > 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. > > Signed-off-by: Jason J. Kushmaul <[email protected]> > --- > docs/grub.texi | 9 ++++++-- > grub-core/disk/cryptodisk.c | 15 ++++++++++-- > grub-core/disk/luks.c | 39 ++++++++++++++++++++++++++------ > grub-core/osdep/aros/config.c | 4 ++++ > grub-core/osdep/unix/config.c | 9 ++++++-- > grub-core/osdep/windows/config.c | 4 ++++ > include/grub/emu/config.h | 1 + > util/config.c | 18 +++++++++++++++ > util/grub-install.c | 12 +++++----- > 9 files changed, 92 insertions(+), 19 deletions(-) > > diff --git a/docs/grub.texi b/docs/grub.texi > index 3d50b16ba..9e6d7ad4e 100644 > --- a/docs/grub.texi > +++ b/docs/grub.texi > @@ -1508,6 +1508,10 @@ 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 not present or zero the default is 1. Currently only LUKS > +cryptodisks support that option. > + > @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 +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..2f3e0e851 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; Does it really need to be a global variable? Could not you add this to existing structs or pass as an argument to relevant function? > 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,15 @@ 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) > + { > + if (state[3].arg) > + max_attempts = grub_strtoul (state[3].arg, NULL, 10); Please do not ignore endptr and check for grub_strtoul() errors. "man strtoul" is your friend. > + } > + max_attempts = max_attempts ? max_attempts : 1; > + > have_it = 0; > if (state[0].set) > { > @@ -1141,8 +1152,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_("SOURCE|-u UUID|-a|-b [-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..60d6dc76b 100644 > --- a/grub-core/disk/luks.c > +++ b/grub-core/disk/luks.c > @@ -33,6 +33,8 @@ GRUB_MOD_LICENSE ("GPLv3+"); > > #define LUKS_KEY_ENABLED 0x00AC71F3 > > +extern unsigned long max_attempts; Could not you find a batter way of passing this from one module to another? If no please change its name to something less generic. > /* On disk LUKS header */ > struct grub_luks_phdr > { > @@ -308,10 +310,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 +324,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 +465,33 @@ 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; > + > + max_attempts = max_attempts ? max_attempts : 1; I think that this is redundant. You did it earlier. > + for (i = 0; i < max_attempts; i++) > + { > + /* When i > 0, the previous failed attempt will have > + * a grub_errno == GRUB_ERR_ACCESS_DENIED > + */ > + grub_errno = GRUB_ERR_NONE; > + err = luks_recover_key_attempt(source, dev, header); > + /* Anything other than GRUB_ERR_ACCESS_DENIED is success, or > unrecoverable. */ > + if (err != GRUB_ERR_ACCESS_DENIED && err != GRUB_ERR_BAD_ARGUMENT) > + return err; > + } > + 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..48959287f 100644 > --- a/grub-core/osdep/aros/config.c > +++ b/grub-core/osdep/aros/config.c > @@ -74,6 +74,10 @@ grub_util_load_config (struct grub_util_config *cfg) > if (v && v[0] == 'y' && v[1] == '\0') > cfg->is_cryptodisk_enabled = 1; > > + v = getenv("GRUB_CRYPTODISK_ATTEMPTS"); > + if (v) > + cfg->cryptodisk_attempts = grub_strtoul(v, 0, 0); "man strtoul". And I think that base should be 10. > 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..d1c5ea1f8 100644 > --- a/grub-core/osdep/unix/config.c > +++ b/grub-core/osdep/unix/config.c > @@ -78,6 +78,10 @@ grub_util_load_config (struct grub_util_config *cfg) > if (v && v[0] == 'y' && v[1] == '\0') > cfg->is_cryptodisk_enabled = 1; > > + v = getenv("GRUB_CRYPTODISK_ATTEMPTS"); > + if (v) > + cfg->cryptodisk_attempts = grub_strtoul(v, 0, 0); Ditto. > + > v = getenv ("GRUB_DISTRIBUTOR"); > if (v) > cfg->grub_distributor = xstrdup (v); > @@ -105,8 +109,9 @@ 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_ENABLE_CRYPTODISK=%s\\nGRUB_CRYPTODISK_ATTEMPTS=%s\\nGRUB_DISTRIBUTOR=%s\\n\" > " > + "\"$GRUB_ENABLE_CRYPTODISK\" \"$GRUB_CRYPTODISK_ATTEMPTS\" > \"$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..9397bc8e3 100644 > --- a/grub-core/osdep/windows/config.c > +++ b/grub-core/osdep/windows/config.c > @@ -41,6 +41,10 @@ grub_util_load_config (struct grub_util_config *cfg) > if (v && v[0] == 'y' && v[1] == '\0') > cfg->is_cryptodisk_enabled = 1; > > + v = getenv("GRUB_CRYPTODISK_ATTEMPTS"); > + if (v) > + cfg->cryptodisk_attempts = grub_strtoul(v, 0, 0); Ditto. > + > 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..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/util/config.c b/util/config.c > index ebcdd8f5e..572cb8a90 100644 > --- a/util/config.c > +++ b/util/config.c > @@ -23,6 +23,7 @@ > #include <grub/emu/config.h> > #include <grub/util/misc.h> > > + Please drop this change. > void > grub_util_parse_config (FILE *f, struct grub_util_config *cfg, int simple) > { > @@ -42,6 +43,23 @@ grub_util_parse_config (FILE *f, struct grub_util_config > *cfg, int simple) > cfg->is_cryptodisk_enabled = 1; > continue; > } > + if (grub_strncmp (ptr, "GRUB_CRYPTODISK_ATTEMPTS=", > + sizeof ("GRUB_CRYPTODISK_ATTEMPTS=") - 1) == 0) > + { > + ptr += sizeof ("GRUB_CRYPTODISK_ATTEMPTS=") - 1; > + if (grub_strlen(ptr) > 1) > + { > + cfg->cryptodisk_attempts = grub_strtoul(ptr, 0, 0); Ditto. And please do not ignore endptr. Same above... Additionally, LUKS2 patches are on their way and I want to see this functionality in LUKS2 too. So, if you can wait until I will merge them that would be nice. Daniel _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
