On Fri, Oct 04, 2019 at 09:11:09PM -0400, Jason Kushmaul wrote: > Hello, > > Daniel or other reviewer, > > I was hoping to get a review for my accessibility patch offering motor > impaired individuals more access to full disk encrypted disks, by > allowing a configurable retry option. > > I've addressed the review items from before from Daniel > > From d135f9f6b7d1503f551e8cced9ffe43a30af31d3 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. > > Reported-by: Jason J. Kushmaul <[email protected]> > Signed-off-by: Daniel Kiper <[email protected]>
Both lines should be replaced with Signed-off-by: Jason J. Kushmaul <[email protected]> > --- > 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. Please replace "The default if not present or zero is 1 to match original behavior. Currently only support with LUKS cryptodisks." with "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 +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; AIUI you do not use max_attempts outside of this module. If yes then static unsigned long max_attempts; here please. And you can drop EXPORT_VAR() from include/grub/cryptodisk.h. > 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; Please drop that variable and use state[3].arg directly. > + if (max_attempts_str) > + max_attempts = grub_strtoul (max_attempts_str, NULL, 10); > + } > + > + if (max_attempts == 0) > + max_attempts = 1; max_attempts = max_attempts ? 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"), I think that this would be better: 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..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; This seems unneeded... > + for (i = 0; i < max_attempts; i++) Redundant space between "i" and "=". > + { > + /* clear last failed attempt which assigns > + grub_errno = GRUB_ERR_ACCESS_DENIED. > + if the last attempt fails, grub_errno is not reset. */ Multiline comments should look like: /* * ... */ And please rephrase it. > + grub_errno = GRUB_ERR_NONE; > + err = luks_recover_key_attempt(source, dev, header); > + if (err != GRUB_ERR_ACCESS_DENIED) > + break; You can do "return err" immediately from here. > + } > + 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\""); May I ask you to put GRUB_CRYPTODISK_ATTEMPTS behind GRUB_ENABLE_CRYPTODISK? > > 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); I think that you should check grub_errno here... > + } > + > + if (result == 0) > + result = def; > + > + return result; You can replace if and return with return result ? result : def; Daniel _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
