* [PATCH] cryptodisk: add luks_recover_key attempts
@ 2019-06-30 1:44 Jason Kushmaul
2019-07-05 23:56 ` Jason Kushmaul
2019-07-11 13:17 ` Daniel Kiper
0 siblings, 2 replies; 9+ messages in thread
From: Jason Kushmaul @ 2019-06-30 1:44 UTC (permalink / raw)
To: grub-devel
[-- Attachment #1: Type: text/plain, Size: 12480 bytes --]
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.
Reported-by: Jason J. Kushmaul <jasonkushmaul@gmail.com>
---
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,
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);
}
}
--
2.20.1
[-- Attachment #2: Type: text/html, Size: 15163 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] cryptodisk: add luks_recover_key attempts
2019-06-30 1:44 [PATCH] cryptodisk: add luks_recover_key attempts Jason Kushmaul
@ 2019-07-05 23:56 ` Jason Kushmaul
2019-07-11 13:17 ` Daniel Kiper
1 sibling, 0 replies; 9+ messages in thread
From: Jason Kushmaul @ 2019-07-05 23:56 UTC (permalink / raw)
To: grub-devel
[-- Attachment #1: Type: text/plain, Size: 13607 bytes --]
Hi,
This is a pretty simple patch - I was hoping someone could review and
consider this patch, as well as provide me feedback if I've submitted it to
the list incorrectly or missed something in my patch.
This is my first contribution to grub so it's quite likely I missed
something you may require.
Jason
On Sat, Jun 29, 2019 at 9:44 PM Jason Kushmaul <jasonkushmaul@gmail.com>
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.
>
> Reported-by: Jason J. Kushmaul <jasonkushmaul@gmail.com>
> ---
> 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,
> 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);
> }
> }
> --
> 2.20.1
>
[-- Attachment #2: Type: text/html, Size: 15963 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cryptodisk: add luks_recover_key attempts
2019-06-30 1:44 [PATCH] cryptodisk: add luks_recover_key attempts Jason Kushmaul
2019-07-05 23:56 ` Jason Kushmaul
@ 2019-07-11 13:17 ` Daniel Kiper
2019-07-21 7:38 ` Jason Kushmaul
1 sibling, 1 reply; 9+ messages in thread
From: Daniel Kiper @ 2019-07-11 13:17 UTC (permalink / raw)
To: Jason Kushmaul; +Cc: grub-devel
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 <jasonkushmaul@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cryptodisk: add luks_recover_key attempts
2019-07-11 13:17 ` Daniel Kiper
@ 2019-07-21 7:38 ` Jason Kushmaul
2019-08-25 5:36 ` Jason Kushmaul
0 siblings, 1 reply; 9+ messages in thread
From: Jason Kushmaul @ 2019-07-21 7:38 UTC (permalink / raw)
To: grub-devel
Hi and thanks for the helpful review.
I wasn't sure about the commit message - if you had meant my email
with headers and sections outlined
with asterisks. That seemed to verbose to me but I'm not sure what
other message you would have seen.
I can update if needed. I had to grub_errno because the operation
that fails within the attempt sets
grub_errno, so I added a comment.
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" <jasonkushmaul@gmail.com>
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 <jasonkushmaul@gmail.com>
Signed-off-by: Daniel Kiper <dkiper@net-space.pl>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] cryptodisk: add luks_recover_key attempts
2019-07-21 7:38 ` Jason Kushmaul
@ 2019-08-25 5:36 ` Jason Kushmaul
2019-10-05 1:11 ` [PATCH] cryptodisk: add luks_recover_key attempts option Jason Kushmaul
0 siblings, 1 reply; 9+ messages in thread
From: Jason Kushmaul @ 2019-08-25 5:36 UTC (permalink / raw)
To: grub-devel
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" <jasonkushmaul@gmail.com>
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 <jasonkushmaul@gmail.com>
Signed-off-by: Daniel Kiper <dkiper@net-space.pl>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] cryptodisk: add luks_recover_key attempts option
2019-08-25 5:36 ` Jason Kushmaul
@ 2019-10-05 1:11 ` Jason Kushmaul
2019-10-15 13:57 ` Daniel Kiper
0 siblings, 1 reply; 9+ messages in thread
From: Jason Kushmaul @ 2019-10-05 1:11 UTC (permalink / raw)
To: grub-devel
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" <jasonkushmaul@gmail.com>
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 <jasonkushmaul@gmail.com>
Signed-off-by: Daniel Kiper <dkiper@net-space.pl>
---
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.22.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] cryptodisk: add luks_recover_key attempts option
2019-10-05 1:11 ` [PATCH] cryptodisk: add luks_recover_key attempts option Jason Kushmaul
@ 2019-10-15 13:57 ` Daniel Kiper
2019-11-02 22:23 ` Jason Kushmaul
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Kiper @ 2019-10-15 13:57 UTC (permalink / raw)
To: Jason Kushmaul; +Cc: grub-devel
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" <jasonkushmaul@gmail.com>
> 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 <jasonkushmaul@gmail.com>
> Signed-off-by: Daniel Kiper <dkiper@net-space.pl>
Both lines should be replaced with
Signed-off-by: Jason J. Kushmaul <jasonkushmaul@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cryptodisk: add luks_recover_key attempts option
2019-10-15 13:57 ` Daniel Kiper
@ 2019-11-02 22:23 ` Jason Kushmaul
2019-11-29 16:34 ` Daniel Kiper
0 siblings, 1 reply; 9+ messages in thread
From: Jason Kushmaul @ 2019-11-02 22:23 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
[-- Attachment #1: Type: text/plain, Size: 104 bytes --]
Hi Daniel,
Please see revised patch addressing all of your comments in the
attached patch file.
Jason
[-- Attachment #2: 0001-cryptodisk-add-luks_recover_key-attempts-option.patch --]
[-- Type: text/x-patch, Size: 11748 bytes --]
From 7a2b845f421d8605e139b2a7e41c2d7722c969c3 Mon Sep 17 00:00:00 2001
From: "Jason J. Kushmaul" <jasonkushmaul@gmail.com>
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 <jasonkushmaul@gmail.com>
---
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;
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);
+ }
+ 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;
+
/* 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;
+ 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);
+
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);
+
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);
+
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>
+
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);
+ if (grub_errno == GRUB_ERR_BAD_NUMBER)
+ {
+ grub_util_warn("Bad number for GRUB_CRYPTODISK_ATTEMPTS, defaulting to 1");
+ cfg->cryptodisk_attempts = 1;
+ /* there is no convenient way to report this */
+ grub_errno = GRUB_ERR_NONE;
+ }
+ }
+ continue;
+ }
if (grub_strncmp (ptr, "GRUB_DISTRIBUTOR=",
sizeof ("GRUB_DISTRIBUTOR=") - 1) == 0)
{
diff --git a/util/grub-install.c b/util/grub-install.c
index 8a55ad4b8..9e39dc4f9 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.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] cryptodisk: add luks_recover_key attempts option
2019-11-02 22:23 ` Jason Kushmaul
@ 2019-11-29 16:34 ` Daniel Kiper
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Kiper @ 2019-11-29 16:34 UTC (permalink / raw)
To: Jason Kushmaul; +Cc: grub-devel
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" <jasonkushmaul@gmail.com>
> 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 <jasonkushmaul@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-29 16:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-30 1:44 [PATCH] cryptodisk: add luks_recover_key attempts Jason Kushmaul
2019-07-05 23:56 ` Jason Kushmaul
2019-07-11 13:17 ` Daniel Kiper
2019-07-21 7:38 ` Jason Kushmaul
2019-08-25 5:36 ` Jason Kushmaul
2019-10-05 1:11 ` [PATCH] cryptodisk: add luks_recover_key attempts option Jason Kushmaul
2019-10-15 13:57 ` Daniel Kiper
2019-11-02 22:23 ` Jason Kushmaul
2019-11-29 16:34 ` Daniel Kiper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.