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

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