From 1b6a8571fb86328f263d903705f770f0dfd274ab Mon Sep 17 00:00:00 2001 From: Kevin Jacobs Date: Thu, 4 Feb 2021 16:15:29 +0000 Subject: [PATCH] Bug 1690583 - Fix CH padding extension size calculation. r=mt Bug 1654332 changed the way that NSS constructs Client Hello messages. `ssl_CalculatePaddingExtLen` now receives a `clientHelloLength` value that includes the 4B handshake header. This looks okay per the inline comment (which states that only the record header is omitted from the length), but the function actually assumes that the handshake header is also omitted. This patch removes the addition of the handshake header length. Those bytes are already included in the buffered CH. Differential Revision: https://phabricator.services.mozilla.com/D103934 --HG-- extra : moz-landing-system : lando --- gtests/ssl_gtest/ssl_recordsize_unittest.cc | 21 +++++++++++++++++++++ lib/ssl/ssl3ext.c | 7 ++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/gtests/ssl_gtest/ssl_recordsize_unittest.cc b/gtests/ssl_gtest/ssl_recordsize_unittest.cc index 8926b5551e..a185104404 100644 --- a/gtests/ssl_gtest/ssl_recordsize_unittest.cc +++ b/gtests/ssl_gtest/ssl_recordsize_unittest.cc @@ -266,6 +266,27 @@ TEST_P(TlsConnectTls13, RecordSizeCiphertextExceed) { server_->CheckErrorCode(SSL_ERROR_RECORD_OVERFLOW_ALERT); } +TEST_F(TlsConnectStreamTls13, ClientHelloF5Padding) { + EnsureTlsSetup(); + ScopedPK11SlotInfo slot(PK11_GetInternalSlot()); + ScopedPK11SymKey key( + PK11_KeyGen(slot.get(), CKM_NSS_CHACHA20_POLY1305, nullptr, 32, nullptr)); + + auto filter = + MakeTlsFilter(client_, kTlsHandshakeClientHello); + + // Add PSK with label long enough to push CH length into [256, 511]. + std::vector label(100); + EXPECT_EQ(SECSuccess, + SSL_AddExternalPsk(client_->ssl_fd(), key.get(), label.data(), + label.size(), ssl_hash_sha256)); + StartConnect(); + client_->Handshake(); + + // Filter removes the 4B handshake header. + EXPECT_EQ(508UL, filter->buffer().len()); +} + // This indiscriminately adds padding to application data records. class TlsRecordPadder : public TlsRecordFilter { public: diff --git a/lib/ssl/ssl3ext.c b/lib/ssl/ssl3ext.c index 199cf459ae..cdbb803fab 100644 --- a/lib/ssl/ssl3ext.c +++ b/lib/ssl/ssl3ext.c @@ -837,9 +837,6 @@ ssl_SendEmptyExtension(const sslSocket *ss, TLSExtensionData *xtnData, static unsigned int ssl_CalculatePaddingExtLen(const sslSocket *ss, unsigned int clientHelloLength) { - unsigned int recordLength = 1 /* handshake message type */ + - 3 /* handshake message length */ + - clientHelloLength; unsigned int extensionLen; /* Don't pad for DTLS, for SSLv3, or for renegotiation. */ @@ -853,11 +850,11 @@ ssl_CalculatePaddingExtLen(const sslSocket *ss, unsigned int clientHelloLength) * the ClientHello doesn't have a length between 256 and 511 bytes * (inclusive). Initial ClientHello records with such lengths trigger bugs * in F5 devices. */ - if (recordLength < 256 || recordLength >= 512) { + if (clientHelloLength < 256 || clientHelloLength >= 512) { return 0; } - extensionLen = 512 - recordLength; + extensionLen = 512 - clientHelloLength; /* Extensions take at least four bytes to encode. Always include at least * one byte of data if we are padding. Some servers will time out or * terminate the connection if the last ClientHello extension is empty. */