Sorry for late reply but I was busy with release and other stuff.
On Sat, Jun 29, 2019 at 09:44:51PM -0400, Jason Kushmaul wrote:
> Add a configurable option for luks key recovery. Existing
> users will continue to have the default of 1 attempt.
>
> Cryptodisk is not accessible to motor impaired individuals
> who may need the extra attempts without having to reboot or
> manually rescue only to be asked again.
I like the longer comment which you put in one of your earlier emails.
So, please move it here.
> Reported-by: Jason J. Kushmaul <jasonkushm...@gmail.com>
Missing SOB.
> ---
> docs/grub.texi | 10 +++++++--
> grub-core/disk/cryptodisk.c | 24 +++++++++++++++++++---
> grub-core/disk/luks.c | 35 +++++++++++++++++++++++++-------
> grub-core/osdep/aros/config.c | 16 +++++++++++++++
> grub-core/osdep/unix/config.c | 20 ++++++++++++++++--
> grub-core/osdep/windows/config.c | 16 +++++++++++++++
> include/grub/emu/config.h | 1 +
> util/config.c | 14 +++++++++++++
> util/grub-install.c | 12 +++++------
> 9 files changed, 128 insertions(+), 20 deletions(-)
>
> 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,
s/luks/LUKS/ Please be consistent with the rest of the documentation...
> 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;
Please move this before grub_cryptodisk_list definition.
> 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)
Please drop this change.
> {
> 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");
> -
> +
This looks strange. Could you fix it?
> + /* 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)
Why we need that limit? And I think that grub_strtoul() would be better here.
> + {
> + max_attempts = (unsigned) tmpl;
> + }
You can drop the curly braces here.
> + }
> + }
Please fix coding style for ifs above. Curly braces are in wrong places and
number of spaces is wrong.
> +
> 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"),
s/[-t] /[-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..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;
Please move this to include/grub/cryptodisk.h.
> +static grub_err_t
> +luks_recover_key (grub_disk_t source,
> + grub_cryptodisk_t dev)
> +{
> + grub_err_t err;
> + struct grub_luks_phdr header;
Please add empty line here.
> + err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> + if (err)
> + return err;
> +
> + if (!max_attempts || max_attempts > 256)
> + max_attempts = 1;
This check, if at all, should be done in grub_cmd_cryptomount().
> + err = GRUB_ERR_FILE_NOT_FOUND;
> + for (unsigned attempt = 0; attempt < max_attempts; attempt++)
s/attempt/i/ and define it at the beginning of the function.
> + {
> + grub_errno = GRUB_ERR_NONE;
Why do you reset grub_errno here?
> + err = luks_recover_key_attempt(source, dev, header);
> + if (err != GRUB_ERR_ACCESS_DENIED)
> + break;
> + }
Wrong coding style for for().
> + 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);
s/grub_strtol/grub_strtoul/ And wrong coding style for this
function call. In general try to mimic the coding style in
this file.
> + if (attempts > 0 && attempts <= 256)
I do not see any valid reason for this limit.
> + {
> + cfg->cryptodisk_attempts = (unsigned) attempts;
> + }
Redundant curly braces.
> + else
> + {
> + grub_util_warn (_("grub_util_load_config:
> GRUB_CRYPTODISK_ATTEMPTS must be postive and less than 256"));
> + }
Ditto.
> + }
>
> 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"));
> + }
Same as above...
> + }
> +
> 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"));
> + }
> + }
Again... And could not you create one function and use it in different places?
> +
> 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;
> + }
Ditto again...
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
