commit 44a389a7fce31013b5953038d4231f33cbf2ba9d
author David Benjamin <davidben@google.com> Sat Dec 10 00:46:54 2022 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Apr 07 06:50:36 2023 +0000
tree 503c682f40f0e58a3400fad198315079d0e017f7
parent e8b168dffef7c423ade6b5d21953dfed454a80b9

Tidy up some lengths in SSL_SESSION

Normally these would be size_t, but we try to reduce per-connection
memory in libssl, so use uint8_t, then add asserts, checks, and casts as
appropriate.

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

ssl/handshake_client.cc

diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index e7dca1b..3e63019 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -833,11 +833,18 @@
       ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
       return ssl_hs_error;
     }
-    // Note: session_id could be empty.
-    hs->new_session->session_id_length = CBS_len(&server_hello.session_id);
+
+    // Save the session ID from the server. This may be empty if the session
+    // isn't resumable, or if we'll receive a session ticket later.
+    assert(CBS_len(&server_hello.session_id) <= SSL3_SESSION_ID_SIZE);
+    static_assert(SSL3_SESSION_ID_SIZE <= UINT8_MAX,
+                  "max session ID is too large");
+    hs->new_session->session_id_length =
+        static_cast<uint8_t>(CBS_len(&server_hello.session_id));
     OPENSSL_memcpy(hs->new_session->session_id,
                    CBS_data(&server_hello.session_id),
                    CBS_len(&server_hello.session_id));
+
     hs->new_session->cipher = hs->new_cipher;
   }
 

ssl/internal.h

diff --git a/ssl/internal.h b/ssl/internal.h
index 1b53023..b4d853a 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -3838,11 +3838,11 @@
   // session. In TLS 1.3 and up, it is the resumption PSK for sessions handed to
   // the caller, but it stores the resumption secret when stored on |SSL|
   // objects.
-  int secret_length = 0;
+  uint8_t secret_length = 0;
   uint8_t secret[SSL_MAX_MASTER_KEY_LENGTH] = {0};
 
   // session_id - valid?
-  unsigned session_id_length = 0;
+  uint8_t session_id_length = 0;
   uint8_t session_id[SSL_MAX_SSL_SESSION_ID_LENGTH] = {0};
   // this is used to determine whether the session is being reused in
   // the appropriate context. It is up to the application to set this,

ssl/ssl_asn1.cc

diff --git a/ssl/ssl_asn1.cc b/ssl/ssl_asn1.cc
index 96cb795..3311246 100644
--- a/ssl/ssl_asn1.cc
+++ b/ssl/ssl_asn1.cc
@@ -486,7 +486,7 @@
     return 0;
   }
   OPENSSL_memcpy(out, CBS_data(&value), CBS_len(&value));
-  *out_len = (uint8_t)CBS_len(&value);
+  *out_len = static_cast<uint8_t>(CBS_len(&value));
   return 1;
 }
 
@@ -578,9 +578,13 @@
     return nullptr;
   }
   OPENSSL_memcpy(ret->session_id, CBS_data(&session_id), CBS_len(&session_id));
-  ret->session_id_length = CBS_len(&session_id);
+  static_assert(SSL3_MAX_SSL_SESSION_ID_LENGTH <= UINT8_MAX,
+                "max session ID is too large");
+  ret->session_id_length = static_cast<uint8_t>(CBS_len(&session_id));
   OPENSSL_memcpy(ret->secret, CBS_data(&secret), CBS_len(&secret));
-  ret->secret_length = CBS_len(&secret);
+  static_assert(SSL_MAX_MASTER_KEY_LENGTH <= UINT8_MAX,
+                "max secret is too large");
+  ret->secret_length = static_cast<uint8_t>(CBS_len(&secret));
 
   CBS child;
   uint64_t timeout;

ssl/tls13_both.cc

diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc
index f67c592..5ab5a1c 100644
--- a/ssl/tls13_both.cc
+++ b/ssl/tls13_both.cc
@@ -513,7 +513,8 @@
   if (!ssl->method->init_message(ssl, cbb.get(), body,
                                  SSL3_MT_COMPRESSED_CERTIFICATE) ||
       !CBB_add_u16(body, hs->cert_compression_alg_id) ||
-      !CBB_add_u24(body, msg.size()) ||
+      msg.size() > (1u << 24) - 1 ||  //
+      !CBB_add_u24(body, static_cast<uint32_t>(msg.size())) ||
       !CBB_add_u24_length_prefixed(body, &compressed)) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
     return false;