Remove the old F5 padding workaround Many, many, many eons ago, SSL 2.0 was born. Many, many eons ago, SSL 3.0 was born. SSL 3.0 was a completely different record layer from SSL 3.0. Since then, all versions of TLS kept continuity with SSL 3.0 in the record layer and ClientHello. Many eons ago, F5 shipped a TLS load balancer that supported SSL 2.0 and SSL 3.0. As part of that, they had to detect SSL 2.0 and SSL 3.0. Eons ago, ClientHellos started hanging when talking to F5 servers. It turned out that, once we made the TLS ClientHello exceed 256 bytes for ALPN, F5 misinterpeted it as an incomplete SSL 2.0 CLIENT-HELLO and hung. https://mailarchive.ietf.org/arch/msg/tls/8wXwhM1d5WSmROHFSgrTyFmWN2o/ https://www.imperialviolet.org/2013/10/07/f5update.html It turned out bumping the size up to 512 bytes would dodge the bug, so the padding extension was born: https://www.rfc-editor.org/info/rfc7685/ It has now been almost 10 years. In that time, ClientHellos have gotten larger. With X25519MLKEM768 enabled by default, every ClientHello over well over 1 KiB and the padding is a no-op. Also the F5s have hopefully gotten updated. Remove the padding, which requires some somewhat subtle bookkeeping. In particular, we usually qualify changes with Chromium but as Chromium has long enabled ML-KEM, we have gotten all the signal we ever will get about F5. Update-Note: If there are still unupdated F5s out there, a TLS client application may hang when connecting to them. This would only happen if your application: 1. Bypasses BoringSSL's default of enabling TLS 1.3, or 2. Bypasses BoringSSL's default of enabling X25519MLKEM768 Impacted applications should enable TLS 1.3 and X25519MLKEM768 for better compatibility and security. Applications using BoringSSLs defaults are not impacted by this change. Fixed: 532111493 Change-Id: I78fd1d65d9fa7c25347fb56803acd6137f038eae Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/101007 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Lily Chen <chlily@google.com> Commit-Queue: David Benjamin <davidben@google.com> Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/ssl/extensions.cc b/ssl/extensions.cc index 90238d9..954af18 100644 --- a/ssl/extensions.cc +++ b/ssl/extensions.cc
@@ -1977,25 +1977,6 @@ type != ssl_client_hello_outer; } -static size_t ext_pre_shared_key_clienthello_length( - const SSL_HANDSHAKE *hs, ssl_client_hello_type_t type) { - if (!should_offer_psk(hs, type)) { - return 0; - } - - // extension value, extension length, identities length, binders length. - size_t ret = 2 + 2 + 2 + 2; - for (const auto &psk : hs->pre_shared_keys) { - // identity - ret += 2 + ssl_pre_shared_key_identity(psk).size(); - // obfuscated_ticket_age - ret += 4; - // binder - ret += 1 + EVP_MD_size(ssl_pre_shared_key_hash(psk)); - } - return ret; -} - // ext_pre_shared_key_add_clienthello writes a pre_shared_key extension to // `out_extensions` and flushes `out_client_hello`, invalidating // `out_extensions`. `out_extensions` must be a child of `out_client_hello`. @@ -4509,9 +4490,6 @@ return ssl_add_clienthello_tlsext_inner(hs, out, out_encoded); } - // Sample the length of the ClientHello thus far, including the message - // header. - size_t msg_len = SSL3_HM_HEADER_LENGTH + CBB_len(out); assert(out_encoded == nullptr); // Only ClientHelloInner needs two outputs. SSLImpl *const ssl = hs->ssl; CBB extensions; @@ -4562,61 +4540,24 @@ last_was_empty = false; } - // In cleartext ClientHellos, we add the padding extension to work around - // bugs. We also apply this padding to ClientHelloOuter, to keep the wire - // images aligned. - size_t psk_len = ext_pre_shared_key_clienthello_length(hs, type); - if (!SSL_is_dtls(ssl) && !SSL_is_quic(ssl) && - !ssl->s3->used_hello_retry_request) { - msg_len += 2 /* length prefix */ + CBB_len(&extensions) + psk_len; - // The length of the padding extension, excluding the four-byte extension - // header. - size_t padding_len = 0; - - // The final extension must be non-empty. WebSphere Application - // Server 7.0 is intolerant to the last extension being zero-length. See - // https://crbug.com/363583. - if (last_was_empty && psk_len == 0) { - padding_len = 1; - // The addition of the padding extension may push us into the F5 bug. - msg_len += 4 + padding_len; - } - - // Add padding to workaround bugs in F5 terminators. See RFC 7685. - // - // NB: because this code works out the length of all existing extensions - // it MUST always appear last (save for any PSK extension). - if (msg_len > 0xff && msg_len < 0x200) { - // If our calculations already included a padding extension, remove that - // factor because we're about to change its length. - if (padding_len != 0) { - msg_len -= 4 + padding_len; - } - padding_len = 0x200 - msg_len; - // Extensions take at least four bytes to encode. WebSphere Application - // Server 7.0 is intolerant to the last extension being zero-length, so - // always include at least one byte of data if including the extension. - // See https://crbug.com/363583. - if (padding_len >= 4 + 1) { - padding_len -= 4; - } else { - padding_len = 1; - } - } - - if (padding_len != 0 && - !add_padding_extension(&extensions, TLSEXT_TYPE_padding, padding_len)) { + // The final extension must be non-empty. WebSphere Application Server 7.0 is + // intolerant to the last extension being zero-length. See + // https://crbug.com/363583. + bool offering_psk = should_offer_psk(hs, type); + if (!offering_psk && last_was_empty && !SSL_is_dtls(ssl) && + !SSL_is_quic(ssl) && !ssl->s3->used_hello_retry_request) { + if (!add_padding_extension(&extensions, TLSEXT_TYPE_padding, 1)) { return false; } } // The PSK extension must be last, including after the padding. - size_t psk_len_actual; - if (!ext_pre_shared_key_add_clienthello(hs, out, &extensions, &psk_len_actual, + size_t psk_len; + if (!ext_pre_shared_key_add_clienthello(hs, out, &extensions, &psk_len, type)) { return false; } - assert(psk_len_actual == psk_len); + assert(offering_psk == (psk_len != 0)); return true; }
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index fc185ec..242c738 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -1605,33 +1605,6 @@ } } -// CreateSessionWithTicket returns a sample `SSL_SESSION` with the specified -// version and ticket length or nullptr on failure. -static bssl::UniquePtr<SSL_SESSION> CreateSessionWithTicket(uint16_t version, - size_t ticket_len) { - std::vector<uint8_t> der; - if (!DecodeBase64(&der, kOpenSSLSession)) { - return nullptr; - } - - bssl::UniquePtr<SSL_CTX> ssl_ctx(SSL_CTX_new(TLS_method())); - if (!ssl_ctx) { - return nullptr; - } - // Use a garbage ticket. - std::vector<uint8_t> ticket(ticket_len, 'a'); - bssl::UniquePtr<SSL_SESSION> session( - SSL_SESSION_from_bytes(der.data(), der.size(), ssl_ctx.get())); - if (!session || // - !SSL_SESSION_set_protocol_version(session.get(), version) || // - !SSL_SESSION_set_ticket(session.get(), ticket.data(), ticket.size())) { - return nullptr; - } - // Fix up the timeout. - SSL_SESSION_set_time(session.get(), time(nullptr)); - return session; -} - static bool GetClientHello(SSL *ssl, std::vector<uint8_t> *out) { bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem())); if (!bio) { @@ -1662,105 +1635,6 @@ return true; } -// GetClientHelloLen creates a client SSL connection with the specified version -// and ticket length. It returns the length of the ClientHello, not including -// the record header, on success and zero on error. -static size_t GetClientHelloLen(uint16_t max_version, uint16_t session_version, - size_t ticket_len) { - bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method())); - - // Reduce the number of supported groups, as we need ClientHellos smaller - // than 254 bytes for SSLTest.Padding. - uint16_t groups[] = {SSL_GROUP_X25519, SSL_GROUP_SECP256R1, - SSL_GROUP_SECP384R1}; - SSL_CTX_set1_group_ids(ctx.get(), groups, sizeof(groups) / sizeof(*groups)); - - bssl::UniquePtr<SSL_SESSION> session = - CreateSessionWithTicket(session_version, ticket_len); - if (!ctx || !session) { - return 0; - } - - // Set a one-element cipher list so the baseline ClientHello is unpadded. - bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get())); - if (!ssl || !SSL_set_session(ssl.get(), session.get()) || - !SSL_set_strict_cipher_list(ssl.get(), "ECDHE-RSA-AES128-GCM-SHA256") || - !SSL_set_max_proto_version(ssl.get(), max_version)) { - return 0; - } - - std::vector<uint8_t> client_hello; - if (!GetClientHello(ssl.get(), &client_hello) || - client_hello.size() <= SSL3_RT_HEADER_LENGTH) { - return 0; - } - - return client_hello.size() - SSL3_RT_HEADER_LENGTH; -} - -TEST(SSLTest, Padding) { - struct PaddingVersions { - uint16_t max_version, session_version; - }; - static const PaddingVersions kPaddingVersions[] = { - // Test the padding extension at TLS 1.2. - {TLS1_2_VERSION, TLS1_2_VERSION}, - // Test the padding extension at TLS 1.3 with a TLS 1.2 session, so there - // will be no PSK binder after the padding extension. - {TLS1_3_VERSION, TLS1_2_VERSION}, - // Test the padding extension at TLS 1.3 with a TLS 1.3 session, so there - // will be a PSK binder after the padding extension. - {TLS1_3_VERSION, TLS1_3_VERSION}, - - }; - - struct PaddingTest { - size_t input_len, padded_len; - }; - static const PaddingTest kPaddingTests[] = { - // ClientHellos of length below 0x100 do not require padding. - {0xfe, 0xfe}, - {0xff, 0xff}, - // ClientHellos of length 0x100 through 0x1fb are padded up to 0x200. - {0x100, 0x200}, - {0x123, 0x200}, - {0x1fb, 0x200}, - // ClientHellos of length 0x1fc through 0x1ff get padded beyond 0x200. The - // padding extension takes a minimum of four bytes plus one required - // content - // byte. (To work around yet more server bugs, we avoid empty final - // extensions.) - {0x1fc, 0x201}, - {0x1fd, 0x202}, - {0x1fe, 0x203}, - {0x1ff, 0x204}, - // Finally, larger ClientHellos need no padding. - {0x200, 0x200}, - {0x201, 0x201}, - }; - - for (const PaddingVersions &versions : kPaddingVersions) { - SCOPED_TRACE(versions.max_version); - SCOPED_TRACE(versions.session_version); - - // Sample a baseline length. - size_t base_len = - GetClientHelloLen(versions.max_version, versions.session_version, 1); - ASSERT_NE(base_len, 0u) << "Baseline length could not be sampled"; - - for (const PaddingTest &test : kPaddingTests) { - SCOPED_TRACE(test.input_len); - ASSERT_LE(base_len, test.input_len) << "Baseline ClientHello too long"; - - size_t padded_len = - GetClientHelloLen(versions.max_version, versions.session_version, - 1 + test.input_len - base_len); - EXPECT_EQ(padded_len, test.padded_len) - << "ClientHello was not padded to expected length"; - } - } -} - static bssl::UniquePtr<X509> CertFromPEM(const char *pem) { bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(pem, strlen(pem))); if (!bio) {
diff --git a/ssl/test/runner/extension_tests.go b/ssl/test/runner/extension_tests.go index 6c520c8..6022b19 100644 --- a/ssl/test/runner/extension_tests.go +++ b/ssl/test/runner/extension_tests.go
@@ -16,7 +16,6 @@ import ( "fmt" - "strconv" ) func addExtensionTests() { @@ -1991,23 +1990,6 @@ } } - testCases = append(testCases, testCase{ - testType: clientTest, - name: "ClientHelloPadding", - config: Config{ - Bugs: ProtocolBugs{ - RequireClientHelloSize: 512, - }, - }, - flags: []string{ - // This hostname just needs to be long enough to push the - // ClientHello into F5's danger zone between 256 and 511 bytes long. - "-host-name", "01234567890123456789012345678901234567890123456789012345678901234567890123456789.com", - // Curve chosen to make handshakes short enough to end up in the danger zone. - "-curves", strconv.Itoa(int(CurveX25519)), - }, - }) - // Test that illegal extensions in TLS 1.3 are rejected by the client if // in ServerHello. testCases = append(testCases, testCase{