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>
diff --git a/crypto/dh_extra/dh_test.cc b/crypto/dh_extra/dh_test.cc
index c77e7e4..7933a8c 100644
--- a/crypto/dh_extra/dh_test.cc
+++ b/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 @@
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));
+}
diff --git a/crypto/fipsmodule/dh/dh.c b/crypto/fipsmodule/dh/dh.c
index 05acbe2..0d437ee 100644
--- a/crypto/fipsmodule/dh/dh.c
+++ b/crypto/fipsmodule/dh/dh.c
@@ -321,6 +321,27 @@
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 @@
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 p_len = BN_num_bytes(dh->p);
- uint8_t *shared_bytes = OPENSSL_malloc(p_len);
+ const size_t dh_len = DH_size(dh);
+ uint8_t *shared_bytes = OPENSSL_malloc(dh_len);
unsigned out_len_unsigned;
- if (!shared_key ||
- !shared_bytes ||
- !dh_compute_key(dh, shared_key, peers_key, ctx) ||
- // |DH_compute_key| doesn't pad the output. SP 800-56A is ambiguous about
- // whether the output should be padded prior to revision three. But
- // revision three, section C.1, awkwardly specifies padding to the length
- // of p.
+ if (!shared_bytes ||
+ // SP 800-56A is ambiguous about 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) ||
- !EVP_Digest(shared_bytes, p_len, out, &out_len_unsigned, digest, NULL) ||
+ DH_compute_key_padded(shared_bytes, peers_key, dh) != (int)dh_len ||
+ !EVP_Digest(shared_bytes, dh_len, out, &out_len_unsigned, digest, NULL) ||
out_len_unsigned != digest_len) {
goto err;
}
@@ -380,8 +391,6 @@
ret = 1;
err:
- BN_CTX_end(ctx);
- BN_CTX_free(ctx);
OPENSSL_free(shared_bytes);
return ret;
}
diff --git a/include/openssl/base.h b/include/openssl/base.h
index 6ecb3d3..b678306 100644
--- a/include/openssl/base.h
+++ b/include/openssl/base.h
@@ -191,7 +191,7 @@
// 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)
diff --git a/include/openssl/dh.h b/include/openssl/dh.h
index 52d831d..a4e2e71 100644
--- a/include/openssl/dh.h
+++ b/include/openssl/dh.h
@@ -163,20 +163,24 @@
// |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
-// writes it as a big-endian integer into |out|, which must have |DH_size|
-// bytes of space. It returns the number of bytes written, or a negative number
-// on error.
+// DH_compute_key_padded calculates the shared key between |dh| and |peers_key|
+// and writes it as a big-endian integer into |out|, padded up to |DH_size|
+// bytes. It returns the number of bytes written, which is always |DH_size|, or
+// 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,
-// 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/.
+// WARNING: this differs from the usual BoringSSL return-value convention.
//
-// This is a legacy algorithm, so we do not provide a fixed-width variant. Use
-// X25519 or ECDH with P-256 instead.
-OPENSSL_EXPORT int DH_compute_key(uint8_t *out, const BIGNUM *peers_key,
- DH *dh);
+// Note this function differs from |DH_compute_key| in that it preserves leading
+// zeros in the secret. This function is the preferred variant. It matches PKCS
+// #3 and avoids some side channel 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 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 @@
// 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 @@
// 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;
