Add DH_compute_key_padded.

OpenSSL has a fixed-width version of DH_compute_key nowadays. Searching
around callers of DH_compute_key, many of them go back and re-pad the
secret anyway. Uses of DH should migrate to modern primitives but, in
the meantime, DH_compute_key_padded seems worthwhile for OpenSSL
compatibility and giving fixed-width users a function to avoid the
timing leak.

Bump BORINGSSL_API_VERSION since one of the uses is in wpa_supplicant
and they like to compile against a wide range of Android revisions.

Update-Note: No compatibility impact, but callers that use
DH_compute_key and then fix up the removed leading zeros can switch to
this function. Then they should migrate to something else.

Change-Id: Icf8b2ace3972fa174a0f08ece39710f7599f96f2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45004
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/dh_extra/dh_test.cc

@@ -71,6 +71,7 @@
 #include <openssl/mem.h>
 
 #include "../internal.h"
+#include "../test/test_util.h"
 
 
 static bool RunBasicTests();
@@ -443,3 +444,41 @@ static bool TestRFC3526() {
 
   return true;
 }
+
+TEST(DHTest, LeadingZeros) {
+  bssl::UniquePtr<BIGNUM> p(BN_get_rfc3526_prime_1536(nullptr));
+  ASSERT_TRUE(p);
+  bssl::UniquePtr<BIGNUM> g(BN_new());
+  ASSERT_TRUE(g);
+  ASSERT_TRUE(BN_set_word(g.get(), 2));
+
+  bssl::UniquePtr<DH> dh(DH_new());
+  ASSERT_TRUE(dh);
+  ASSERT_TRUE(DH_set0_pqg(dh.get(), p.get(), /*q=*/nullptr, g.get()));
+  p.release();
+  g.release();
+
+  // These values are far too small to be reasonable Diffie-Hellman keys, but
+  // they are an easy way to get a shared secret with leading zeros.
+  bssl::UniquePtr<BIGNUM> priv_key(BN_new()), peer_key(BN_new());
+  ASSERT_TRUE(priv_key);
+  ASSERT_TRUE(BN_set_word(priv_key.get(), 2));
+  ASSERT_TRUE(peer_key);
+  ASSERT_TRUE(BN_set_word(peer_key.get(), 3));
+  ASSERT_TRUE(DH_set0_key(dh.get(), /*pub_key=*/nullptr, priv_key.get()));
+  priv_key.release();
+
+  uint8_t padded[192] = {0};
+  padded[191] = 9;
+  static const uint8_t kTruncated[] = {9};
+  EXPECT_EQ(int(sizeof(padded)), DH_size(dh.get()));
+
+  std::vector<uint8_t> buf(DH_size(dh.get()));
+  int len = DH_compute_key(buf.data(), peer_key.get(), dh.get());
+  ASSERT_GT(len, 0);
+  EXPECT_EQ(Bytes(buf.data(), len), Bytes(kTruncated));
+
+  len = DH_compute_key_padded(buf.data(), peer_key.get(), dh.get());
+  ASSERT_GT(len, 0);
+  EXPECT_EQ(Bytes(buf.data(), len), Bytes(padded));
+}

crypto/fipsmodule/dh/dh.c

@@ -321,6 +321,27 @@ static int dh_compute_key(DH *dh, BIGNUM *out_shared_key,
   return ret;
 }
 
