From a6a049a6fb51a052347611d41583a0622bc89d60 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Thu, 6 Dec 2018 17:15:58 -0800 Subject: [PATCH] Add start of infrastructure for checking constant-time properties. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Valgrind's checking of uninitialised memory behaves very much like a check for constant-time code: branches and memory indexes based on uninitialised memory trigger warnings. Therefore, if we can tell Valgrind that some secret is “uninitialised”, it'll give us a warning if we do something non-constant-time with it. This was the idea behind https://github.com/agl/ctgrind. But tricks like that are no longer needed because Valgrind now comes with support for marking regions of memory as defined or not. Therefore we can use that API to check constant-time code. This CL defines |CONSTTIME_SECRET| and |CONSTTIME_DECLASSIFY|, which are no-ops unless the code is built with |BORINGSSL_CONSTANT_TIME_VALIDATION| defined, which it isn't by default. So this CL is a no-op itself so far. But it does show that a couple of bits of constant-time time are, in fact, constant-time—seemingly even when compiled with optimisations, which is nice. The annotations in the RSA code are a) probably not marking all the secrets as secret, and b) triggers warnings that are a little interesting: The anti-glitch check calls |BN_mod_exp_mont| which checks that the input is less than the modulus. Of course, it is because the input is the RSA plaintext that we just decrypted, but the plaintext is supposed to be secret and so branching based on its contents isn't allows by Valgrind. The answer isn't totally clear, but I've run out of time on this for now. Change-Id: I1608ed0b22d201e97595fafe46127159e02d5b1b Reviewed-on: https://boringssl-review.googlesource.com/c/33504 Reviewed-by: Adam Langley Reviewed-by: David Benjamin Commit-Queue: Adam Langley --- CMakeLists.txt | 6 ++++++ crypto/cipher_extra/e_tls.c | 6 ++++++ crypto/fipsmodule/rsa/padding.c | 3 +++ crypto/fipsmodule/rsa/rsa_impl.c | 14 ++++++++++++++ crypto/internal.h | 24 ++++++++++++++++++++++++ ssl/handshake_server.cc | 4 ++++ 6 files changed, 57 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index bfde5d58..1f18782f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -336,6 +336,12 @@ if(OPENSSL_SMALL) add_definitions(-DOPENSSL_SMALL) endif() +if(CONSTANT_TIME_VALIDATION) + add_definitions(-DBORINGSSL_CONSTANT_TIME_VALIDATION) + # Asserts will often test secret data. + add_definitions(-DNDEBUG) +endif() + function(go_executable dest package) set(godeps "${CMAKE_SOURCE_DIR}/util/godeps.go") if(${CMAKE_VERSION} VERSION_LESS "3.7" OR diff --git a/crypto/cipher_extra/e_tls.c b/crypto/cipher_extra/e_tls.c index ff41989c..c812b6b8 100644 --- a/crypto/cipher_extra/e_tls.c +++ b/crypto/cipher_extra/e_tls.c @@ -299,6 +299,8 @@ static int aead_tls_open(const EVP_AEAD_CTX *ctx, uint8_t *out, size_t *out_len, total += len; assert(total == in_len); + CONSTTIME_SECRET(out, total); + // Remove CBC padding. Code from here on is timing-sensitive with respect to // |padding_ok| and |data_plus_mac_len| for CBC ciphers. size_t data_plus_mac_len; @@ -375,11 +377,15 @@ static int aead_tls_open(const EVP_AEAD_CTX *ctx, uint8_t *out, size_t *out_len, crypto_word_t good = constant_time_eq_int(CRYPTO_memcmp(record_mac, mac, mac_len), 0); good &= padding_ok; + CONSTTIME_DECLASSIFY(&good, sizeof(good)); if (!good) { OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BAD_DECRYPT); return 0; } + CONSTTIME_DECLASSIFY(&data_len, sizeof(data_len)); + CONSTTIME_DECLASSIFY(out, data_len); + // End of timing-sensitive code. *out_len = data_len; diff --git a/crypto/fipsmodule/rsa/padding.c b/crypto/fipsmodule/rsa/padding.c index b7998fe3..28f1b45b 100644 --- a/crypto/fipsmodule/rsa/padding.c +++ b/crypto/fipsmodule/rsa/padding.c @@ -233,6 +233,9 @@ int RSA_padding_check_PKCS1_type_2(uint8_t *out, size_t *out_len, // impossible to completely avoid Bleichenbacher's attack. Consumers should // use |RSA_PADDING_NONE| and perform the padding check in constant-time // combined with a swap to a random session key or other mitigation. + CONSTTIME_DECLASSIFY(&valid_index, sizeof(valid_index)); + CONSTTIME_DECLASSIFY(&zero_index, sizeof(zero_index)); + if (!valid_index) { OPENSSL_PUT_ERROR(RSA, RSA_R_PKCS_DECODING_ERROR); return 0; diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c index 895408df..903ba9a9 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.c +++ b/crypto/fipsmodule/rsa/rsa_impl.c @@ -120,6 +120,8 @@ static int ensure_fixed_copy(BIGNUM **out, const BIGNUM *in, int width) { return 0; } *out = copy; + CONSTTIME_SECRET(copy->d, sizeof(BN_ULONG) * width); + return 1; } @@ -166,6 +168,11 @@ static int freeze_private_key(RSA *rsa, BN_CTX *ctx) { } if (rsa->p != NULL && rsa->q != NULL) { + // TODO: p and q are also CONSTTIME_SECRET but not yet marked as such + // because the Montgomery code does things like test whether or not values + // are zero. So the secret marking probably needs to happen inside that + // code. + if (rsa->mont_p == NULL) { rsa->mont_p = BN_MONT_CTX_new_consttime(rsa->p, ctx); if (rsa->mont_p == NULL) { @@ -224,6 +231,9 @@ static int freeze_private_key(RSA *rsa, BN_CTX *ctx) { goto err; } rsa->inv_small_mod_large_mont = inv_small_mod_large_mont; + CONSTTIME_SECRET( + rsa->inv_small_mod_large_mont->d, + sizeof(BN_ULONG) * rsa->inv_small_mod_large_mont->width); } } } @@ -480,6 +490,7 @@ int rsa_default_sign_raw(RSA *rsa, size_t *out_len, uint8_t *out, goto err; } + CONSTTIME_DECLASSIFY(out, rsa_size); *out_len = rsa_size; ret = 1; @@ -539,8 +550,11 @@ int rsa_default_decrypt(RSA *rsa, size_t *out_len, uint8_t *out, size_t max_out, goto err; } + CONSTTIME_DECLASSIFY(&ret, sizeof(ret)); if (!ret) { OPENSSL_PUT_ERROR(RSA, RSA_R_PADDING_CHECK_FAILED); + } else { + CONSTTIME_DECLASSIFY(out, out_len); } err: diff --git a/crypto/internal.h b/crypto/internal.h index b98b556c..52799e89 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -116,6 +116,10 @@ #include #include +#if defined(BORINGSSL_CONSTANT_TIME_VALIDATION) +#include +#endif + #if !defined(__cplusplus) #if defined(_MSC_VER) #define alignas(x) __declspec(align(x)) @@ -365,6 +369,26 @@ static inline int constant_time_select_int(crypto_word_t mask, int a, int b) { (crypto_word_t)(b))); } +#if defined(BORINGSSL_CONSTANT_TIME_VALIDATION) + +// CONSTTIME_SECRET takes a pointer and a number of bytes and marks that region +// 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) + +// 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) + +#else + +#define CONSTTIME_SECRET(x, y) +#define CONSTTIME_DECLASSIFY(x, y) + +#endif // BORINGSSL_CONSTANT_TIME_VALIDATION + // Thread-safe initialisation. diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index 8b3b9428..15720967 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc @@ -1226,6 +1226,8 @@ static enum ssl_hs_wait_t do_read_client_key_exchange(SSL_HANDSHAKE *hs) { return ssl_hs_error; } + CONSTTIME_SECRET(decrypt_buf.data(), decrypt_len); + // Prepare a random premaster, to be used on invalid padding. See RFC 5246, // section 7.4.7.1. if (!premaster_secret.Init(SSL_MAX_MASTER_KEY_LENGTH) || @@ -1348,6 +1350,8 @@ static enum ssl_hs_wait_t do_read_client_key_exchange(SSL_HANDSHAKE *hs) { return ssl_hs_error; } hs->new_session->extended_master_secret = hs->extended_master_secret; + CONSTTIME_DECLASSIFY(hs->new_session->master_key, + hs->new_session->master_key_length); ssl->method->next_message(ssl); hs->state = state12_read_client_certificate_verify;