[PATCH 2/5] common: integrate crypt-based passwords
Steffen Jaeckel
jaeckel-floss at eyet-services.de
Wed Apr 21 10:55:27 CEST 2021
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().
>> -
>> #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
>
More information about the U-Boot
mailing list