+int DH_compute_key_padded(unsigned char *out, const BIGNUM *peers_key, DH *dh) {
+  BN_CTX *ctx = BN_CTX_new();
+  if (ctx == NULL) {
+    return -1;
+  }
+  BN_CTX_start(ctx);
+
+  int dh_size = DH_size(dh);
+  int ret = -1;
+  BIGNUM *shared_key = BN_CTX_get(ctx);
+  if (shared_key &&
+      dh_compute_key(dh, shared_key, peers_key, ctx) &&
+      BN_bn2bin_padded(out, dh_size, shared_key)) {
+    ret = dh_size;
+  }
+
+  BN_CTX_end(ctx);
+  BN_CTX_free(ctx);
+  return ret;
+}
+
 int DH_compute_key(unsigned char *out, const BIGNUM *peers_key, DH *dh) {
   BN_CTX *ctx = BN_CTX_new();
   if (ctx == NULL) {
@@ -349,29 +370,19 @@ int DH_compute_key_hashed(DH *dh, uint8_t *out, size_t *out_len,
     return 0;
   }
 
-  BN_CTX *ctx = BN_CTX_new();
-  if (ctx == NULL) {
-    return 0;
-  }
-  BN_CTX_start(ctx);
-
   int ret = 0;
-  BIGNUM *shared_key = BN_CTX_get(ctx);
+  const size_t dh_len = DH_size(dh);
-  const size_t p_len = BN_num_bytes(dh->p);
+  uint8_t *shared_bytes = OPENSSL_malloc(dh_len);
-  uint8_t *shared_bytes = OPENSSL_malloc(p_len);
   unsigned out_len_unsigned;
-  if (!shared_key ||
+  if (!shared_bytes ||
-      !shared_bytes ||
+      // SP 800-56A is ambiguous about whether the output should be padded prior
-      !dh_compute_key(dh, shared_key, peers_key, ctx) ||
+      // to revision three. But revision three, section C.1, awkwardly specifies
-      // |DH_compute_key| doesn't pad the output. SP 800-56A is ambiguous about
+      // padding to the length of p.
-      // whether the output should be padded prior to revision three. But
-      // revision three, section C.1, awkwardly specifies padding to the length
-      // of p.
       //
       // Also, padded output avoids side-channels, so is always strongly
       // advisable.
-      !BN_bn2bin_padded(shared_bytes, p_len, shared_key) ||
+      DH_compute_key_padded(shared_bytes, peers_key, dh) != (int)dh_len ||
-      !EVP_Digest(shared_bytes, p_len, out, &out_len_unsigned, digest, NULL) ||
+      !EVP_Digest(shared_bytes, dh_len, out, &out_len_unsigned, digest, NULL) ||
       out_len_unsigned != digest_len) {
     goto err;
   }
@@ -380,8 +391,6 @@ int DH_compute_key_hashed(DH *dh, uint8_t *out, size_t *out_len,
   ret = 1;
 
  err:
-  BN_CTX_end(ctx);
-  BN_CTX_free(ctx);
   OPENSSL_free(shared_bytes);
   return ret;
 }

include/openssl/base.h

@@ -191,7 +191,7 @@ extern "C" {
 // A consumer may use this symbol in the preprocessor to temporarily build
 // against multiple revisions of BoringSSL at the same time. It is not
 // recommended to do so for longer than is necessary.
-#define BORINGSSL_API_VERSION 13
+#define BORINGSSL_API_VERSION 14
 
 #if defined(BORINGSSL_SHARED_LIBRARY)
 

include/openssl/dh.h

@@ -163,20 +163,24 @@ OPENSSL_EXPORT int DH_generate_parameters_ex(DH *dh, int prime_bits,
 // |dh|. It returns one on success and zero on error.
 OPENSSL_EXPORT int DH_generate_key(DH *dh);
 
-// DH_compute_key calculates the shared key between |dh| and |peers_key| and
+// DH_compute_key_padded calculates the shared key between |dh| and |peers_key|
-// writes it as a big-endian integer into |out|, which must have |DH_size|
+// and writes it as a big-endian integer into |out|, padded up to |DH_size|
-// bytes of space. It returns the number of bytes written, or a negative number
+// bytes. It returns the number of bytes written, which is always |DH_size|, or
-// on error.
+// a negative number on error. |out| must have |DH_size| bytes of space.
 //
-// Note the output may be shorter than |DH_size| bytes. Contrary to PKCS #3,
+// WARNING: this differs from the usual BoringSSL return-value convention.
-// this function returns a variable-length shared key with leading zeros
-// removed. This may result in sporadic key mismatch and, if |dh| is reused,
-// side channel attacks such as https://raccoon-attack.com/.
 //
-// This is a legacy algorithm, so we do not provide a fixed-width variant. Use
+// Note this function differs from |DH_compute_key| in that it preserves leading
-// X25519 or ECDH with P-256 instead.
+// zeros in the secret. This function is the preferred variant. It matches PKCS
-OPENSSL_EXPORT int DH_compute_key(uint8_t *out, const BIGNUM *peers_key,
+// #3 and avoids some side channel attacks. However, the two functions are not
-                                  DH *dh);
+// drop-in replacements for each other. Using a different variant than the
+// application expects will result in sporadic key mismatches.
+//
+// Callers that expect a fixed-width secret should use this function over
+// |DH_compute_key|. Callers that use either function should migrate to a modern
+// primitive such as X25519 or ECDH with P-256 instead.
+OPENSSL_EXPORT int DH_compute_key_padded(uint8_t *out, const BIGNUM *peers_key,
+                                         DH *dh);
 
 // DH_compute_key_hashed calculates the shared key between |dh| and |peers_key|
 // and hashes it with the given |digest|. If the hash output is less than
@@ -185,7 +189,7 @@ OPENSSL_EXPORT int DH_compute_key(uint8_t *out, const BIGNUM *peers_key,
 // returns one on success or zero on error.
 //
 // NOTE: this follows the usual BoringSSL return-value convention, but that's
-// different from |DH_compute_key|, above.
+// different from |DH_compute_key| and |DH_compute_key_padded|.
 OPENSSL_EXPORT int DH_compute_key_hashed(DH *dh, uint8_t *out, size_t *out_len,
                                          size_t max_out_len,
                                          const BIGNUM *peers_key,
@@ -278,6 +282,28 @@ OPENSSL_EXPORT DH *d2i_DHparams(DH **ret, const unsigned char **inp, long len);
 // Use |DH_marshal_parameters| instead.
 OPENSSL_EXPORT int i2d_DHparams(const DH *in, unsigned char **outp);
 
+// DH_compute_key behaves like |DH_compute_key_padded| but, contrary to PKCS #3,
+// returns a variable-length shared key with leading zeros. It returns the
+// number of bytes written, or a negative number on error. |out| must have
+// |DH_size| bytes of space.
+//
+// WARNING: this differs from the usual BoringSSL return-value convention.
+//
+// Note this function's running time and memory access pattern leaks information
+// about the shared secret. Particularly if |dh| is reused, this may result in
+// side channel attacks such as https://raccoon-attack.com/.
+//
+// |DH_compute_key_padded| is the preferred variant and avoids the above
+// attacks. However, the two functions are not drop-in replacements for each
+// other. Using a different variant than the application expects will result in
+// sporadic key mismatches.
+//
+// Callers that expect a fixed-width secret should use |DH_compute_key_padded|
+// instead. Callers that use either function should migrate to a modern
+// primitive such as X25519 or ECDH with P-256 instead.
+OPENSSL_EXPORT int DH_compute_key(uint8_t *out, const BIGNUM *peers_key,
+                                  DH *dh);
+
 
 struct dh_st {
   BIGNUM *p;