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

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. */