[2/5] common: integrate crypt-based passwords

Hook into the autoboot flow as an alternative to the existing
mechanisms.

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
---

 common/Kconfig.boot | 23 +++++++++++++---
 common/autoboot.c   | 67 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 77 insertions(+), 13 deletions(-)

Hi Steffen,

On Tue, 13 Apr 2021 at 10:16, Steffen Jaeckel
<jaeckel-floss@eyet-services.de> wrote:
>
> Hook into the autoboot flow as an alternative to the existing
> mechanisms.
>
> Signed-off-by: Steffen Jaeckel <jaeckel-floss@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?

> -
>  #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

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!

> +{
> +       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.

> +       int abort = 0;
> +
> +       if (IS_ENABLED(HAS_STOP_STR_CRYPT) && !crypt_env_str)

We normally use IS_ENABLED with a CONFIG_....

> +               crypt_env_str = AUTOBOOT_STOP_STR_ENC;
> +
> +       if (!crypt_env_str)
> +               return 0;
> +
> +       /*
> +        * We expect the stop-string to be newline terminated.
> +        */

/* ... */ is the single-line comment format

> +       do {
> +               if (tstc()) {
> +                       /* Check for input string overflow */
> +                       if (presskey_len >= MAX_DELAY_STOP_STR)
> +                               return 0;
> +
> +                       presskey[presskey_len] = getchar();
> +
> +                       if ((presskey[presskey_len] == '\r') ||
> +                           (presskey[presskey_len] == '\n')) {
> +                               presskey[presskey_len] = '\0';
> +                               crypt_compare(crypt_env_str, presskey, &abort);
> +                               /* you had one chance */
> +                               break;
> +                       } else {
> +                               presskey_len++;
> +                       }
> +               }
> +       } while (get_ticks() <= etime);
> +
> +       return abort;
> +}
> +
>  /*
>   * Use a "constant-length" time compare function for this
>   * hash compare:
> @@ -89,7 +134,7 @@ static int passwd_abort_sha256(uint64_t etime)
>         int ret;
>
>         if (sha_env_str == NULL)
> -               sha_env_str = AUTOBOOT_STOP_STR_SHA256;
> +               sha_env_str = AUTOBOOT_STOP_STR_ENC;
>
>         presskey = malloc_cache_aligned(MAX_DELAY_STOP_STR);
>         c = strstr(sha_env_str, ":");
> @@ -245,10 +290,14 @@ static int abortboot_key_sequence(int bootdelay)
>         printf(CONFIG_AUTOBOOT_PROMPT, bootdelay);
>  #  endif
>
> -       if (IS_ENABLED(CONFIG_AUTOBOOT_ENCRYPTION))
> -               abort = passwd_abort_sha256(etime);
> -       else
> +       if (IS_ENABLED(CONFIG_AUTOBOOT_ENCRYPTION)) {
> +               if (IS_ENABLED(CONFIG_CRYPT_PW))
> +                       abort = passwd_abort_crypt(etime);
> +               else
> +                       abort = passwd_abort_sha256(etime);
> +       } else {
>                 abort = passwd_abort_key(etime);
> +       }
>         if (!abort)
>                 debug_bootkeys("key timeout\n");
>
> --
> 2.30.1
>

Regards,
SImon

On 4/21/21 9:14 AM, Simon Glass wrote:
> Hi Steffen,
> 
> On Tue, 13 Apr 2021 at 10:16, Steffen Jaeckel
> <jaeckel-floss@eyet-services.de> wrote:
>>
>> Hook into the autoboot flow as an alternative to the existing
>> mechanisms.
>>
>> Signed-off-by: Steffen Jaeckel <jaeckel-floss@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().


>> -
>>  #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)


>> +{
>> +       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?


> 
>> +       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).

> 
>> +               crypt_env_str = AUTOBOOT_STOP_STR_ENC;
>> +
>> +       if (!crypt_env_str)
>> +               return 0;
>> +
>> +       /*
>> +        * We expect the stop-string to be newline terminated.
>> +        */
> 
> /* ... */ is the single-line comment format

OK

> 
>> +       do {
>> +               if (tstc()) {
>> +                       /* Check for input string overflow */
>> +                       if (presskey_len >= MAX_DELAY_STOP_STR)
>> +                               return 0;
>> +
>> +                       presskey[presskey_len] = getchar();
>> +
>> +                       if ((presskey[presskey_len] == '\r') ||
>> +                           (presskey[presskey_len] == '\n')) {
>> +                               presskey[presskey_len] = '\0';
>> +                               crypt_compare(crypt_env_str, presskey, &abort);
>> +                               /* you had one chance */
>> +                               break;
>> +                       } else {
>> +                               presskey_len++;
>> +                       }
>> +               }
>> +       } while (get_ticks() <= etime);
>> +
>> +       return abort;
>> +}
>> +
>>  /*
>>   * Use a "constant-length" time compare function for this
>>   * hash compare:
>> @@ -89,7 +134,7 @@ static int passwd_abort_sha256(uint64_t etime)
>>         int ret;
>>
>>         if (sha_env_str == NULL)
>> -               sha_env_str = AUTOBOOT_STOP_STR_SHA256;
>> +               sha_env_str = AUTOBOOT_STOP_STR_ENC;
>>
>>         presskey = malloc_cache_aligned(MAX_DELAY_STOP_STR);
>>         c = strstr(sha_env_str, ":");
>> @@ -245,10 +290,14 @@ static int abortboot_key_sequence(int bootdelay)
>>         printf(CONFIG_AUTOBOOT_PROMPT, bootdelay);
>>  #  endif
>>
>> -       if (IS_ENABLED(CONFIG_AUTOBOOT_ENCRYPTION))
>> -               abort = passwd_abort_sha256(etime);
>> -       else
>> +       if (IS_ENABLED(CONFIG_AUTOBOOT_ENCRYPTION)) {
>> +               if (IS_ENABLED(CONFIG_CRYPT_PW))
>> +                       abort = passwd_abort_crypt(etime);
>> +               else
>> +                       abort = passwd_abort_sha256(etime);
>> +       } else {
>>                 abort = passwd_abort_key(etime);
>> +       }
>>         if (!abort)
>>                 debug_bootkeys("key timeout\n");
>>
>> --
>> 2.30.1
>>
> 
> Regards,
> SImon
>

Hi Steffen,

On Wed, 21 Apr 2021 at 01:55, Steffen Jaeckel
<jaeckel-floss@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@eyet-services.de> wrote:
> >>
> >> Hook into the autoboot flow as an alternative to the existing
> >> mechanisms.
> >>
> >> Signed-off-by: Steffen Jaeckel <jaeckel-floss@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