Bug 1350576 - Move padding back after SNI, test it, r=ekr

--HG-- extra : rebase_source : ff2f42211c80284aad9c52d2ad17c7377427c098 extra : amend_source : b76211729dc5fdb7bdda9930849f5600f7735f47
sailfishos-mirror · Mar 24, 2017 · b5e1ad4 · b5e1ad4
1 parent 37dd462
commit b5e1ad4
Show file tree

Hide file tree

Showing 7 changed files with 78 additions and 55 deletions.
diff --git a/gtests/ssl_gtest/ssl_extension_unittest.cc b/gtests/ssl_gtest/ssl_extension_unittest.cc
@@ -1120,6 +1120,25 @@ TEST_P(TlsBogusExtensionTest13, AddBogusExtensionNewSessionTicket) {
   SendReceive();
 }
 
+TEST_P(TlsConnectStream, IncludePadding) {
+  EnsureTlsSetup();
+
+  // This needs to be long enough to push a TLS 1.0 ClientHello over 255, but
+  // short enough not to push a TLS 1.3 ClientHello over 511.
+  static const char* long_name =
+      "chickenchickenchickenchickenchickenchickenchickenchicken."
+      "chickenchickenchickenchickenchickenchickenchickenchicken."
+      "chickenchickenchickenchickenchicken.";
+  SECStatus rv = SSL_SetURL(client_->ssl_fd(), long_name);
+  EXPECT_EQ(SECSuccess, rv);
+
+  auto capture = std::make_shared<TlsExtensionCapture>(ssl_padding_xtn);
+  client_->SetPacketFilter(capture);
+  client_->StartConnect();
+  client_->Handshake();
+  EXPECT_TRUE(capture->captured());
+}
+
 INSTANTIATE_TEST_CASE_P(ExtensionStream, TlsExtensionTestGeneric,
                         ::testing::Combine(TlsConnectTestBase::kTlsModesStream,
                                            TlsConnectTestBase::kTlsVAll));

diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
@@ -4971,7 +4971,6 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
     PRBool isTLS = PR_FALSE;
     PRBool requestingResume = PR_FALSE, fallbackSCSV = PR_FALSE;
     PRInt32 total_exten_len = 0;
-    unsigned paddingExtensionLen;
     unsigned numCompressionMethods;
     PRUint16 version;
     PRInt32 flags;
@@ -5267,19 +5266,12 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
         length += 1 + ss->ssl3.hs.cookie.len;
     }
 
-    /* A padding extension may be included to ensure that the record containing
-     * the ClientHello doesn't have a length between 256 and 511 bytes
-     * (inclusive). Initial, ClientHello records with such lengths trigger bugs
-     * in F5 devices.
-     *
-     * This is not done for DTLS, for renegotiation, or when there are no
-     * extensions. */
-    if (!IS_DTLS(ss) && isTLS && !ss->firstHsDone && total_exten_len) {
-        paddingExtensionLen = ssl3_CalculatePaddingExtensionLength(length);
-        total_exten_len += paddingExtensionLen;
-        length += paddingExtensionLen;
-    } else {
-        paddingExtensionLen = 0;
+    if (total_exten_len > 0) {
+        ssl3_CalculatePaddingExtLen(ss, length);
+        if (ss->xtnData.paddingLen) {
+            total_exten_len += 4 + ss->xtnData.paddingLen;
+            length += 4 + ss->xtnData.paddingLen;
+        }
     }
 
     rv = ssl3_AppendHandshakeHeader(ss, client_hello, length);
@@ -5450,15 +5442,6 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
             return rv; /* err set by AppendHandshake. */
         }
 
