summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorKevin Jacobs <kjacobs@mozilla.com>2021-02-04 16:15:29 +0000
committerKevin Jacobs <kjacobs@mozilla.com>2021-02-04 16:15:29 +0000
commit4abb071b49596dfc2210664b7f7159c3a3da4444 (patch)
tree0c5d6495ab8f3c55cc3f7f35e8409387790b9a04
parent63b785f8240d01a5be76b2fe98770764c3843b7a (diff)
downloadnss-hg-4abb071b49596dfc2210664b7f7159c3a3da4444.tar.gz
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
-rw-r--r--gtests/ssl_gtest/ssl_recordsize_unittest.cc21
-rw-r--r--lib/ssl/ssl3ext.c7
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 8926b5551..a18510440 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<TlsHandshakeRecorder>(client_, kTlsHandshakeClientHello);
+
+ // Add PSK with label long enough to push CH length into [256, 511].
+ std::vector<uint8_t> 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 199cf459a..cdbb803fa 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. */