diff --git a/crypto/chacha/chacha.c b/crypto/chacha/chacha.c index 1092b7aa28..a4d88c0a40 100644 --- a/crypto/chacha/chacha.c +++ b/crypto/chacha/chacha.c @@ -91,7 +91,25 @@ void CRYPTO_chacha_20(uint8_t *out, const uint8_t *in, size_t in_len, } #endif - ChaCha20_ctr32(out, in, in_len, key_ptr, counter_nonce); + while (in_len > 0) { + // The assembly functions do not have defined overflow behavior. While + // overflow is almost always a bug in the caller, we prefer our functions to + // behave the same across platforms, so divide into multiple calls to avoid + // this case. + uint64_t todo = 64 * ((UINT64_C(1) << 32) - counter_nonce[0]); + if (todo > in_len) { + todo = in_len; + } + + ChaCha20_ctr32(out, in, (size_t)todo, key_ptr, counter_nonce); + in += todo; + out += todo; + in_len -= todo; + + // We're either done and will next break out of the loop, or we stopped at + // the wraparound point and the counter should continue at zero. + counter_nonce[0] = 0; + } } #else diff --git a/crypto/chacha/chacha_test.cc b/crypto/chacha/chacha_test.cc index 25313cad6d..00683ce51e 100644 --- a/crypto/chacha/chacha_test.cc +++ b/crypto/chacha/chacha_test.cc @@ -218,8 +218,102 @@ static const uint8_t kOutput[] = { 0x8f, 0x40, 0xcf, 0x4a, }; +static uint32_t kOverflowCounter = 0xffffffff; + +static const uint8_t kOverflowOutput[] = { + 0x37, 0x64, 0x38, 0xcb, 0x25, 0x69, 0x2c, 0xf5, 0x88, 0x8a, 0xfe, 0x6d, + 0x3b, 0x10, 0x07, 0x3c, 0x77, 0xac, 0xcd, 0x1c, 0x0c, 0xa7, 0x17, 0x31, + 0x1d, 0xc3, 0x81, 0xd1, 0xa5, 0x20, 0x55, 0xea, 0xd3, 0x00, 0xc9, 0x84, + 0xde, 0xe2, 0xe5, 0x5e, 0x7b, 0x28, 0x28, 0x59, 0x73, 0x3a, 0x8e, 0x57, + 0x62, 0x18, 0x50, 0x55, 0x97, 0xca, 0x50, 0x3e, 0x8a, 0x84, 0x61, 0x28, + 0x4c, 0x22, 0x93, 0x50, 0x48, 0x7e, 0x65, 0x78, 0x06, 0x5a, 0xcd, 0x2b, + 0x11, 0xf7, 0x10, 0xfd, 0x6f, 0x41, 0x92, 0x82, 0x7c, 0x3a, 0x71, 0x07, + 0x67, 0xd0, 0x7e, 0xb7, 0xdf, 0xdc, 0xfc, 0xee, 0xe6, 0x55, 0xdd, 0x6f, + 0x79, 0x23, 0xf3, 0xae, 0xb1, 0x21, 0x96, 0xbe, 0xea, 0x0e, 0x1b, 0x58, + 0x0b, 0x3f, 0x63, 0x51, 0xd4, 0xce, 0x98, 0xfe, 0x1a, 0xc7, 0xa7, 0x43, + 0x7f, 0x0c, 0xe8, 0x62, 0xcf, 0x78, 0x3f, 0x4e, 0x31, 0xbf, 0x2b, 0x76, + 0x91, 0xcd, 0x19, 0x80, 0x0d, 0x7f, 0x11, 0x8b, 0x76, 0xef, 0x43, 0x3c, + 0x4f, 0x61, 0x86, 0xc5, 0x64, 0xa8, 0xc2, 0x73, 0xc2, 0x64, 0x39, 0xa0, + 0x8b, 0xe6, 0x7f, 0xf6, 0x26, 0xd4, 0x47, 0x4f, 0xe4, 0x46, 0xe2, 0xf5, + 0x9e, 0xe6, 0xc7, 0x76, 0x6c, 0xa9, 0x0f, 0x1d, 0x1b, 0x22, 0xa5, 0x62, + 0x0a, 0x88, 0x3e, 0x8c, 0xf0, 0xbc, 0x4c, 0x11, 0x3f, 0x0d, 0xf7, 0x85, + 0x67, 0x0b, 0x4c, 0xa3, 0x3f, 0xa8, 0xf1, 0x2a, 0x65, 0x2e, 0x00, 0x03, + 0xc9, 0x49, 0x91, 0x48, 0xb7, 0xc8, 0x29, 0x28, 0x2f, 0x46, 0x8e, 0x8b, + 0xd6, 0x73, 0x19, 0x06, 0x3e, 0x6f, 0x92, 0xc8, 0x3d, 0x3f, 0x4d, 0x68, + 0xbc, 0x02, 0xc0, 0x8f, 0x71, 0x46, 0x0d, 0x28, 0x63, 0xfe, 0xad, 0x14, + 0x81, 0x04, 0xb7, 0x23, 0xfd, 0x21, 0x0a, 0xf0, 0x6f, 0xcd, 0x47, 0x0b, + 0x0e, 0x93, 0xa3, 0xa8, 0x44, 0x15, 0xd6, 0xae, 0x06, 0x44, 0x6b, 0xbc, + 0xff, 0x8a, 0x56, 0x60, 0x3c, 0x38, 0xd6, 0xed, 0x03, 0x2d, 0x79, 0x2a, + 0xe9, 0x15, 0xef, 0xfc, 0x92, 0x1f, 0x83, 0xa4, 0x60, 0x8f, 0xc9, 0x29, + 0xb2, 0xb4, 0x9e, 0x3f, 0xa9, 0xe8, 0xfb, 0xa2, 0x62, 0x20, 0x2e, 0xc9, + 0x43, 0xb2, 0xd1, 0x36, 0x85, 0x1e, 0xa4, 0xb3, 0x4f, 0x8c, 0x9e, 0x81, + 0x75, 0x68, 0xbc, 0xf1, 0x52, 0xd5, 0x03, 0x22, 0xcf, 0xdf, 0x64, 0xb0, + 0x28, 0xd2, 0x45, 0x18, 0x38, 0x8c, 0xd0, 0xf6, 0x30, 0x3c, 0x04, 0xd9, + 0x8d, 0xb6, 0xb2, 0x57, 0x2a, 0xee, 0x28, 0xeb, 0x5f, 0x1a, 0x10, 0x6e, + 0x88, 0x79, 0x08, 0x23, 0x19, 0x84, 0xf8, 0x80, 0x1a, 0x7d, 0x6f, 0x8b, + 0xc1, 0x8e, 0x5f, 0x5f, 0x54, 0x14, 0x2a, 0xdc, 0x41, 0x5d, 0xeb, 0x00, + 0xf2, 0x50, 0xae, 0xd3, 0x55, 0x32, 0xf6, 0xd9, 0x34, 0xf4, 0xb2, 0xf2, + 0xf5, 0x90, 0x05, 0x8a, 0x9c, 0xc7, 0x94, 0x5d, 0x2d, 0x5a, 0x0f, 0xdd, + 0x03, 0xde, 0xbe, 0x18, 0xb3, 0xe3, 0x07, 0x6b, 0x57, 0xfa, 0x1b, 0x7b, + 0x75, 0xcb, 0xc2, 0x4d, 0xf7, 0x88, 0xfe, 0xf9, 0xc0, 0x6c, 0xdb, 0x5f, + 0xf6, 0x48, 0x00, 0x4a, 0x5d, 0x75, 0xfa, 0x6b, 0x45, 0x43, 0xc4, 0x7f, + 0x97, 0x31, 0x22, 0xb4, 0x9c, 0xa3, 0xee, 0x2f, 0x27, 0xa9, 0x9f, 0x0e, + 0xdc, 0x40, 0x67, 0x17, 0x2e, 0xcb, 0xfd, 0x9e, 0xe7, 0xb2, 0x85, 0xcd, + 0x49, 0x24, 0xc8, 0x8a, 0x59, 0x6b, 0x1f, 0xec, 0x72, 0x89, 0xf8, 0x30, + 0xdf, 0x82, 0x61, 0x3b, 0x8b, 0xc9, 0x80, 0xe4, 0x27, 0x0d, 0xfe, 0x42, + 0x27, 0x6c, 0xaf, 0x62, 0x3e, 0x2f, 0x1d, 0x38, 0xb6, 0x88, 0x8f, 0x71, + 0x5a, 0x54, 0x6c, 0x68, 0x57, 0x40, 0x49, 0x7a, 0xb2, 0xe8, 0xb6, 0x97, + 0xab, 0xd6, 0x3c, 0x35, 0xf3, 0x95, 0x12, 0xde, 0xa2, 0x39, 0x54, 0x52, + 0x8c, 0x38, 0x2a, 0x2b, 0xe7, 0x21, 0x38, 0x63, 0xb0, 0xd6, 0xad, 0x94, + 0x44, 0xaf, 0x49, 0x5d, 0xfc, 0x49, 0x6b, 0x30, 0xdf, 0xe9, 0x19, 0x1e, + 0xed, 0x98, 0x0d, 0x4a, 0x3d, 0x56, 0x5e, 0x74, 0xad, 0x13, 0x8b, 0x68, + 0x45, 0x08, 0xbe, 0x0e, 0x6c, 0xb4, 0x62, 0x93, 0x27, 0x8b, 0x4f, 0xab, + 0x3e, 0xba, 0xe1, 0xe5, 0xff, 0xa8, 0x5d, 0x33, 0x32, 0xff, 0x34, 0xf9, + 0x8d, 0x67, 0x24, 0x4a, 0xbb, 0x2c, 0x60, 0xb5, 0x88, 0x96, 0x1b, 0xcc, + 0x53, 0xfb, 0x2e, 0x05, 0x1d, 0x8b, 0xc2, 0xa0, 0xde, 0x21, 0x41, 0x5e, + 0x11, 0x1b, 0x96, 0xd9, 0xa6, 0xae, 0xbd, 0xf0, 0x91, 0xad, 0x69, 0x2b, + 0xd2, 0x3f, 0xe4, 0x3d, 0x16, 0x69, 0xa6, 0xb2, 0x9c, 0xbe, 0x59, 0x7b, + 0x87, 0x79, 0xf5, 0xc2, 0x5a, 0xcc, 0xdf, 0xfe, 0x7f, 0xf9, 0xa6, 0x52, + 0xde, 0x5f, 0x46, 0x91, 0x21, 0x2c, 0x2c, 0x49, 0x25, 0x00, 0xd5, 0xe4, + 0x81, 0x6b, 0x85, 0xad, 0x98, 0xaf, 0x06, 0x4a, 0x83, 0xb2, 0xe3, 0x42, + 0x39, 0x31, 0x50, 0xe1, 0x2d, 0x22, 0xe6, 0x07, 0x24, 0x65, 0x29, 0x3f, + 0x4c, 0xbd, 0x14, 0x8d, 0xfa, 0x31, 0xfa, 0xa4, 0xb5, 0x99, 0x04, 0xa2, + 0xa5, 0xcc, 0x3b, 0x12, 0xb1, 0xaa, 0x6a, 0x17, 0x78, 0x8b, 0xb3, 0xe4, + 0x3c, 0x4c, 0xc5, 0xaa, 0x79, 0x12, 0x17, 0xe0, 0x22, 0x4d, 0xf4, 0xa9, + 0xd5, 0xd0, 0xed, 0xf8, 0xfe, 0x0a, 0x45, 0x80, 0x9f, 0x3b, 0x74, 0xa0, + 0xb1, 0xda, 0x18, 0xfa, 0xc2, 0x7d, 0xf6, 0x18, 0x2e, 0xa9, 0x2b, 0x7e, + 0x69, 0x06, 0x43, 0x2d, 0x62, 0x09, 0x42, 0x10, 0x9f, 0x83, 0xad, 0xd9, + 0xdd, 0xcd, 0xcb, 0x1b, 0x33, 0x32, 0x3e, 0x1f, 0xf6, 0xac, 0x3b, 0xa3, + 0x29, 0xd7, 0xc0, 0x88, 0xf9, 0xb7, 0x4c, 0xcd, 0x0a, 0x1f, 0xb8, 0x0f, + 0xe6, 0xf7, 0xd7, 0x4d, 0x5f, 0x06, 0x12, 0x8a, 0x12, 0xa6, 0x2d, 0xbe, + 0x5c, 0x57, 0xf8, 0x7f, 0x54, 0x3f, 0x90, 0x83, 0x2c, 0x0a, 0xc5, 0x3d, + 0x03, 0x78, 0x8a, 0x68, 0xf0, 0xbd, 0xa5, 0x3e, 0xe7, 0x07, 0xab, 0xc8, + 0x58, 0x2f, 0x5c, 0xfd, 0xb5, 0x39, 0xe3, 0xc6, 0x1c, 0x27, 0xf9, 0x0b, + 0xc7, 0x4c, 0xcc, 0x67, 0x62, 0xe6, 0x79, 0xe8, 0xc1, 0x0a, 0x86, 0x8a, + 0xb2, 0x32, 0x7b, 0x90, 0x36, 0x50, 0x92, 0x1f, 0x3e, 0x68, 0x39, 0x1c, + 0x4d, 0x5d, 0xf8, 0x2b, 0xe8, 0x7d, 0xe2, 0x34, 0x61, 0x9e, 0xc3, 0x77, + 0xb9, 0x4c, 0x34, 0x08, 0xda, 0x31, 0xc9, 0x1d, 0xbd, 0x3b, 0x7b, 0xf1, + 0x14, 0xba, 0x3a, 0x34, 0x13, 0xaa, 0x5e, 0xa8, 0x36, 0xf6, 0xfe, 0xed, + 0x5b, 0xef, 0xaf, 0x24, 0x42, 0xba, 0xfc, 0xc9, 0x30, 0x84, 0xec, 0x49, + 0x14, 0xab, 0x58, 0x71, 0xfe, 0x4b, 0x6d, 0x7b, 0x9f, 0xbb, 0x3c, 0x83, + 0xdf, 0x3a, 0xfb, 0x54, 0xff, 0x36, 0xaa, 0x6c, 0x47, 0x94, 0xc0, 0xde, + 0x89, 0x2e, 0xac, 0x68, 0xee, 0xe8, 0xf4, 0xae, 0xa3, 0xe0, 0x91, 0x55, + 0x0b, 0x0c, 0xd7, 0xf4, 0x33, 0xb5, 0xf9, 0xf2, 0x9e, 0xda, 0x78, 0xe5, + 0x75, 0xec, 0xdb, 0xf6, 0xed, 0x27, 0x9f, 0x44, 0x19, 0x9f, 0xb7, 0xf0, + 0xac, 0x1b, 0x3a, 0xf5, 0x77, 0xc7, 0x76, 0x1e, 0x3f, 0x78, 0x12, 0x48, + 0x1d, 0xb8, 0xe0, 0x30, 0x29, 0x9a, 0x8c, 0x8f, 0x21, 0x44, 0x9c, 0x89, + 0xec, 0x8e, 0xd0, 0x81, 0xf5, 0x6a, 0xd0, 0xac, 0x5e, 0xf0, 0x0f, 0x88, + 0x86, 0x31, 0x2e, 0x15, 0x1e, 0x0d, 0x2d, 0xeb, 0x56, 0x30, 0x27, 0x02, + 0x93, 0xf4, 0x07, 0x07, 0xba, 0xf7, 0xbd, 0xe8, 0x27, 0x4f, 0xc6, 0xd9, + 0x57, 0x10, 0x3b, 0xf0, 0xff, 0x2f, 0x2d, 0x6b, 0xd0, 0x17, 0xb3, 0x49, + 0xeb, 0xc2, 0x49, 0xdb, +}; + + static_assert(sizeof(kInput) == sizeof(kOutput), "Input and output lengths don't match."); +static_assert(sizeof(kInput) == sizeof(kOverflowOutput), + "Input and output lengths don't match."); TEST(ChaChaTest, TestVector) { // Run the test with the test vector at all lengths. @@ -237,6 +331,22 @@ TEST(ChaChaTest, TestVector) { } } +TEST(ChaChaTest, CounterOverflow) { + // Run the test with the test vector at all lengths. + for (size_t len = 0; len <= sizeof(kInput); len++) { + SCOPED_TRACE(len); + + std::unique_ptr buf(new uint8_t[len]); + CRYPTO_chacha_20(buf.get(), kInput, len, kKey, kNonce, kOverflowCounter); + EXPECT_EQ(Bytes(kOverflowOutput, len), Bytes(buf.get(), len)); + + // Test the in-place version. + OPENSSL_memcpy(buf.get(), kInput, len); + CRYPTO_chacha_20(buf.get(), buf.get(), len, kKey, kNonce, kOverflowCounter); + EXPECT_EQ(Bytes(kOverflowOutput, len), Bytes(buf.get(), len)); + } +} + #if defined(CHACHA20_ASM) && defined(SUPPORTS_ABI_TEST) TEST(ChaChaTest, ABI) { uint32_t key[8]; diff --git a/crypto/chacha/internal.h b/crypto/chacha/internal.h index 1435e3b01e..5f442ec461 100644 --- a/crypto/chacha/internal.h +++ b/crypto/chacha/internal.h @@ -32,7 +32,14 @@ void CRYPTO_hchacha20(uint8_t out[32], const uint8_t key[32], defined(OPENSSL_ARM) || defined(OPENSSL_AARCH64)) #define CHACHA20_ASM -// ChaCha20_ctr32 is defined in asm/chacha-*.pl. +// ChaCha20_ctr32 encrypts |in_len| bytes from |in| and writes the result to +// |out|. If |in| and |out| alias, they must be equal. +// +// |counter[0]| is the initial 32-bit block counter, and the remainder is the +// 96-bit nonce. If the counter overflows, the output is undefined. The function +// will produce output, but the output may vary by machine and may not be +// self-consistent. (On some architectures, the assembly implements a mix of +// 64-bit and 32-bit counters.) void ChaCha20_ctr32(uint8_t *out, const uint8_t *in, size_t in_len, const uint32_t key[8], const uint32_t counter[4]); #endif diff --git a/crypto/curve25519/curve25519.c b/crypto/curve25519/curve25519.c index 7dea771e12..4e4e33aeb2 100644 --- a/crypto/curve25519/curve25519.c +++ b/crypto/curve25519/curve25519.c @@ -406,6 +406,6 @@ int X25519(uint8_t out_shared_key[32], const uint8_t private_key[32], } // The all-zero output results when the input is a point of small order. - // See https://www.rfc-editor.org/rfc/rfc7748#section-6.1. - return CRYPTO_memcmp(kZeros, out_shared_key, 32) != 0; + return constant_time_declassify_int( + CRYPTO_memcmp(kZeros, out_shared_key, 32)) != 0; } diff --git a/crypto/curve25519/curve25519_nohw.c b/crypto/curve25519/curve25519_nohw.c index c2adbaba90..3776c49c51 100644 --- a/crypto/curve25519/curve25519_nohw.c +++ b/crypto/curve25519/curve25519_nohw.c @@ -1946,7 +1946,7 @@ void x25519_scalar_mult_generic_nohw(uint8_t out[32], } void x25519_public_from_private_nohw(uint8_t out_public_value[32], - const uint8_t private_key[32]) { + const uint8_t private_key[32]) { uint8_t e[32]; OPENSSL_memcpy(e, private_key, 32); @@ -1966,4 +1966,5 @@ void x25519_public_from_private_nohw(uint8_t out_public_value[32], fe_loose_invert(&zminusy_inv, &zminusy); fe_mul_tlt(&zminusy_inv, &zplusy, &zminusy_inv); fe_tobytes(out_public_value, &zminusy_inv); + CONSTTIME_DECLASSIFY(out_public_value, 32); } diff --git a/crypto/curve25519/ed25519_test.cc b/crypto/curve25519/ed25519_test.cc index d56abe686c..0b7c585cf6 100644 --- a/crypto/curve25519/ed25519_test.cc +++ b/crypto/curve25519/ed25519_test.cc @@ -35,9 +35,15 @@ TEST(Ed25519Test, TestVectors) { ASSERT_TRUE(t->GetBytes(&expected_signature, "SIG")); ASSERT_EQ(64u, expected_signature.size()); + // Signing should not leak the private key or the message. + CONSTTIME_SECRET(private_key.data(), private_key.size()); + CONSTTIME_SECRET(message.data(), message.size()); uint8_t signature[64]; ASSERT_TRUE(ED25519_sign(signature, message.data(), message.size(), private_key.data())); + CONSTTIME_DECLASSIFY(signature, sizeof(signature)); + CONSTTIME_DECLASSIFY(message.data(), message.size()); + EXPECT_EQ(Bytes(expected_signature), Bytes(signature)); EXPECT_TRUE(ED25519_verify(message.data(), message.size(), signature, public_key.data())); @@ -114,9 +120,12 @@ TEST(Ed25519Test, KeypairFromSeed) { uint8_t seed[32]; OPENSSL_memcpy(seed, private_key1, sizeof(seed)); + CONSTTIME_SECRET(seed, sizeof(seed)); uint8_t public_key2[32], private_key2[64]; ED25519_keypair_from_seed(public_key2, private_key2, seed); + CONSTTIME_DECLASSIFY(public_key2, sizeof(public_key2)); + CONSTTIME_DECLASSIFY(private_key2, sizeof(private_key2)); EXPECT_EQ(Bytes(public_key1), Bytes(public_key2)); EXPECT_EQ(Bytes(private_key1), Bytes(private_key2)); diff --git a/crypto/curve25519/x25519_test.cc b/crypto/curve25519/x25519_test.cc index 3cea9fbb9d..f512d01ab1 100644 --- a/crypto/curve25519/x25519_test.cc +++ b/crypto/curve25519/x25519_test.cc @@ -27,9 +27,31 @@ #include "../test/wycheproof_util.h" #include "internal.h" +static inline int ctwrapX25519(uint8_t out_shared_key[32], + const uint8_t private_key[32], + const uint8_t peer_public_value[32]) { + uint8_t scalar[32], point[32]; + // Copy all the secrets into a temporary buffer, so we can run constant-time + // validation on them. + OPENSSL_memcpy(scalar, private_key, sizeof(scalar)); + OPENSSL_memcpy(point, peer_public_value, sizeof(point)); + + // X25519 should not leak the private key. + CONSTTIME_SECRET(scalar, sizeof(scalar)); + // All other inputs are also marked as secret. This is not to support any + // particular use case for calling X25519 with a secret *point*, but + // rather to ensure that the choice of the point cannot influence whether + // the scalar is leaked or not. Same for the initial contents of the + // output buffer. This conservative choice may be revised in the future. + CONSTTIME_SECRET(point, sizeof(point)); + CONSTTIME_SECRET(out_shared_key, 32); + int r = X25519(out_shared_key, scalar, point); + CONSTTIME_DECLASSIFY(out_shared_key, 32); + return r; +} TEST(X25519Test, TestVector) { - // Taken from https://tools.ietf.org/html/rfc7748#section-5.2 + // Taken from https://www.rfc-editor.org/rfc/rfc7748#section-5.2 static const uint8_t kScalar1[32] = { 0xa5, 0x46, 0xe3, 0x6b, 0xf0, 0x52, 0x7c, 0x9d, 0x3b, 0x16, 0x15, 0x4b, 0x82, 0x46, 0x5e, 0xdd, 0x62, 0x14, 0x4c, 0x0a, 0xc1, 0xfc, @@ -41,9 +63,8 @@ TEST(X25519Test, TestVector) { 0x35, 0x3b, 0x10, 0xa9, 0x03, 0xa6, 0xd0, 0xab, 0x1c, 0x4c, }; - uint8_t out[32]; - EXPECT_TRUE(X25519(out, kScalar1, kPoint1)); - + uint8_t out[32], secret[32]; + EXPECT_TRUE(ctwrapX25519(out, kScalar1, kPoint1)); static const uint8_t kExpected1[32] = { 0xc3, 0xda, 0x55, 0x37, 0x9d, 0xe9, 0xc6, 0x90, 0x8e, 0x94, 0xea, 0x4d, 0xf2, 0x8d, 0x08, 0x4f, 0x32, 0xec, 0xcf, 0x03, 0x49, 0x1c, @@ -61,15 +82,58 @@ TEST(X25519Test, TestVector) { 0x9d, 0x05, 0x38, 0xae, 0x2c, 0x31, 0xdb, 0xe7, 0x10, 0x6f, 0xc0, 0x3c, 0x3e, 0xfc, 0x4c, 0xd5, 0x49, 0xc7, 0x15, 0xa4, 0x93, }; - - EXPECT_TRUE(X25519(out, kScalar2, kPoint2)); - + EXPECT_TRUE(ctwrapX25519(out, kScalar2, kPoint2)); static const uint8_t kExpected2[32] = { 0x95, 0xcb, 0xde, 0x94, 0x76, 0xe8, 0x90, 0x7d, 0x7a, 0xad, 0xe4, 0x5c, 0xb4, 0xb8, 0x73, 0xf8, 0x8b, 0x59, 0x5a, 0x68, 0x79, 0x9f, 0xa1, 0x52, 0xe6, 0xf8, 0xf7, 0x64, 0x7a, 0xac, 0x79, 0x57, }; EXPECT_EQ(Bytes(kExpected2), Bytes(out)); + + // Taken from https://www.rfc-editor.org/rfc/rfc7748.html#section-6.1 + static const uint8_t kPrivateA[32] = { + 0x77, 0x07, 0x6d, 0x0a, 0x73, 0x18, 0xa5, 0x7d, 0x3c, 0x16, 0xc1, + 0x72, 0x51, 0xb2, 0x66, 0x45, 0xdf, 0x4c, 0x2f, 0x87, 0xeb, 0xc0, + 0x99, 0x2a, 0xb1, 0x77, 0xfb, 0xa5, 0x1d, 0xb9, 0x2c, 0x2a, + }; + static const uint8_t kPublicA[32] = { + 0x85, 0x20, 0xf0, 0x09, 0x89, 0x30, 0xa7, 0x54, 0x74, 0x8b, 0x7d, + 0xdc, 0xb4, 0x3e, 0xf7, 0x5a, 0x0d, 0xbf, 0x3a, 0x0d, 0x26, 0x38, + 0x1a, 0xf4, 0xeb, 0xa4, 0xa9, 0x8e, 0xaa, 0x9b, 0x4e, 0x6a, + }; + static const uint8_t kPrivateB[32] = { + 0x5d, 0xab, 0x08, 0x7e, 0x62, 0x4a, 0x8a, 0x4b, 0x79, 0xe1, 0x7f, + 0x8b, 0x83, 0x80, 0x0e, 0xe6, 0x6f, 0x3b, 0xb1, 0x29, 0x26, 0x18, + 0xb6, 0xfd, 0x1c, 0x2f, 0x8b, 0x27, 0xff, 0x88, 0xe0, 0xeb, + }; + static const uint8_t kPublicB[32] = { + 0xde, 0x9e, 0xdb, 0x7d, 0x7b, 0x7d, 0xc1, 0xb4, 0xd3, 0x5b, 0x61, + 0xc2, 0xec, 0xe4, 0x35, 0x37, 0x3f, 0x83, 0x43, 0xc8, 0x5b, 0x78, + 0x67, 0x4d, 0xad, 0xfc, 0x7e, 0x14, 0x6f, 0x88, 0x2b, 0x4f, + }; + static const uint8_t kSecret[32] = { + 0x4a, 0x5d, 0x9d, 0x5b, 0xa4, 0xce, 0x2d, 0xe1, 0x72, 0x8e, 0x3b, + 0xf4, 0x80, 0x35, 0x0f, 0x25, 0xe0, 0x7e, 0x21, 0xc9, 0x47, 0xd1, + 0x9e, 0x33, 0x76, 0xf0, 0x9b, 0x3c, 0x1e, 0x16, 0x17, 0x42, + }; + + OPENSSL_memcpy(secret, kPrivateA, sizeof(secret)); + CONSTTIME_SECRET(secret, sizeof(secret)); + X25519_public_from_private(out, secret); + CONSTTIME_DECLASSIFY(out, sizeof(out)); + EXPECT_EQ(Bytes(out), Bytes(kPublicA)); + + OPENSSL_memcpy(secret, kPrivateB, sizeof(secret)); + CONSTTIME_SECRET(secret, sizeof(secret)); + X25519_public_from_private(out, secret); + CONSTTIME_DECLASSIFY(out, sizeof(out)); + EXPECT_EQ(Bytes(out), Bytes(kPublicB)); + + ctwrapX25519(out, kPrivateA, kPublicB); + EXPECT_EQ(Bytes(out), Bytes(kSecret)); + + ctwrapX25519(out, kPrivateB, kPublicA); + EXPECT_EQ(Bytes(out), Bytes(kSecret)); } TEST(X25519Test, SmallOrder) { @@ -83,7 +147,7 @@ TEST(X25519Test, SmallOrder) { OPENSSL_memset(private_key, 0x11, sizeof(private_key)); OPENSSL_memset(out, 0xff, sizeof(out)); - EXPECT_FALSE(X25519(out, private_key, kSmallOrderPoint)) + EXPECT_FALSE(ctwrapX25519(out, private_key, kSmallOrderPoint)) << "X25519 returned success with a small-order input."; // For callers which don't check, |out| should still be filled with zeros. @@ -96,7 +160,7 @@ TEST(X25519Test, Iterated) { uint8_t scalar[32] = {9}, point[32] = {9}, out[32]; for (unsigned i = 0; i < 1000; i++) { - EXPECT_TRUE(X25519(out, scalar, point)); + EXPECT_TRUE(ctwrapX25519(out, scalar, point)); OPENSSL_memcpy(point, scalar, sizeof(point)); OPENSSL_memcpy(scalar, out, sizeof(scalar)); } @@ -115,7 +179,7 @@ TEST(X25519Test, DISABLED_IteratedLarge) { uint8_t scalar[32] = {9}, point[32] = {9}, out[32]; for (unsigned i = 0; i < 1000000; i++) { - EXPECT_TRUE(X25519(out, scalar, point)); + EXPECT_TRUE(ctwrapX25519(out, scalar, point)); OPENSSL_memcpy(point, scalar, sizeof(point)); OPENSSL_memcpy(scalar, out, sizeof(scalar)); } @@ -143,8 +207,9 @@ TEST(X25519Test, Wycheproof) { ASSERT_TRUE(t->GetBytes(&shared, "shared")); ASSERT_EQ(32u, priv.size()); ASSERT_EQ(32u, pub.size()); + uint8_t secret[32]; - int ret = X25519(secret, priv.data(), pub.data()); + int ret = ctwrapX25519(secret, priv.data(), pub.data()); EXPECT_EQ(ret, result.IsValid({"NonCanonicalPublic", "Twist"}) ? 1 : 0); EXPECT_EQ(Bytes(secret), Bytes(shared)); }); diff --git a/crypto/fipsmodule/bn/bn_test.cc b/crypto/fipsmodule/bn/bn_test.cc index a11d04de91..51d46921ec 100644 --- a/crypto/fipsmodule/bn/bn_test.cc +++ b/crypto/fipsmodule/bn/bn_test.cc @@ -74,6 +74,7 @@ #include #include +#include #include #include @@ -908,6 +909,14 @@ static void TestModInv(BIGNUMFileTest *t, BN_CTX *ctx) { bn_mod_inverse_consttime(ret.get(), &no_inverse, a.get(), m.get(), ctx)); EXPECT_BIGNUMS_EQUAL("inv(A) (mod M) (constant-time)", mod_inv.get(), ret.get()); + + ASSERT_TRUE(BN_copy(ret.get(), m.get())); + ASSERT_TRUE(BN_mod_inverse(ret.get(), a.get(), ret.get(), ctx)); + EXPECT_BIGNUMS_EQUAL("inv(A) (mod M) (ret == m)", mod_inv.get(), ret.get()); + + ASSERT_TRUE(BN_copy(ret.get(), a.get())); + ASSERT_TRUE(BN_mod_inverse(ret.get(), ret.get(), m.get(), ctx)); + EXPECT_BIGNUMS_EQUAL("inv(A) (mod M) (ret == a)", mod_inv.get(), ret.get()); } static void TestGCD(BIGNUMFileTest *t, BN_CTX *ctx) { @@ -2796,6 +2805,27 @@ TEST_F(BNTest, MontgomeryLarge) { ctx(), nullptr)); } +TEST_F(BNTest, FormatWord) { + char buf[32]; + snprintf(buf, sizeof(buf), BN_DEC_FMT1, BN_ULONG{1234}); + EXPECT_STREQ(buf, "1234"); + snprintf(buf, sizeof(buf), BN_HEX_FMT1, BN_ULONG{1234}); + EXPECT_STREQ(buf, "4d2"); + + // |BN_HEX_FMT2| is zero-padded up to the maximum value. +#if defined(OPENSSL_64_BIT) + snprintf(buf, sizeof(buf), BN_HEX_FMT2, BN_ULONG{1234}); + EXPECT_STREQ(buf, "00000000000004d2"); + snprintf(buf, sizeof(buf), BN_HEX_FMT2, std::numeric_limits::max()); + EXPECT_STREQ(buf, "ffffffffffffffff"); +#else + snprintf(buf, sizeof(buf), BN_HEX_FMT2, BN_ULONG{1234}); + EXPECT_STREQ(buf, "000004d2"); + snprintf(buf, sizeof(buf), BN_HEX_FMT2, std::numeric_limits::max()); + EXPECT_STREQ(buf, "ffffffff"); +#endif +} + #if defined(OPENSSL_BN_ASM_MONT) && defined(SUPPORTS_ABI_TEST) TEST_F(BNTest, BNMulMontABI) { for (size_t words : {4, 5, 6, 7, 8, 16, 32}) { diff --git a/crypto/fipsmodule/bn/bytes.c b/crypto/fipsmodule/bn/bytes.c index 39083b775b..97b0d3f958 100644 --- a/crypto/fipsmodule/bn/bytes.c +++ b/crypto/fipsmodule/bn/bytes.c @@ -205,6 +205,18 @@ static int fits_in_bytes(const BN_ULONG *words, size_t num_words, return mask == 0; } +void bn_assert_fits_in_bytes(const BIGNUM *bn, size_t num) { + const uint8_t *bytes = (const uint8_t *)bn->d; + size_t tot_bytes = bn->width * sizeof(BN_ULONG); + if (tot_bytes > num) { + CONSTTIME_DECLASSIFY(bytes + num, tot_bytes - num); + for (size_t i = num; i < tot_bytes; i++) { + assert(bytes[i] == 0); + } + (void)bytes; + } +} + void bn_words_to_big_endian(uint8_t *out, size_t out_len, const BN_ULONG *in, size_t in_len) { // The caller should have selected an output length without truncation. diff --git a/crypto/fipsmodule/bn/exponentiation.c b/crypto/fipsmodule/bn/exponentiation.c index f430f45d2f..da4152e4cd 100644 --- a/crypto/fipsmodule/bn/exponentiation.c +++ b/crypto/fipsmodule/bn/exponentiation.c @@ -640,7 +640,8 @@ int BN_mod_exp_mont(BIGNUM *rr, const BIGNUM *a, const BIGNUM *p, OPENSSL_PUT_ERROR(BN, BN_R_NEGATIVE_NUMBER); return 0; } - if (a->neg || BN_ucmp(a, m) >= 0) { + // |a| is secret, but |a < m| is not. + if (a->neg || constant_time_declassify_int(BN_ucmp(a, m)) >= 0) { OPENSSL_PUT_ERROR(BN, BN_R_INPUT_NOT_REDUCED); return 0; } diff --git a/crypto/fipsmodule/bn/gcd.c b/crypto/fipsmodule/bn/gcd.c index e8cc764cf8..df12569a71 100644 --- a/crypto/fipsmodule/bn/gcd.c +++ b/crypto/fipsmodule/bn/gcd.c @@ -263,15 +263,14 @@ int BN_mod_inverse_odd(BIGNUM *out, int *out_no_inverse, const BIGNUM *a, // Now Y*a == A (mod |n|). // Y*a == 1 (mod |n|) - if (!Y->neg && BN_ucmp(Y, n) < 0) { - if (!BN_copy(R, Y)) { - goto err; - } - } else { - if (!BN_nnmod(R, Y, n, ctx)) { + if (Y->neg || BN_ucmp(Y, n) >= 0) { + if (!BN_nnmod(Y, Y, n, ctx)) { goto err; } } + if (!BN_copy(R, Y)) { + goto err; + } ret = 1; diff --git a/crypto/fipsmodule/bn/internal.h b/crypto/fipsmodule/bn/internal.h index 30437deafe..417a3eac6c 100644 --- a/crypto/fipsmodule/bn/internal.h +++ b/crypto/fipsmodule/bn/internal.h @@ -221,8 +221,8 @@ extern "C" { #define BN_GENCB_NEW_STYLE 1 #define BN_GENCB_OLD_STYLE 2 -// bn_minimal_width returns the minimal value of |bn->top| which fits the -// value of |bn|. +// bn_minimal_width returns the minimal number of words needed to represent +// |bn|. int bn_minimal_width(const BIGNUM *bn); // bn_set_minimal_width sets |bn->width| to |bn_minimal_width(bn)|. If |bn| is @@ -238,7 +238,7 @@ int bn_wexpand(BIGNUM *bn, size_t words); // than a number of words. int bn_expand(BIGNUM *bn, size_t bits); -// bn_resize_words adjusts |bn->top| to be |words|. It returns one on success +// bn_resize_words adjusts |bn->width| to be |words|. It returns one on success // and zero on allocation error or if |bn|'s value is too large. OPENSSL_EXPORT int bn_resize_words(BIGNUM *bn, size_t words); @@ -267,6 +267,12 @@ int bn_fits_in_words(const BIGNUM *bn, size_t num); // is representable in |num| words. Otherwise, it returns zero. int bn_copy_words(BN_ULONG *out, size_t num, const BIGNUM *bn); +// bn_assert_fits_in_bytes asserts that |bn| fits in |num| bytes. This is a +// no-op in release builds, but triggers an assert in debug builds, and +// declassifies all bytes which are therefore known to be zero in constant-time +// validation. +void bn_assert_fits_in_bytes(const BIGNUM *bn, size_t num); + // bn_mul_add_words multiples |ap| by |w|, adds the result to |rp|, and places // the result in |rp|. |ap| and |rp| must both be |num| words long. It returns // the carry word of the operation. |ap| and |rp| may be equal but otherwise may diff --git a/crypto/fipsmodule/ec/ec.c b/crypto/fipsmodule/ec/ec.c index 112933f126..ea96c33741 100644 --- a/crypto/fipsmodule/ec/ec.c +++ b/crypto/fipsmodule/ec/ec.c @@ -1122,8 +1122,11 @@ int ec_point_mul_scalar_base(const EC_GROUP *group, EC_JACOBIAN *r, group->meth->mul_base(group, r, scalar); // Check the result is on the curve to defend against fault attacks or bugs. - // This has negligible cost compared to the multiplication. - if (!ec_GFp_simple_is_on_curve(group, r)) { + // This has negligible cost compared to the multiplication. This can only + // happen on bug or CPU fault, so it is okay to leak this information (if the + // computed point is on the curve or not). The alternative would be to + // proceed with bad data. + if (!constant_time_declassify_int(ec_GFp_simple_is_on_curve(group, r))) { OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR); return 0; } diff --git a/crypto/fipsmodule/ec/ec_montgomery.c b/crypto/fipsmodule/ec/ec_montgomery.c index eeaee64ca9..78e0507699 100644 --- a/crypto/fipsmodule/ec/ec_montgomery.c +++ b/crypto/fipsmodule/ec/ec_montgomery.c @@ -177,7 +177,8 @@ void ec_GFp_mont_felem_exp(const EC_GROUP *group, EC_FELEM *out, static int ec_GFp_mont_point_get_affine_coordinates(const EC_GROUP *group, const EC_JACOBIAN *point, EC_FELEM *x, EC_FELEM *y) { - if (ec_GFp_simple_is_at_infinity(group, point)) { + if (constant_time_declassify_int( + ec_GFp_simple_is_at_infinity(group, point))) { OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY); return 0; } @@ -317,7 +318,7 @@ void ec_GFp_mont_add(const EC_GROUP *group, EC_JACOBIAN *out, // This case will never occur in the constant-time |ec_GFp_mont_mul|. BN_ULONG is_nontrivial_double = ~xneq & ~yneq & z1nz & z2nz; - if (is_nontrivial_double) { + if (constant_time_declassify_w(is_nontrivial_double)) { ec_GFp_mont_dbl(group, out, a); return; } diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h index 1a1b84dfe8..4d693f678c 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/crypto/fipsmodule/ec/internal.h @@ -491,7 +491,8 @@ struct ec_method_st { // point_get_affine_coordinates sets |*x| and |*y| to the affine coordinates // of |p|. Either |x| or |y| may be NULL to omit it. It returns one on success - // and zero if |p| is the point at infinity. + // and zero if |p| is the point at infinity. It leaks whether |p| was the + // point at infinity, but otherwise treats |p| as secret. int (*point_get_affine_coordinates)(const EC_GROUP *, const EC_JACOBIAN *p, EC_FELEM *x, EC_FELEM *y); diff --git a/crypto/fipsmodule/ec/p224-64.c b/crypto/fipsmodule/ec/p224-64.c index 9aaf2154b6..720d1bcde5 100644 --- a/crypto/fipsmodule/ec/p224-64.c +++ b/crypto/fipsmodule/ec/p224-64.c @@ -734,8 +734,8 @@ static void p224_point_add(p224_felem x3, p224_felem y3, p224_felem z3, // tmp[i] < 2^116 + 2^64 + 8 < 2^117 p224_felem_reduce(ftmp, tmp); - // the formulae are incorrect if the points are equal - // so we check for this and do doubling if this happens + // The formulae are incorrect if the points are equal, so we check for this + // and do doubling if this happens. x_equal = p224_felem_is_zero(ftmp); y_equal = p224_felem_is_zero(ftmp3); z1_is_zero = p224_felem_is_zero(z1); @@ -743,7 +743,7 @@ static void p224_point_add(p224_felem x3, p224_felem y3, p224_felem z3, // In affine coordinates, (X_1, Y_1) == (X_2, Y_2) p224_limb is_nontrivial_double = x_equal & y_equal & (1 - z1_is_zero) & (1 - z2_is_zero); - if (is_nontrivial_double) { + if (constant_time_declassify_w(is_nontrivial_double)) { p224_point_double(x3, y3, z3, x1, y1, z1); return; } @@ -862,7 +862,8 @@ static crypto_word_t p224_get_bit(const EC_SCALAR *in, size_t i) { static int ec_GFp_nistp224_point_get_affine_coordinates( const EC_GROUP *group, const EC_JACOBIAN *point, EC_FELEM *x, EC_FELEM *y) { - if (ec_GFp_simple_is_at_infinity(group, point)) { + if (constant_time_declassify_int( + ec_GFp_simple_is_at_infinity(group, point))) { OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY); return 0; } diff --git a/crypto/fipsmodule/ec/p256-nistz.c b/crypto/fipsmodule/ec/p256-nistz.c index 0c499842ef..0229a1aaea 100644 --- a/crypto/fipsmodule/ec/p256-nistz.c +++ b/crypto/fipsmodule/ec/p256-nistz.c @@ -437,7 +437,8 @@ static void ecp_nistz256_points_mul_public(const EC_GROUP *group, static int ecp_nistz256_get_affine(const EC_GROUP *group, const EC_JACOBIAN *point, EC_FELEM *x, EC_FELEM *y) { - if (ec_GFp_simple_is_at_infinity(group, point)) { + if (constant_time_declassify_int( + ec_GFp_simple_is_at_infinity(group, point))) { OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY); return 0; } diff --git a/crypto/fipsmodule/ec/p256.c b/crypto/fipsmodule/ec/p256.c index cd4563a419..8684d1e1b0 100644 --- a/crypto/fipsmodule/ec/p256.c +++ b/crypto/fipsmodule/ec/p256.c @@ -324,7 +324,7 @@ static void fiat_p256_point_add(fiat_p256_felem x3, fiat_p256_felem y3, fiat_p256_limb_t is_nontrivial_double = constant_time_is_zero_w(xneq | yneq) & ~constant_time_is_zero_w(z1nz) & ~constant_time_is_zero_w(z2nz); - if (is_nontrivial_double) { + if (constant_time_declassify_w(is_nontrivial_double)) { fiat_p256_point_double(x3, y3, z3, x1, y1, z1); return; } @@ -416,7 +416,8 @@ static crypto_word_t fiat_p256_get_bit(const EC_SCALAR *in, int i) { static int ec_GFp_nistp256_point_get_affine_coordinates( const EC_GROUP *group, const EC_JACOBIAN *point, EC_FELEM *x_out, EC_FELEM *y_out) { - if (ec_GFp_simple_is_at_infinity(group, point)) { + if (constant_time_declassify_int( + ec_GFp_simple_is_at_infinity(group, point))) { OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY); return 0; } diff --git a/crypto/fipsmodule/ec/p384.c b/crypto/fipsmodule/ec/p384.c index 40b0090943..42f1ee4399 100644 --- a/crypto/fipsmodule/ec/p384.c +++ b/crypto/fipsmodule/ec/p384.c @@ -454,7 +454,7 @@ static void p384_point_add(p384_felem x3, p384_felem y3, p384_felem z3, p384_limb_t is_nontrivial_double = constant_time_is_zero_w(xneq | yneq) & ~constant_time_is_zero_w(z1nz) & ~constant_time_is_zero_w(z2nz); - if (is_nontrivial_double) { + if (constant_time_declassify_w(is_nontrivial_double)) { p384_point_double(x3, y3, z3, x1, y1, z1); return; } @@ -502,7 +502,7 @@ static int ec_GFp_nistp384_point_get_affine_coordinates( const EC_GROUP *group, const EC_JACOBIAN *point, EC_FELEM *x_out, EC_FELEM *y_out) { - if (ec_GFp_simple_is_at_infinity(group, point)) { + if (constant_time_declassify_w(ec_GFp_simple_is_at_infinity(group, point))) { OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY); return 0; } diff --git a/crypto/fipsmodule/ec/p521.c b/crypto/fipsmodule/ec/p521.c index b467284790..ea5982c26e 100644 --- a/crypto/fipsmodule/ec/p521.c +++ b/crypto/fipsmodule/ec/p521.c @@ -461,7 +461,7 @@ static void p521_point_add(p521_felem x3, p521_felem y3, p521_felem z3, p521_limb_t is_nontrivial_double = constant_time_is_zero_w(xneq | yneq) & ~constant_time_is_zero_w(z1nz) & ~constant_time_is_zero_w(z2nz); - if (is_nontrivial_double) { + if (constant_time_declassify_w(is_nontrivial_double)) { p521_point_double(x3, y3, z3, x1, y1, z1); return; } @@ -509,7 +509,7 @@ static int ec_GFp_nistp521_point_get_affine_coordinates( const EC_GROUP *group, const EC_JACOBIAN *point, EC_FELEM *x_out, EC_FELEM *y_out) { - if (ec_GFp_simple_is_at_infinity(group, point)) { + if (constant_time_declassify_w(ec_GFp_simple_is_at_infinity(group, point))) { OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY); return 0; } diff --git a/crypto/fipsmodule/ecdsa/ecdsa.c b/crypto/fipsmodule/ecdsa/ecdsa.c index 9f7a4ab222..3d38ef3472 100644 --- a/crypto/fipsmodule/ecdsa/ecdsa.c +++ b/crypto/fipsmodule/ecdsa/ecdsa.c @@ -224,7 +224,7 @@ static ECDSA_SIG *ecdsa_sign_impl(const EC_GROUP *group, int *out_retry, return NULL; } - if (ec_scalar_is_zero(group, &r)) { + if (constant_time_declassify_int(ec_scalar_is_zero(group, &r))) { *out_retry = 1; return NULL; } @@ -251,11 +251,13 @@ static ECDSA_SIG *ecdsa_sign_impl(const EC_GROUP *group, int *out_retry, ec_scalar_inv0_montgomery(group, &tmp, k); // tmp = k^-1 R^2 ec_scalar_from_montgomery(group, &tmp, &tmp); // tmp = k^-1 R ec_scalar_mul_montgomery(group, &s, &s, &tmp); - if (ec_scalar_is_zero(group, &s)) { + if (constant_time_declassify_int(ec_scalar_is_zero(group, &s))) { *out_retry = 1; return NULL; } + CONSTTIME_DECLASSIFY(r.words, sizeof(r.words)); + CONSTTIME_DECLASSIFY(s.words, sizeof(r.words)); ECDSA_SIG *ret = ECDSA_SIG_new(); if (ret == NULL || // !bn_set_words(ret->r, r.words, order->width) || @@ -348,6 +350,10 @@ ECDSA_SIG *ECDSA_do_sign(const uint8_t *digest, size_t digest_len, return NULL; } + // TODO(davidben): Move this inside |ec_random_nonzero_scalar| or lower, so + // that all scalars we generate are, by default, secret. + CONSTTIME_SECRET(k.words, sizeof(k.words)); + int retry; ECDSA_SIG *sig = ecdsa_sign_impl(group, &retry, priv_key, &k, digest, digest_len); diff --git a/crypto/fipsmodule/rsa/padding.c b/crypto/fipsmodule/rsa/padding.c index 36e068ec2b..932d0f03d4 100644 --- a/crypto/fipsmodule/rsa/padding.c +++ b/crypto/fipsmodule/rsa/padding.c @@ -462,10 +462,16 @@ int RSA_padding_check_PKCS1_OAEP_mgf1(uint8_t *out, size_t *out_len, bad |= looking_for_one_byte; - if (bad) { + // Whether the overall padding was valid or not in OAEP is public. + if (constant_time_declassify_w(bad)) { goto decoding_err; } + // Once the padding is known to be valid, the output length is also public. + OPENSSL_STATIC_ASSERT(sizeof(size_t) <= sizeof(crypto_word_t), + size_t_does_not_fit_in_crypto_word_t); + one_index = constant_time_declassify_w(one_index); + one_index++; size_t mlen = dblen - one_index; if (max_out < mlen) { @@ -480,8 +486,8 @@ int RSA_padding_check_PKCS1_OAEP_mgf1(uint8_t *out, size_t *out_len, return 1; decoding_err: - // to avoid chosen ciphertext attacks, the error message should not reveal - // which kind of decoding error happened + // To avoid chosen ciphertext attacks, the error message should not reveal + // which kind of decoding error happened. OPENSSL_PUT_ERROR(RSA, RSA_R_OAEP_DECODING_ERROR); err: OPENSSL_free(db); diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c index 29529d3efd..79466f16f4 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.c +++ b/crypto/fipsmodule/rsa/rsa_impl.c @@ -763,6 +763,8 @@ int rsa_default_private_transform(RSA *rsa, uint8_t *out, const uint8_t *in, goto err; } + // The caller should have ensured this. + assert(len == BN_num_bytes(rsa->n)); if (BN_bin2bn(in, len, f) == NULL) { goto err; } @@ -824,16 +826,16 @@ int rsa_default_private_transform(RSA *rsa, uint8_t *out, const uint8_t *in, // works when the CRT isn't used. That attack is much less likely to succeed // than the CRT attack, but there have likely been improvements since 1997. // - // This check is cheap assuming |e| is small; it almost always is. + // This check is cheap assuming |e| is small, which we require in + // |rsa_check_public_key|. if (rsa->e != NULL) { BIGNUM *vrfy = BN_CTX_get(ctx); if (vrfy == NULL || !BN_mod_exp_mont(vrfy, result, rsa->e, rsa->n, ctx, rsa->mont_n) || - !BN_equal_consttime(vrfy, f)) { + !constant_time_declassify_int(BN_equal_consttime(vrfy, f))) { OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR); goto err; } - } if (do_blinding && @@ -847,6 +849,7 @@ int rsa_default_private_transform(RSA *rsa, uint8_t *out, const uint8_t *in, // // See Falko Strenzke, "Manger's Attack revisited", ICICS 2010. assert(result->width == rsa->mont_n->N.width); + bn_assert_fits_in_bytes(result, len); if (!BN_bn2bin_padded(out, len, result)) { OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR); goto err; @@ -965,11 +968,18 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) { // so it is correct mod q. Finally, the result is bounded by [m1, n + m1), // and the result is at least |m1|, so this must be the unique answer in // [0, n). - !bn_mul_consttime(r0, r0, q, ctx) || - !bn_uadd_consttime(r0, r0, m1) || - // The result should be bounded by |n|, but fixed-width operations may - // bound the width slightly higher, so fix it. - !bn_resize_words(r0, n->width)) { + !bn_mul_consttime(r0, r0, q, ctx) || // + !bn_uadd_consttime(r0, r0, m1)) { + goto err; + } + + // The result should be bounded by |n|, but fixed-width operations may + // bound the width slightly higher, so fix it. This trips constant-time checks + // because a naive data flow analysis does not realize the excess words are + // publicly zero. + assert(BN_cmp(r0, n) < 0); + bn_assert_fits_in_bytes(r0, BN_num_bytes(n)); + if (!bn_resize_words(r0, n->width)) { goto err; } diff --git a/crypto/internal.h b/crypto/internal.h index a0b958d5a6..295f1dd971 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -445,20 +445,44 @@ static inline int constant_time_select_int(crypto_word_t mask, int a, int b) { // of memory as secret. Secret data is tracked as it flows to registers and // other parts of a memory. If secret data is used as a condition for a branch, // or as a memory index, it will trigger warnings in valgrind. -#define CONSTTIME_SECRET(x, y) VALGRIND_MAKE_MEM_UNDEFINED(x, y) +#define CONSTTIME_SECRET(ptr, len) VALGRIND_MAKE_MEM_UNDEFINED(ptr, len) // CONSTTIME_DECLASSIFY takes a pointer and a number of bytes and marks that // region of memory as public. Public data is not subject to constant-time // rules. -#define CONSTTIME_DECLASSIFY(x, y) VALGRIND_MAKE_MEM_DEFINED(x, y) +#define CONSTTIME_DECLASSIFY(ptr, len) VALGRIND_MAKE_MEM_DEFINED(ptr, len) #else -#define CONSTTIME_SECRET(x, y) -#define CONSTTIME_DECLASSIFY(x, y) +#define CONSTTIME_SECRET(ptr, len) +#define CONSTTIME_DECLASSIFY(ptr, len) #endif // BORINGSSL_CONSTANT_TIME_VALIDATION +static inline crypto_word_t constant_time_declassify_w(crypto_word_t v) { + // Return |v| through a value barrier to be safe. Valgrind-based constant-time + // validation is partly to check the compiler has not undone any constant-time + // work. Any place |BORINGSSL_CONSTANT_TIME_VALIDATION| influences + // optimizations, this validation is inaccurate. + // + // However, by sending pointers through valgrind, we likely inhibit escape + // analysis. On local variables, particularly booleans, we likely + // significantly impact optimizations. + // + // Thus, to be safe, stick a value barrier, in hopes of comparably inhibiting + // compiler analysis. + CONSTTIME_DECLASSIFY(&v, sizeof(v)); + return value_barrier_w(v); +} + +static inline int constant_time_declassify_int(int v) { + OPENSSL_STATIC_ASSERT(sizeof(uint32_t) == sizeof(int), + int_is_not_the_same_size_as_uint32_t); + // See comment above. + CONSTTIME_DECLASSIFY(&v, sizeof(v)); + return value_barrier_u32(v); +} + // Thread-safe initialisation. diff --git a/include/openssl/bn.h b/include/openssl/bn.h index faec6694f6..a761317513 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -164,14 +164,12 @@ extern "C" { typedef uint64_t BN_ULONG; #define BN_BITS2 64 #define BN_DEC_FMT1 "%" PRIu64 -#define BN_DEC_FMT2 "%019" PRIu64 #define BN_HEX_FMT1 "%" PRIx64 #define BN_HEX_FMT2 "%016" PRIx64 #elif defined(OPENSSL_32_BIT) typedef uint32_t BN_ULONG; #define BN_BITS2 32 #define BN_DEC_FMT1 "%" PRIu32 -#define BN_DEC_FMT2 "%09" PRIu32 #define BN_HEX_FMT1 "%" PRIx32 #define BN_HEX_FMT2 "%08" PRIx32 #else diff --git a/include/openssl/chacha.h b/include/openssl/chacha.h index cfbaa75680..2868c29062 100644 --- a/include/openssl/chacha.h +++ b/include/openssl/chacha.h @@ -29,6 +29,12 @@ extern "C" { // CRYPTO_chacha_20 encrypts |in_len| bytes from |in| with the given key and // nonce and writes the result to |out|. If |in| and |out| alias, they must be // equal. The initial block counter is specified by |counter|. +// +// This function implements a 32-bit block counter as in RFC 8439. On overflow, +// the counter wraps. Reusing a key, nonce, and block counter combination is not +// secure, so wrapping is usually a bug in the caller. While it is possible to +// wrap without reuse with a large initial block counter, this is not +// recommended and may not be portable to other ChaCha20 implementations. OPENSSL_EXPORT void CRYPTO_chacha_20(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t key[32], const uint8_t nonce[12], uint32_t counter);