-        extLen = ssl3_AppendPaddingExtension(ss, paddingExtensionLen, maxBytes);
-        if (extLen < 0) {
-            if (sid->u.ssl3.lock) {
-                PR_RWLock_Unlock(sid->u.ssl3.lock);
-            }
-            return SECFailure;
-        }
-        maxBytes -= extLen;
-
         extLen = ssl3_CallHelloExtensionSenders(ss, PR_TRUE, maxBytes, NULL);
         if (extLen < 0) {
             if (sid->u.ssl3.lock) {
@@ -11165,9 +11148,7 @@ ssl3_SendNextProto(sslSocket *ss)
 
     padding_len = 32 - ((ss->xtnData.nextProto.len + 2) % 32);
 
-    rv = ssl3_AppendHandshakeHeader(ss, next_proto, ss->xtnData.nextProto.len +
-                                                        2 +
-                                                        padding_len);
+    rv = ssl3_AppendHandshakeHeader(ss, next_proto, ss->xtnData.nextProto.len + 2 + padding_len);
     if (rv != SECSuccess) {
         return rv; /* error code set by AppendHandshakeHeader */
     }

diff --git a/lib/ssl/ssl3ext.c b/lib/ssl/ssl3ext.c
@@ -126,6 +126,7 @@ static const ssl3HelloExtensionSender clientHelloSendersTLS[SSL_MAX_EXTENSIONS]
       { ssl_tls13_cookie_xtn, &tls13_ClientSendHrrCookieXtn },
       { ssl_tls13_psk_key_exchange_modes_xtn,
         &tls13_ClientSendPskKeyExchangeModesXtn },
+      { ssl_padding_xtn, &ssl3_ClientSendPaddingExtension },
       /* The pre_shared_key extension MUST be last. */
       { ssl_tls13_pre_shared_key_xtn, &tls13_ClientSendPreSharedKeyXtn },
       /* any extra entries will appear as { 0, NULL }    */

diff --git a/lib/ssl/ssl3ext.h b/lib/ssl/ssl3ext.h
@@ -54,6 +54,9 @@ struct TLSExtensionDataStr {
     PRUint16 advertised[SSL_MAX_EXTENSIONS];
     PRUint16 negotiated[SSL_MAX_EXTENSIONS];
 
+    /* Amount of padding we need to add. */
+    PRUint16 paddingLen;
+
     /* SessionTicket Extension related data. */
     PRBool ticketTimestampVerified;
     PRBool emptySessionTicket;
@@ -130,9 +133,8 @@ SECStatus ssl3_RegisterExtensionSender(const sslSocket *ss,
 PRInt32 ssl3_CallHelloExtensionSenders(sslSocket *ss, PRBool append, PRUint32 maxBytes,
                                        const ssl3HelloExtensionSender *sender);
 
-unsigned int ssl3_CalculatePaddingExtensionLength(unsigned int clientHelloLength);
-PRInt32 ssl3_AppendPaddingExtension(sslSocket *ss, unsigned int extensionLen,
-                                    PRUint32 maxBytes);
+void ssl3_CalculatePaddingExtLen(sslSocket *ss,
+                                 unsigned int clientHelloLength);
 
 /* Thunks to let us operate on const sslSocket* objects. */
 SECStatus ssl3_ExtAppendHandshake(const sslSocket *ss, const void *void_src,

diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c
@@ -2185,55 +2185,73 @@ ssl3_ClientSendSigAlgsXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRBool
 
 /* Takes the size of the ClientHello, less the record header, and determines how
  * much padding is required. */
-unsigned int
-ssl3_CalculatePaddingExtensionLength(unsigned int clientHelloLength)
+void
+ssl3_CalculatePaddingExtLen(sslSocket *ss,
+                            unsigned int clientHelloLength)
 {
     unsigned int recordLength = 1 /* handshake message type */ +
                                 3 /* handshake message length */ +
                                 clientHelloLength;
-    unsigned int extensionLength;
+    unsigned int extensionLen;
+
+    /* Don't pad for DTLS, for SSLv3, or for renegotiation. */
+    if (IS_DTLS(ss) ||
+        ss->vrange.max < SSL_LIBRARY_VERSION_TLS_1_0 ||
+        ss->firstHsDone) {
+        return;
+    }
 
+    /* A padding extension may be included to ensure that the record containing
+     * 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) {
-        return 0;
+        return;
     }
 
-    extensionLength = 512 - recordLength;
+    extensionLen = 512 - recordLength;
     /* Extensions take at least four bytes to encode. Always include at least
-     * one byte of data if including the extension. Some servers (e.g.
-     * WebSphere Application Server 7.0 and Tomcat) will time out or terminate
-     * the connection if the last extension in the client hello is empty. */
-    if (extensionLength < 4 + 1) {
-        extensionLength = 4 + 1;
+     * one byte of data if we are padding. Some servers will time out or
+     * terminate the connection if the last ClientHello extension is empty. */
+    if (extensionLen < 4 + 1) {
+        extensionLen = 4 + 1;
     }
 
-    return extensionLength;
+    ss->xtnData.paddingLen = extensionLen - 4;
 }
 
-/* ssl3_AppendPaddingExtension possibly adds an extension which ensures that a
+/* ssl3_SendPaddingExtension possibly adds an extension which ensures that a
  * ClientHello record is either < 256 bytes or is >= 512 bytes. This ensures
  * that we don't trigger bugs in F5 products. */
 PRInt32
-ssl3_AppendPaddingExtension(sslSocket *ss, unsigned int extensionLen,
-                            PRUint32 maxBytes)
+ssl3_ClientSendPaddingExtension(const sslSocket *ss, TLSExtensionData *xtnData,
+                                PRBool append, PRUint32 maxBytes)
 {
-    unsigned int paddingLen = extensionLen - 4;
-    static unsigned char padding[252];
+    static unsigned char padding[252] = { 0 };
+    unsigned int extensionLen;
+    SECStatus rv;
 
-    if (extensionLen == 0) {
+    /* On the length-calculation pass, report zero total length.  The record
+     * will be larger on the second pass if needed. */
+    if (!append || !xtnData->paddingLen) {
         return 0;
     }
 
+    extensionLen = xtnData->paddingLen + 4;
     if (extensionLen > maxBytes ||
-        !paddingLen ||
-        paddingLen > sizeof(padding)) {
+        xtnData->paddingLen > sizeof(padding)) {
         PORT_Assert(0);
         return -1;
     }
 
-    if (SECSuccess != ssl3_ExtAppendHandshakeNumber(ss, ssl_padding_xtn, 2))
+    rv = ssl3_ExtAppendHandshakeNumber(ss, ssl_padding_xtn, 2);
+    if (rv != SECSuccess) {
         return -1;
-    if (SECSuccess != ssl3_ExtAppendHandshakeVariable(ss, padding, paddingLen, 2))
+    }
+    rv = ssl3_ExtAppendHandshakeVariable(ss, padding, xtnData->paddingLen, 2);
+    if (rv != SECSuccess) {
         return -1;
+    }
 
     return extensionLen;
 }

diff --git a/lib/ssl/ssl3exthandle.h b/lib/ssl/ssl3exthandle.h
@@ -49,6 +49,9 @@ PRInt32 ssl3_ClientSendSigAlgsXtn(const sslSocket *ss, TLSExtensionData *xtnData
 SECStatus ssl3_ServerHandleSigAlgsXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 ex_type,
                                       SECItem *data);
 
+PRInt32 ssl3_ClientSendPaddingExtension(const sslSocket *ss, TLSExtensionData *xtnData,
+                                        PRBool append, PRUint32 maxBytes);
+
 PRInt32 ssl3_ClientSendSignedCertTimestampXtn(const sslSocket *ss, TLSExtensionData *xtnData,
                                               PRBool append,
                                               PRUint32 maxBytes);

diff --git a/lib/ssl/sslt.h b/lib/ssl/sslt.h
@@ -410,11 +410,10 @@ typedef enum {
 /* This is the old name for the supported_groups extensions. */
 #define ssl_elliptic_curves_xtn ssl_supported_groups_xtn
 
-/* SSL_MAX_EXTENSIONS doesn't include ssl_padding_xtn.  It includes the maximum
- * number of extensions that are supported for any single message type.  That
- * is, a ClientHello; ServerHello and TLS 1.3 NewSessionTicket and
- * HelloRetryRequest extensions are smaller. */
-#define SSL_MAX_EXTENSIONS 19
+/* SSL_MAX_EXTENSIONS includes the maximum number of extensions that are
+ * supported for any single message type.  That is, a ClientHello; ServerHello
+ * and TLS 1.3 NewSessionTicket and HelloRetryRequest extensions have fewer. */
+#define SSL_MAX_EXTENSIONS 20
 
 /* Deprecated */
 typedef enum {