crypto: drbg - Include get_random_bytes() output in additional input

Woodage & Shumow (2018) (https://eprint.iacr.org/2018/349.pdf) showed
that contrary to the claims made by NIST in SP800-90A, HMAC_DRBG doesn't
satisfy the formal definition of forward secrecy (i.e. "backtracking
resistance") when it's called with an empty additional input string.

The actual attack seems pretty benign, as it doesn't actually give the
attacker any previous RNG output, but rather just allows them to test
whether their guess of the previous block of RNG output is correct.
Regardless, it's an annoying design flaw, and it's yet another example
of why NIST's DRBGs aren't all that great.

Meanwhile, the kernel's HMAC_DRBG code also tries to reseed itself
automatically after random.c has reseeded itself.  But the
implementation is buggy, as it just checks whether 300 seconds have
elapsed, rather than looking at the actual generation counter.

Let's just follow the example of BoringSSL and use the conservative
approach of always including 32 bytes of "regular" random data in the
additional input string.  This fixes both issues described above.

This does reduce performance.  But this should be tolerable, since:

  - Due to earlier changes, the kernel code that was previously using
    drbg.c regardless of FIPS mode is now using it only in FIPS mode.

  - The additional input string is processed only once per request.  So
    if a lot of bytes are generated at once, the cost is amortized.

  - The NIST DRBGs are notoriously slow anyway.

Note that this fix should have no impact (either positive or negative)
on FIPS 140 certifiability.  From FIPS's point of view the code added by
this commit simply doesn't matter: it adds zero entropy to something
that doesn't need to contain entropy.

Fixes: 541af946fe13 ("crypto: drbg - SP800-90A Deterministic Random Bit Generator")
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/drbg.c b/crypto/drbg.c
index b2af481aef010cb2b77bfcbdb944f2b83832ec73..cda79d601f4f43ec8e2b5583b6421f73acf108f1 100644 (file)
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -193,13 +193,32 @@ static void drbg_hmac_update(struct drbg_state *drbg,
 static void drbg_hmac_generate(struct drbg_state *drbg,
                               unsigned char *buf,
                               unsigned int buflen,
-                              const u8 *addtl, size_t addtl_len)
+                              const u8 *addtl1, size_t addtl1_len)
 {
        int len = 0;
+       u8 addtl2[32];
+       size_t addtl2_len = 0;
+
+       /*
+        * Append some bytes from get_random_bytes() to the additional input
+        * string, except when in test mode (as it would break the tests).
+        * Using a nonempty additional input string works around the forward
+        * secrecy bug in HMAC_DRBG described by Woodage & Shumow (2018)
+        * (https://eprint.iacr.org/2018/349.pdf).  Filling the string with
+        * get_random_bytes() rather than a fixed value is safer still, and in
+        * particular makes random.c reseeds be immediately reflected.
+        *
+        * Note that there's no need to pull bytes from jitterentropy here too,
+        * since FIPS doesn't require any entropy in the additional input.
+        */
+       if (drbg->test_entropylen == 0) {
+               get_random_bytes(addtl2, sizeof(addtl2));
+               addtl2_len = sizeof(addtl2);
+       }
 
        /* 10.1.2.5 step 2 */
-       if (addtl_len)
-               drbg_hmac_update(drbg, addtl, addtl_len, NULL, 0);
+       if (addtl1_len || addtl2_len)
+               drbg_hmac_update(drbg, addtl1, addtl1_len, addtl2, addtl2_len);
 
        while (len < buflen) {
                unsigned int outlen = 0;
@@ -215,7 +234,9 @@ static void drbg_hmac_generate(struct drbg_state *drbg,
        }
 
        /* 10.1.2.5 step 6 */
-       drbg_hmac_update(drbg, addtl, addtl_len, NULL, 0);
+       drbg_hmac_update(drbg, addtl1, addtl1_len, addtl2, addtl2_len);
+
+       memzero_explicit(addtl2, sizeof(addtl2));
 }
 
 static inline void __drbg_seed(struct drbg_state *drbg,