[PATCH 2/5] common: integrate crypt-based passwords
Simon Glass
sjg at chromium.org
Thu Apr 29 18:10:15 CEST 2021
Hi Steffen,
On Wed, 21 Apr 2021 at 01:55, Steffen Jaeckel
<jaeckel-floss at eyet-services.de> wrote:
>
> On 4/21/21 9:14 AM, Simon Glass wrote:
> > Hi Steffen,
> >
> > On Tue, 13 Apr 2021 at 10:16, Steffen Jaeckel
> > <jaeckel-floss at eyet-services.de> wrote:
> >>
> >> Hook into the autoboot flow as an alternative to the existing
> >> mechanisms.
> >>
> >> Signed-off-by: Steffen Jaeckel <jaeckel-floss at eyet-services.de>
> >> ---
> >>
> >> common/Kconfig.boot | 23 +++++++++++++---
> >> common/autoboot.c | 67 +++++++++++++++++++++++++++++++++++++++------
> >> 2 files changed, 77 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> >> index 9c335f4f8c..59fec48c5d 100644
> >> --- a/common/Kconfig.boot
> >> +++ b/common/Kconfig.boot
> >> @@ -802,10 +802,16 @@ config AUTOBOOT_ENCRYPTION
> >> depends on AUTOBOOT_KEYED
> >> help
> >> This option allows a string to be entered into U-Boot to stop the
> >> - autoboot. The string itself is hashed and compared against the hash
> >> - in the environment variable 'bootstopkeysha256'. If it matches then
> >> - boot stops and a command-line prompt is presented.
> >> -
> >> + autoboot.
> >> + The behavior depends whether CONFIG_CRYPT_PW is enabled or not.
> >> + In case CONFIG_CRYPT_PW is enabled, the string will be forwarded
> >> + to the crypt-based functionality and be compared against the
> >> + string in the environment variable 'bootstopkeycrypt'.
> >> + In case CONFIG_CRYPT_PW is disabled the string itself is hashed
> >> + and compared against the hash in the environment variable
> >> + 'bootstopkeysha256'.
> >> + If it matches in either case then boot stops and
> >> + a command-line prompt is presented.
> >> This provides a way to ship a secure production device which can also
> >> be accessed at the U-Boot command line.
> >>
> >> @@ -843,6 +849,15 @@ config AUTOBOOT_KEYED_CTRLC
> >> Setting this variable provides an escape sequence from the
> >> limited "password" strings.
> >>
> >> +config AUTOBOOT_STOP_STR_CRYPT
> >> + string "Stop autobooting via crypt-hashed password"
> >> + depends on AUTOBOOT_KEYED && AUTOBOOT_ENCRYPTION
> >> + help
> >> + This option adds the feature to only stop the autobooting,
> >> + and therefore boot into the U-Boot prompt, when the input
> >> + string / password matches a values that is hashed via
> >> + one of support crypt options and saved in the environment.
> >> +
> >> config AUTOBOOT_STOP_STR_SHA256
> >> string "Stop autobooting via SHA256 encrypted password"
> >> depends on AUTOBOOT_KEYED && AUTOBOOT_ENCRYPTION
> >> diff --git a/common/autoboot.c b/common/autoboot.c
> >> index 0bb08e7a4c..732a01d0e5 100644
> >> --- a/common/autoboot.c
> >> +++ b/common/autoboot.c
> >> @@ -23,6 +23,7 @@
> >> #include <linux/delay.h>
> >> #include <u-boot/sha256.h>
> >> #include <bootcount.h>
> >> +#include <crypt.h>
> >>
> >> DECLARE_GLOBAL_DATA_PTR;
> >>
> >> @@ -38,18 +39,62 @@ DECLARE_GLOBAL_DATA_PTR;
> >> static int stored_bootdelay;
> >> static int menukey;
> >>
> >> -#ifdef CONFIG_AUTOBOOT_ENCRYPTION
> >> -#define AUTOBOOT_STOP_STR_SHA256 CONFIG_AUTOBOOT_STOP_STR_SHA256
> >> -#else
> >> -#define AUTOBOOT_STOP_STR_SHA256 ""
> >> +#if defined(CONFIG_AUTOBOOT_ENCRYPTION)
> >> +#if defined(CONFIG_CRYPT_PW) && defined(CONFIG_AUTOBOOT_STOP_STR_CRYPT)
> >> +#define AUTOBOOT_STOP_STR_ENC CONFIG_AUTOBOOT_STOP_STR_CRYPT
> >> +#define HAS_STOP_STR_CRYPT 1
> >> +#elif defined(CONFIG_AUTOBOOT_STOP_STR_SHA256)
> >> +#define AUTOBOOT_STOP_STR_ENC CONFIG_AUTOBOOT_STOP_STR_SHA256
> >> +#endif
> >> +#endif
> >> +#if !defined(AUTOBOOT_STOP_STR_ENC)
> >> +#define AUTOBOOT_STOP_STR_ENC ""
> >> #endif
> >
> > I wonder if we actually need all this now that things are in Kconfig?
> > Can we use IS_ENABLED() in the code instead?
>
> The problem here is that CONFIG_AUTOBOOT_STOP_STR_{CRYPT,SHA256} are
> strings which can't be checked with IS_ENABLED().
OK...then they should be split into a CONFIG that enables the feature
and one that sets the value.
>
>
> >> -
> >> #ifdef CONFIG_USE_AUTOBOOT_MENUKEY
> >> #define AUTOBOOT_MENUKEY CONFIG_USE_AUTOBOOT_MENUKEY
> >> #else
> >> #define AUTOBOOT_MENUKEY 0
> >> #endif
> >>
> >> +static int passwd_abort_crypt(uint64_t etime)
> >
> > Please add function comment
>
> OK
>
>
> > Also unsigned long if you can...if you need 64-bit on 32-bit machines
> > it is u64...but why? That is a very large number of milliseconds!
>
> I've simply c&p'ed the prototype of the existing functions :)
> static int passwd_abort_sha256(uint64_t etime)
> static int passwd_abort_key(uint64_t etime)
OK but it still seems wrong to me so I don't think we should copy it.
Perhaps clean up the existing code?
>
>
> >> +{
> >> + const char *crypt_env_str = env_get("bootstopkeycrypt");
> >> + char presskey[MAX_DELAY_STOP_STR];
> >> + u_int presskey_len = 0;
> >
> > uint
> >
> > Can you drop the presskey_ prefix on these. There is no other kind of
> > key, so just 'key' and 'len' (or num_keys) is good enough.
>
> same as above, this is a c&p from passwd_abort_sha256()
>
> I had started by including my functionality into passwd_abort_sha256(),
> when I realized that it won't work like that I decided to c&p the entire
> implementation...
>
> should I still change both? those variable names and the uint64_t in the
> arguments?
Yes you can change the existing code in a separate patch.
>
>
> >
> >> + int abort = 0;
> >> +
> >> + if (IS_ENABLED(HAS_STOP_STR_CRYPT) && !crypt_env_str)
> >
> > We normally use IS_ENABLED with a CONFIG_....
>
> I don't see a different way than either this one or introducing a
> separate enable switch in Kconfig (which would in turn then either
> require different behavior for enabling CONFIG_AUTOBOOT_STOP_STR_SHA256
> vs CONFIG_AUTOBOOT_STOP_STR_CRYPT or we'd have to break the existing
> flow of CONFIG_AUTOBOOT_STOP_STR_SHA256).
My point was just that you should use HAS_STOP_STR_CRYPT , not
IS_ENABLED(HAS_STOP_STR_CRYPT)
[...]
Regards,
Simon
More information about the U-Boot
mailing list