bneradt commented on code in PR #13809:
URL: https://github.com/apache/trafficserver/pull/13809#discussion_r4262205046
##########
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:
Fixed in 574706f: the parser now rejects a legacy session id longer than 32
bytes, an empty or odd-length cipher suite list, and an empty compression
method list. Each case has a unit test.
##########
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 doesn't apply to handshake messages. For `SSL3_RT_HANDSHAKE`, OpenSSL
calls the message callback once per complete, reassembled handshake message
(from `tls_get_message_body()`), not per record payload. Record-level calls use
`SSL3_RT_HEADER`, which the callback ignores. To confirm, 574706f adds an
autest case that sends the ClientHello split across 100-byte records: it
produces the same JA4 as the single-record ClientHello and leaves
`proxy.process.ssl.client_hello_raw_parse_failure` at 0.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -1025,6 +1043,10 @@ ssl_callback_info(const SSL *ssl, int where, int ret)
}
if (where & SSL_CB_HANDSHAKE_DONE) {
// handshake is complete
+#if HAVE_SSL_CTX_SET_CLIENT_HELLO_CB
+ // The raw ClientHello has been recorded. Stop the per record message
callbacks.
+ SSL_set_msg_callback(const_cast<SSL *>(ssl), nullptr);
Review Comment:
Per maskit's review, we're not handling TLS 1.2 client renegotiation here:
the plugin API doesn't support it, HTTP/2 forbids it, and TLS 1.3 removed it.
--
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]
