Add start of infrastructure for checking constant-time properties.
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 <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
diff --git a/CMakeLists.txt b/CMakeLists.txt
index bfde5d5..1f18782 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -336,6 +336,12 @@
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 ff41989..c812b6b 100644
--- a/crypto/cipher_extra/e_tls.c
+++ b/crypto/cipher_extra/e_tls.c
@@ -299,6 +299,8 @@
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 @@
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 b7998fe..28f1b45 100644
--- a/crypto/fipsmodule/rsa/padding.c
+++ b/crypto/fipsmodule/rsa/padding.c
@@ -233,6 +233,9 @@
// 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 895408d..903ba9a 100644
--- a/crypto/fipsmodule/rsa/rsa_impl.c
+++ b/crypto/fipsmodule/rsa/rsa_impl.c
@@ -120,6 +120,8 @@
return 0;
}
*out = copy;
+ CONSTTIME_SECRET(copy->d, sizeof(BN_ULONG) * width);
+
return 1;
}
@@ -166,6 +168,11 @@
}
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 @@
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 @@
goto err;
}
+ CONSTTIME_DECLASSIFY(out, rsa_size);
*out_len = rsa_size;
ret = 1;
@@ -539,8 +550,11 @@
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 b98b556..52799e8 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -116,6 +116,10 @@
#include <assert.h>
#include <string.h>
+#if defined(BORINGSSL_CONSTANT_TIME_VALIDATION)
+#include <valgrind/memcheck.h>
+#endif
+
#if !defined(__cplusplus)
#if defined(_MSC_VER)
#define alignas(x) __declspec(align(x))
@@ -365,6 +369,26 @@
(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 8b3b942..1572096 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -1226,6 +1226,8 @@
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 @@
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;
