Copilot commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4262112299
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -248,6 +248,24 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO
*client_hello)
return CLIENT_HELLO_SUCCESS;
}
+#if HAVE_SSL_CTX_SET_CLIENT_HELLO_CB
+// OpenSSL's ClientHello API drops extensions it doesn't recognize, including
+// GREASE. Record the extension types from the raw ClientHello message, which
+// this callback receives before ssl_client_hello_callback runs.
+static void
+ssl_msg_callback(int write_p, int /* version ATS_UNUSED */, int content_type,
const void *buf, size_t len, SSL *ssl,
+ void * /* arg ATS_UNUSED */)
+{
+ auto const *msg = static_cast<const uint8_t *>(buf);
+ if (write_p || content_type != SSL3_RT_HANDSHAKE || len == 0 || msg[0] !=
SSL3_MT_CLIENT_HELLO) {
+ return;
+ }
+ if (TLSSNISupport *snis = TLSSNISupport::getInstance(ssl); snis != nullptr) {
+ snis->on_client_hello_message(msg, len);
+ }
Review Comment:
This assumes the msg callback receives a complete ClientHello handshake
message starting at `SSL3_MT_CLIENT_HELLO`. In OpenSSL,
`SSL_CTX_set_msg_callback` is commonly invoked with TLS record payloads (which
may contain partial handshake messages, a handshake message split across
records, or multiple handshake messages in one record). In those cases
`on_client_hello_message(msg, len)` will frequently fail to parse (and
permanently lose the ClientHello types), producing incorrect fingerprints and
incrementing `client_hello_raw_parse_failure` for valid handshakes.\n\nFix:
parse the handshake framing robustly in the callback by (a) iterating over all
handshake messages in the provided buffer (type + 3-byte length + body), and
(b) buffering across callbacks when a ClientHello is fragmented. A simple
approach is to store an in-progress buffer per connection (e.g., on
TLSSNISupport) until the full ClientHello length is available, then parse and
optionally disable further msg callbacks fo
r that SSL once captured.
##########
src/iocore/net/TLSSNISupport.cc:
##########
@@ -138,6 +142,84 @@ TLSSNISupport::on_client_hello(ClientHello &client_hello)
}
}
+void
+TLSSNISupport::on_client_hello_message(const uint8_t *msg, size_t len)
+{
+ _has_raw_extension_types = parse_client_hello_extension_types(msg, len,
_raw_extension_types);
+ if (!_has_raw_extension_types) {
+ Metrics::Counter::increment(ssl_rsb.client_hello_raw_parse_failure);
+ Dbg(dbg_ctl_ssl_sni, "Could not parse the extensions of the raw
ClientHello");
+ }
+}
+
+bool
+TLSSNISupport::parse_client_hello_extension_types(const uint8_t *msg, size_t
msg_len, std::vector<int> &types)
+{
+ types.clear();
+
+ size_t offset = 0;
+ size_t end = 0;
+
+ auto read_u8 = [&](size_t &value) {
+ if (offset + 1 > end) {
+ return false;
+ }
+ value = msg[offset];
+ offset += 1;
+ return true;
+ };
+ auto read_u16 = [&](size_t &value) {
+ if (offset + 2 > end) {
+ return false;
+ }
+ value = (msg[offset] << 8) | msg[offset + 1];
+ offset += 2;
+ return true;
+ };
+ auto skip = [&](size_t n) {
+ if (offset + n > end) {
+ return false;
+ }
+ offset += n;
+ return true;
+ };
+
+ // Handshake header: msg_type(1) length(3).
+ if (msg_len < 4 || msg[0] != SSL3_MT_CLIENT_HELLO) {
+ return false;
+ }
+ size_t const body_len = (msg[1] << 16) | (msg[2] << 8) | msg[3];
+ if (4 + body_len > msg_len) {
+ return false;
+ }
+ offset = 4;
+ end = 4 + body_len;
+
+ // legacy_version(2) random(32) session_id<0..32> cipher_suites<2..2^16-2>
compression_methods<1..2^8-1>
+ size_t len = 0;
+ if (!skip(2 + 32) || !read_u8(len) || !skip(len) || !read_u16(len) ||
!skip(len) || !read_u8(len) || !skip(len)) {
+ return false;
+ }
+
Review Comment:
The parser claims to accept only a “well formed ClientHello” (per
header/docs), but it currently accepts several structurally invalid values as
successful parses, which can lead to returning extension types for malformed
ClientHellos (and potentially masking real parse failures). Concrete gaps
here:\n- `session_id` length is not validated to be <= 32.\n- `cipher_suites`
length is not validated to be >= 2 and an even number of bytes.\n-
`compression_methods` length is not validated to be >= 1.\n\nFix: add explicit
checks for these TLS ClientHello invariants before `skip(len)` calls. Also add
unit tests covering these malformed-length cases (e.g., odd cipher_suites
length, compression_methods length 0, session_id length 33) to ensure
`parse_client_hello_extension_types()` returns false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
