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

29 Apr 2021

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

    