From c1e156ae16fa8b61af9b5d2b74e59d3f86e141be Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 8 Jan 2021 18:15:40 -0500 Subject: [PATCH] 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 Reviewed-by: Adam Langley --- crypto/dh_extra/dh_test.cc | 39 ++++++++++++++++++++++++++++ crypto/fipsmodule/dh/dh.c | 49 ++++++++++++++++++++--------------- include/openssl/base.h | 2 +- include/openssl/dh.h | 52 ++++++++++++++++++++++++++++---------- 4 files changed, 108 insertions(+), 34 deletions(-) diff --git a/crypto/dh_extra/dh_test.cc b/crypto/dh_extra/dh_test.cc index c77e7e4f1..7933a8cc5 100644 --- a/crypto/dh_extra/dh_test.cc +++ b/crypto/dh_extra/dh_test.cc @@ -71,6 +71,7 @@ #include #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 p(BN_get_rfc3526_prime_1536(nullptr)); + ASSERT_TRUE(p); + bssl::UniquePtr g(BN_new()); + ASSERT_TRUE(g); + ASSERT_TRUE(BN_set_word(g.get(), 2)); + + bssl::UniquePtr 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 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 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 05acbe23a..0d437eee4 100644 --- a/crypto/fipsmodule/dh/dh.c +++ b/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 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 @@ 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; } diff --git a/include/openssl/base.h b/include/openssl/base.h index 6ecb3d345..b6783062a 100644 --- a/include/openssl/base.h +++ b/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) diff --git a/include/openssl/dh.h b/include/openssl/dh.h index 52d831dbc..a4e2e715e 100644 --- a/include/openssl/dh.h +++ b/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 -// 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 @@ 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;