[U-Boot] [PATCH v1 4/4] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password

[Magnus Lilja]

Hi Stefan

On 7 May 2015 at 14:13, Stefan Roese <sr at denx.de> wrote:
> This patch adds the feature to only stop the autobooting, and therefor
> boot into the U-Boot prompt, when the input string / password matches
> a values that is encypted via a SHA256 hash and saved in the environment.
>
> This feature is enabled by defined these config options:
>      CONFIG_AUTOBOOT_KEYED
>      CONFIG_AUTOBOOT_STOP_STR_SHA256
>
> +       /*
> +        * Generate the binary value from the environment hash value
> +        * so that we can compare this value with the computed hash
> +        * from the user input
> +        */
> +       for (i = 0; i < SHA256_SUM_LEN; i++) {
> +               char chr[3];
> +
> +               strncpy(chr, &sha_env_str[i * 2], 2);
> +               sha_env[i] = simple_strtoul(chr, NULL, 16);
> +       }
> +
> +       /*
> +        * We don't know how long the stop-string is, so we need to
> +        * generate the sha256 hash upon each input character and
> +        * compare the value with the one saved in the environment
> +        */
> +       do {
> +               if (tstc()) {
> +                       presskey[presskey_len++] = getc();
> +
> +                       /* Calculate sha256 upon each new char */
> +                       sha256_csum_wd((unsigned char *)presskey, presskey_len,
> +                                      sha, CHUNKSZ_SHA256);
> +
> +                       /* And check if sha matches saved value in env */
> +                       if (memcmp(sha, sha_env, SHA256_SUM_LEN) == 0)
> +                               abort = 1;
> +               }
> +       } while (!abort && get_ticks() <= etime);

I don't know what the security requirements are for this feature, i.e.
what strength the mechanism should have but:

1. Simply hashing the password is not recommended, a long salt
(generated by a good random number generator) should be pre-pended to
the passphrase before hashing. See [1]

2. Using memcmp() is not recommended for the above comparison. See [1]
(SlowEqual example).


3. I haven't looked closely at the code above but it looks to me that
there is no check that the stop-string entered by the user/attacker
fits the presskey buffer. I.e. a buffer overflow attack might be
possible.


[1] https://crackstation.net/hashing-security.htm

Regards, Magnus