openssl: properly check return value of RAND_bytes()

This patch is in response to an off-list report by Sebastian Krahmer of the SuSE security team. Sebastian noticed we do not check the return value of RAND_bytes() correctly.

The RAND_bytes() man page first says "RAND_bytes() returns 1 on success, 0 otherwise.", but then a bit later "Both functions return -1 if they are not supported by the current RAND method.". This second case was not covered by our return value checking.

Note that if RAND_bytes() would return -1, it would *always* return -1 and fail to generate random.

Also note that if RAND_bytes() would return -1, it would do so too in the openssl internal ssl funtions. The openssl internal function do check the return value properly, and connection setup would fail all together. If that would be at least somewhat common, we would have received a *lot* of bug reports. In other words, the error affects static key setups only, and seems highly unlikely to occur in actual setups.

Only builds using OpenSSL as the crypto backend are affected.

This patch:
1. Changes the behaviour of rand_bytes() in openssl builds to match what the doxygen claims (and polarssl builds already do).
2. Adds error reporting for RAND_bytes() failures.

---
 src/openvpn/crypto_openssl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 2d81a6d..1d68662 100644
@@ -352,7 +352,12 @@ show_available_engines ()
 
 int rand_bytes(uint8_t *output, int len)
 {
-  return RAND_bytes (output, len);
+  if (unlikely(1 != RAND_bytes (output, len)))
+    {
+      crypto_msg(D_CRYPT_ERRORS, "RAND_bytes() failed");
+      return 0;
+    }
+  return 1;
 }
 
 /*

2.5.0

This patch is in response to an off-list report by Sebastian Krahmer of the SuSE security team. Sebastian noticed we do not check the return value of rand_bytes() in prng_bytes(), which we really should.

Failing to check the return value occurs if no prng is used (i.e. in static key mode, or when explicitly disabled using --prng none). prng_bytes() is used for generating IVs, session IDs and filenames.

The impact of failing to check the return value seems very limited:

Not generating random file names or session IDs could cause collisions in (temporary) file names and/or session IDs. These in turn could cause availability issues, but would not result in a breach in confidentiality and/or integrity.

Our CBC mode protocol uses a packet id (timestamp + packet counter in static key mode, or just the packet counter in TLS mode) at the start of each packet (by default, but can be disabled using --no-iv and --no-replay). Because the timestamp and packet counter are not controllable by an attacker, it is not clear how predictable or even repeating IVs could be used to mount an attack. (Note that the fact that *I* can't find or come up with an attack is not a very strong argument, this remains somewhat worrisome.)

CFB and OFB modes are not affected, because they do not rely on the prng for IVS.

Finally, RAND_bytes() actually failing is quite unlikely, as that would result in all sorts of other problems we should have heard about.

Of course, we still really should fix this, so this patch adds return value checking of rand_bytes() inside prng_bytes(). The ASSERT() might be a bit crude, so a follow-up patch that adds a return value to prng_bytes() and proper return value checking probably makes sense. But at least this is a quick and simple fix for the issue at hand.

---
 src/openvpn/crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1ceb411..c18d88b 100644
@@ -1320,7 +1320,7 @@ prng_bytes (uint8_t *output, int len)
 	}
     }
   else
-    rand_bytes (output, len);
+    ASSERT (rand_bytes (output, len));
 }
 
 /* an analogue to the random() function, but use prng_bytes */

2.5.0

Am 28.11.15 um 11:38 schrieb Steffan Karger:

ACK. Asserting on failed random seem reasonable.

Arne

Am 28.11.15 um 11:38 schrieb Steffan Karger:

ACK. Reasonable change. You cannot be too careful with crypto/RNG.

Arne

ACK2.

Your patch has been applied to the master and release/2.3 branch.

Note: crypto_msg() was changed to msg() for 2.3

commit 756602e7da11362f25be04743cd09f798b6f528a (master) commit ee714263892068171181e683aef9bc5e9dcac54a (release/2.3)

Author: Steffan Karger
Date:   Sat Nov 28 11:38:24 2015 +0100

     openssl: properly check return value of RAND_bytes()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10637
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

ACK2.

Your patch has been applied to the master and release/2.3 branch.

commit 5a73356ae5d0bf94ec81a33c7dcda6a41651ca6c (master) commit 2a15e74dd2669fd25bb0a77f4f52918713665ce6 (release/2.3)

Author: Steffan Karger
Date:   Sat Nov 28 11:38:25 2015 +0100

     Fix rand_bytes return value checking

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10636
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering