commit ad36a4fc4a825b7874fde30c95a196b71ae044a8
author David Benjamin <davidben@google.com> Sat Apr 13 17:40:13 2024 -0400
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Apr 22 15:46:08 2024 +0000
tree c702378349eeee6622e851f21ef8b96341950890
parent 0aa300b9ba9d66b914793ad18c5b469163e58905

Make SSL_CTX_set_keylog_callback constant time

We encode the secrets in hex. When we do so, we should not leak them
based on memory access patterns. Of course, the caller is presumably
going to leak them anyway, because this is the SSLKEYLOGFILE callback.

But it's plausible that the caller might have registered the callback
unconditionally and then, in the callback, decide whether to discard the
data. In that case, we should not introduce a side channel.

Change-Id: If6358a3081c658038232b4610603967cb38659b4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67829
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

include/openssl/ssl.h

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 97f1c89..04c191f 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4376,8 +4376,17 @@
 //
 // The format is described in
 // https://www.ietf.org/archive/id/draft-ietf-tls-keylogfile-01.html
-OPENSSL_EXPORT void SSL_CTX_set_keylog_callback(
-    SSL_CTX *ctx, void (*cb)(const SSL *ssl, const char *line));
+//
+// WARNING: The data in |line| allows an attacker to break security properties
+// of the TLS protocol, including confidentiality, integrity, and forward
+// secrecy. This impacts both the current connection, and, in TLS 1.2, future
+// connections that resume a session from it. Both direct access to the data and
+// side channel leaks from application code are possible attack vectors. This
+// callback is intended for debugging and should not be used in production
+// connections.
+OPENSSL_EXPORT void SSL_CTX_set_keylog_callback(SSL_CTX *ctx,
+                                                void (*cb)(const SSL *ssl,
+                                                           const char *line));
 
 // SSL_CTX_get_keylog_callback returns the callback configured by
 // |SSL_CTX_set_keylog_callback|.

ssl/ssl_lib.cc

diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 98f97eb..ec0ee89 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -279,17 +279,21 @@
   return ret;
 }
 
-static bool cbb_add_hex(CBB *cbb, Span<const uint8_t> in) {
-  static const char hextable[] = "0123456789abcdef";
-  uint8_t *out;
+static uint8_t hex_char_consttime(uint8_t b) {
+  declassify_assert(b < 16);
+  return constant_time_select_8(constant_time_lt_8(b, 10), b + '0',
+                                b - 10 + 'a');
+}
 
-  if (!CBB_add_space(cbb, &out, in.size() * 2)) {
+static bool cbb_add_hex_consttime(CBB *cbb, Span<const uint8_t> in) {
+  uint8_t *out;
+if (!CBB_add_space(cbb, &out, in.size() * 2)) {
     return false;
   }
 
   for (uint8_t b : in) {
-    *(out++) = (uint8_t)hextable[b >> 4];
-    *(out++) = (uint8_t)hextable[b & 0xf];
+    *(out++) = hex_char_consttime(b >> 4);
+    *(out++) = hex_char_consttime(b & 0xf);
   }
 
   return true;
@@ -308,9 +312,11 @@
       !CBB_add_bytes(cbb.get(), reinterpret_cast<const uint8_t *>(label),
                      strlen(label)) ||
       !CBB_add_u8(cbb.get(), ' ') ||
-      !cbb_add_hex(cbb.get(), ssl->s3->client_random) ||
+      !cbb_add_hex_consttime(cbb.get(), ssl->s3->client_random) ||
       !CBB_add_u8(cbb.get(), ' ') ||
-      !cbb_add_hex(cbb.get(), secret) ||
+      // Convert to hex in constant time to avoid leaking |secret|. If the
+      // callback discards the data, we should not introduce side channels.
+      !cbb_add_hex_consttime(cbb.get(), secret) ||
       !CBB_add_u8(cbb.get(), 0 /* NUL */) ||
       !CBBFinishArray(cbb.get(), &line)) {
     return false;