Bug 1306869 - Support cookie extension in TLS 1.3, r=ekr

--HG--
extra : rebase_source : eeca8ab30217f5be67dbc3692ea1100e0e6f1654
extra : amend_source : dcf9ba67f47af15acbd0023fc9a0b786a32851b1

external_tests/ssl_gtest/ssl_hrr_unittest.cc

@@ -183,59 +183,95 @@ TEST_F(TlsConnectTest, Select12AfterHelloRetryRequest) {
 
 class HelloRetryRequestAgentTest : public TlsAgentTestClient {
  protected:
-  void MakeHelloRetryRequestRecord(SSLNamedGroup group, DataBuffer* hrr_record,
-                                   uint32_t seq_num = 0) const {
-    const uint8_t canned_hrr[] = {
-        SSL_LIBRARY_VERSION_TLS_1_3 >> 8,
-        SSL_LIBRARY_VERSION_TLS_1_3 & 0xff,
-        0,
-        6,  // length of extensions
+  void SetUp() override {
+    TlsAgentTestClient::SetUp();
+    EnsureInit();
+    agent_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
+                            SSL_LIBRARY_VERSION_TLS_1_3);
+    agent_->StartConnect();
+  }
+
+  void MakeCannedHrr(const uint8_t* body, size_t len, DataBuffer* hrr_record,
+                     uint32_t seq_num = 0) const {
+    DataBuffer hrr_data;
+    hrr_data.Allocate(len + 4);
+    size_t i = 0;
+    i = hrr_data.Write(i, static_cast<uint32_t>(SSL_LIBRARY_VERSION_TLS_1_3),
+                       2);
+    i = hrr_data.Write(i, static_cast<uint32_t>(len), 2);
+    if (len) {
+      hrr_data.Write(i, body, len);
+    }
+    DataBuffer hrr;
+    MakeHandshakeMessage(kTlsHandshakeHelloRetryRequest, hrr_data.data(),
+                         hrr_data.len(), &hrr, seq_num);
+    MakeRecord(kTlsHandshakeType, SSL_LIBRARY_VERSION_TLS_1_3, hrr.data(),
+               hrr.len(), hrr_record, seq_num);
+  }
+
+  void MakeGroupHrr(SSLNamedGroup group, DataBuffer* hrr_record,
+                    uint32_t seq_num = 0) const {
+    const uint8_t group_hrr[] = {
         static_cast<uint8_t>(ssl_tls13_key_share_xtn >> 8),
         static_cast<uint8_t>(ssl_tls13_key_share_xtn),
         0,
         2,  // length of key share extension
         static_cast<uint8_t>(group >> 8),
-        static_cast<uint8_t>(group)
-    };
-    DataBuffer hrr;
-    MakeHandshakeMessage(kTlsHandshakeHelloRetryRequest, canned_hrr,
-                         sizeof(canned_hrr), &hrr, seq_num);
-    MakeRecord(kTlsHandshakeType, SSL_LIBRARY_VERSION_TLS_1_3, hrr.data(),
-               hrr.len(), hrr_record, seq_num);
+        static_cast<uint8_t>(group)};
+    MakeCannedHrr(group_hrr, sizeof(group_hrr), hrr_record, seq_num);
   }
 };
 
 // Send two HelloRetryRequest messages in response to the ClientHello.  The are
 // constructed to appear legitimate by asking for a new share in each, so that
 // the client has to count to work out that the server is being unreasonable.
 TEST_P(HelloRetryRequestAgentTest, SendSecondHelloRetryRequest) {
-  EnsureInit();
-  agent_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
-                          SSL_LIBRARY_VERSION_TLS_1_3);
-  agent_->StartConnect();
-
-  DataBuffer hrr_record;
-  MakeHelloRetryRequestRecord(ssl_grp_ec_secp384r1, &hrr_record, 0);
-  ProcessMessage(hrr_record, TlsAgent::STATE_CONNECTING);
-  MakeHelloRetryRequestRecord(ssl_grp_ec_secp521r1, &hrr_record, 1);
-  ProcessMessage(hrr_record, TlsAgent::STATE_ERROR,
+  DataBuffer hrr;
+  MakeGroupHrr(ssl_grp_ec_secp384r1, &hrr, 0);
+  ProcessMessage(hrr, TlsAgent::STATE_CONNECTING);
+  MakeGroupHrr(ssl_grp_ec_secp521r1, &hrr, 1);
+  ProcessMessage(hrr, TlsAgent::STATE_ERROR,
                  SSL_ERROR_RX_UNEXPECTED_HELLO_RETRY_REQUEST);
 }
 
 // Here the client receives a HelloRetryRequest with a group that they already
 // provided a share for.
 TEST_P(HelloRetryRequestAgentTest, HandleBogusHelloRetryRequest) {
-  EnsureInit();
-  agent_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
-                          SSL_LIBRARY_VERSION_TLS_1_3);
-  agent_->StartConnect();
-
-  DataBuffer hrr_record;
-  MakeHelloRetryRequestRecord(ssl_grp_ec_secp256r1, &hrr_record);
-  ProcessMessage(hrr_record, TlsAgent::STATE_ERROR,
+  DataBuffer hrr;
+  MakeGroupHrr(ssl_grp_ec_secp256r1, &hrr);
+  ProcessMessage(hrr, TlsAgent::STATE_ERROR,
+                 SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST);
+}
+
+TEST_P(HelloRetryRequestAgentTest, HandleNoopHelloRetryRequest) {
+  DataBuffer hrr;
+  MakeCannedHrr(nullptr, 0U, &hrr);
+  ProcessMessage(hrr, TlsAgent::STATE_ERROR,
                  SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST);
 }
 
+TEST_P(HelloRetryRequestAgentTest, HandleHelloRetryRequestCookie) {
+  const uint8_t canned_cookie_hrr[] = {
+      static_cast<uint8_t>(ssl_tls13_cookie_xtn >> 8),
+      static_cast<uint8_t>(ssl_tls13_cookie_xtn),
+      0,
+      5,  // length of cookie extension
+      0,
+      3,  // cookie value length
+      0xc0,
+      0x0c,
+      0x13};
+  DataBuffer hrr;
+  MakeCannedHrr(canned_cookie_hrr, sizeof(canned_cookie_hrr), &hrr);
+  TlsExtensionCapture* capture = new TlsExtensionCapture(ssl_tls13_cookie_xtn);
+  agent_->SetPacketFilter(capture);
+  ProcessMessage(hrr, TlsAgent::STATE_CONNECTING);
+  const size_t cookie_pos = 2 + 2;  // cookie_xtn, extension len
+  DataBuffer cookie(canned_cookie_hrr + cookie_pos,
+                    sizeof(canned_cookie_hrr) - cookie_pos);
+  EXPECT_EQ(cookie, capture->extension());
+}
+
 INSTANTIATE_TEST_CASE_P(HelloRetryRequestAgentTests, HelloRetryRequestAgentTest,
                         TlsConnectTestBase::kTlsModesAll);
 #ifndef NSS_DISABLE_TLS_1_3

lib/ssl/dtlscon.c

@@ -1000,7 +1000,6 @@ dtls_HandleHelloVerifyRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
     int errCode = SSL_ERROR_RX_MALFORMED_HELLO_VERIFY_REQUEST;
     SECStatus rv;
     PRInt32 temp;
-    SECItem cookie = { siBuffer, NULL, 0 };
     SSL3AlertDescription desc = illegal_parameter;
 
     SSL_TRC(3, ("%d: SSL3[%d]: handle hello_verify_request handshake",
@@ -1025,19 +1024,18 @@ dtls_HandleHelloVerifyRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
         goto alert_loser;
     }
 
-    /* The cookie */
-    rv = ssl3_ConsumeHandshakeVariable(ss, &cookie, 1, &b, &length);
+    /* Read the cookie.
+     * IMPORTANT: The value of ss->ssl3.hs.cookie is only valid while the
+     * HelloVerifyRequest message remains valid. */
+    rv = ssl3_ConsumeHandshakeVariable(ss, &ss->ssl3.hs.cookie, 1, &b, &length);
     if (rv != SECSuccess) {
         goto loser; /* alert has been sent */
     }
-    if (cookie.len > DTLS_COOKIE_BYTES) {
+    if (ss->ssl3.hs.cookie.len > DTLS_COOKIE_BYTES) {
         desc = decode_error;
         goto alert_loser; /* malformed. */
     }
 
-    PORT_Memcpy(ss->ssl3.hs.cookie, cookie.data, cookie.len);
-    ss->ssl3.hs.cookieLen = cookie.len;
-
     ssl_GetXmitBufLock(ss); /*******************************/
 
     /* Now re-send the client hello */

lib/ssl/ssl3con.c

@@ -5157,7 +5157,7 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
              2 + num_suites * sizeof(ssl3CipherSuite) +
              1 + numCompressionMethods + total_exten_len;
     if (IS_DTLS(ss)) {
-        length += 1 + ss->ssl3.hs.cookieLen;
+        length += 1 + ss->ssl3.hs.cookie.len;
     }
 
     /* A padding extension may be included to ensure that the record containing
@@ -5237,7 +5237,7 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
 
     if (IS_DTLS(ss)) {
         rv = ssl3_AppendHandshakeVariable(
-            ss, ss->ssl3.hs.cookie, ss->ssl3.hs.cookieLen, 1);
+            ss, ss->ssl3.hs.cookie.data, ss->ssl3.hs.cookie.len, 1);
         if (rv != SECSuccess) {
             if (sid->u.ssl3.lock) {
                 PR_RWLock_Unlock(sid->u.ssl3.lock);

lib/ssl/ssl3ext.c

@@ -125,6 +125,11 @@ static SECStatus tls13_ClientHandleSigAlgsXtn(sslSocket *ss, PRUint16 ex_type,
 static PRInt32 tls13_ClientSendSupportedVersionsXtn(sslSocket *ss,
                                                     PRBool append,
                                                     PRUint32 maxBytes);
+static SECStatus tls13_ClientHandleHrrCookie(sslSocket *ss, PRUint16 ex_type,
+                                             SECItem *data);
+static PRInt32 tls13_ClientSendHrrCookieXtn(sslSocket *ss,
+                                            PRBool append,
+                                            PRUint32 maxBytes);
 
 /*
  * Write bytes.  Using this function means the SECItem structure
@@ -292,6 +297,7 @@ static const ssl3ExtensionHandler serverHelloHandlersTLS[] = {
 
 static const ssl3ExtensionHandler helloRetryRequestHandlers[] = {
     { ssl_tls13_key_share_xtn, tls13_ClientHandleKeyShareXtnHrr },
+    { ssl_tls13_cookie_xtn, tls13_ClientHandleHrrCookie },
     { -1, NULL }
 };
 
@@ -337,7 +343,8 @@ static const ssl3HelloExtensionSender clientHelloSendersTLS[SSL_MAX_EXTENSIONS]
        * client hello is empty. They are not intolerant of TLS 1.2, so list
        * signature_algorithms at the end. See bug 1243641. */
       { ssl_tls13_supported_versions_xtn, &tls13_ClientSendSupportedVersionsXtn },
-      { ssl_signature_algorithms_xtn, &ssl3_ClientSendSigAlgsXtn }
+      { ssl_signature_algorithms_xtn, &ssl3_ClientSendSigAlgsXtn },
+      { ssl_tls13_cookie_xtn, &tls13_ClientSendHrrCookieXtn }
       /* any extra entries will appear as { 0, NULL }    */
     };
 
@@ -2203,7 +2210,8 @@ ssl3_HandleParsedExtensions(sslSocket *ss,
          * in the ClientHello */
         if (!ss->sec.isServer &&
             !ssl3_ClientExtensionAdvertised(ss, extension->type) &&
-            (handshakeMessage != new_session_ticket)) {
+            (handshakeMessage != new_session_ticket) &&
+            (extension->type != ssl_tls13_cookie_xtn)) {
             (void)SSL3_SendAlert(ss, alert_fatal, unsupported_extension);
             PORT_SetError(SSL_ERROR_RX_UNEXPECTED_EXTENSION);
             return SECFailure;
@@ -3815,7 +3823,7 @@ tls13_ClientSendSupportedVersionsXtn(sslSocket *ss, PRBool append,
     extensions_len = 2 + 2 + 1 +
                      2 * (ss->vrange.max - ss->vrange.min + 1);
 
-    if (maxBytes < extensions_len) {
+    if (maxBytes < (PRUint32)extensions_len) {
         PORT_Assert(0);
         return 0;
     }
@@ -3844,6 +3852,74 @@ tls13_ClientSendSupportedVersionsXtn(sslSocket *ss, PRBool append,
     return extensions_len;
 }
 
+/*
+ *    struct {
+ *        opaque cookie<1..2^16-1>;
+ *    } Cookie;
+ */
+SECStatus
+tls13_ClientHandleHrrCookie(sslSocket *ss, PRUint16 ex_type, SECItem *data)
+{
+    SECStatus rv;
+
+    SSL_TRC(3, ("%d: TLS13[%d]: handle cookie extension",
+                SSL_GETPID(), ss->fd));
+
+    PORT_Assert(ss->vrange.max >= SSL_LIBRARY_VERSION_TLS_1_3);
+
+    /* IMPORTANT: this is only valid while the HelloRetryRequest is still valid. */
+    rv = ssl3_ConsumeHandshakeVariable(ss, &ss->ssl3.hs.cookie, 2,
+                                       &data->data, &data->len);
+    if (rv != SECSuccess) {
+        PORT_SetError(SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST);
+        return SECFailure;
+    }
+    if (!ss->ssl3.hs.cookie.len || data->len) {
+        (void)SSL3_SendAlert(ss, alert_fatal, decode_error);
+        PORT_SetError(SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST);
+        return SECFailure;
+    }
+
+    return SECSuccess;
+}
+
+PRInt32
+tls13_ClientSendHrrCookieXtn(sslSocket *ss, PRBool append, PRUint32 maxBytes)
+{
+    PRInt32 extension_len;
+
+    if (ss->vrange.max < SSL_LIBRARY_VERSION_TLS_1_3 ||
+        !ss->ssl3.hs.cookie.len) {
+        return 0;
+    }
+
+    SSL_TRC(3, ("%d: TLS13[%d]: send cookie extension", SSL_GETPID(), ss->fd));
+
+    /* Extension type, length, cookie length, cookie value. */
+    extension_len = 2 + 2 + 2 + ss->ssl3.hs.cookie.len;
+
+    if (maxBytes < (PRUint32)extension_len) {
+        PORT_Assert(0);
+        return 0;
+    }
+
+    if (append) {
+        SECStatus rv = ssl3_AppendHandshakeNumber(ss, ssl_tls13_cookie_xtn, 2);
+        if (rv != SECSuccess)
+            return -1;
+
+        rv = ssl3_AppendHandshakeNumber(ss, extension_len - 4, 2);
+        if (rv != SECSuccess)
+            return -1;
+
+        rv = ssl3_AppendHandshakeVariable(ss, ss->ssl3.hs.cookie.data,
+                                          ss->ssl3.hs.cookie.len, 2);
+        if (rv != SECSuccess)
+            return -1;
+    }
+    return extension_len;
+}
+
 void
 ssl3_DestroyRemoteExtensions(PRCList *list)
 {

lib/ssl/sslimpl.h

@@ -937,8 +937,7 @@ typedef struct SSL3HandshakeStateStr {
     PRInt32 recvdHighWater;        /* The high water mark for fragments
                                     * received. -1 means no reassembly
                                     * in progress. */
-    unsigned char cookie[32];      /* The cookie */
-    unsigned char cookieLen;       /* The length of the cookie */
+    SECItem cookie;                /* The Hello(Retry|Verify)Request cookie. */
     PRIntervalTime rtTimerStarted; /* When the timer was started */
     DTLSTimerCb rtTimerCb;         /* The function to call on expiry */
     PRUint32 rtTimeoutMs;          /* The length of the current timeout

lib/ssl/sslt.h

@@ -314,6 +314,7 @@ typedef enum {
     ssl_tls13_pre_shared_key_xtn = 41,
     ssl_tls13_early_data_xtn = 42,
     ssl_tls13_supported_versions_xtn = 43,
+    ssl_tls13_cookie_xtn = 44,
     ssl_next_proto_nego_xtn = 13172,
     ssl_renegotiation_info_xtn = 0xff01
 } SSLExtensionType;
@@ -329,7 +330,7 @@ typedef enum {
  * 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 16
+#define SSL_MAX_EXTENSIONS 17
 
 /* Deprecated */
 typedef enum {

lib/ssl/tls13con.c

@@ -1408,7 +1408,7 @@ tls13_SendHelloRetryRequest(sslSocket *ss, const sslNamedGroupDef *selectedGroup
                                         2 + /* extension length */
                                         2 + /* group extension id */
                                         2 + /* group extension length */
-                                        2   /* group */);
+                                        2 /* group */);
     if (rv != SECSuccess) {
         FATAL_ERROR(ss, SEC_ERROR_LIBRARY_FAILURE, internal_error);
         goto loser;
@@ -1659,7 +1659,12 @@ tls13_HandleHelloRetryRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
     if (tmp < 0) {
         return SECFailure; /* error code already set */
     }
-    if (tmp != length) {
+    /* Extensions must be non-empty and use the remainder of the message.
+     * This means that a HelloRetryRequest cannot be a no-op: we must have an
+     * extension, it must be one that we understand and recognize as being valid
+     * for HelloRetryRequest, and all the extensions we permit cause us to
+     * modify our ClientHello in some way. */
+    if (!tmp || tmp != length) {
         FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST,
                     decode_error);
         return SECFailure;
@@ -3699,6 +3704,7 @@ typedef enum {
     ExtensionClientOnly,
     ExtensionSendClear,
     ExtensionSendClearOrHrr,
+    ExtensionSendHrr,
     ExtensionSendEncrypted,
     ExtensionNewSessionTicket
 } Tls13ExtensionStatus;
@@ -3723,7 +3729,8 @@ static const struct {
     { ssl_renegotiation_info_xtn, ExtensionNotUsed },
     { ssl_signed_cert_timestamp_xtn, ExtensionSendEncrypted },
     { ssl_cert_status_xtn, ExtensionSendEncrypted },
-    { ssl_tls13_ticket_early_data_info_xtn, ExtensionNewSessionTicket }
+    { ssl_tls13_ticket_early_data_info_xtn, ExtensionNewSessionTicket },
+    { ssl_tls13_cookie_xtn, ExtensionSendHrr }
 };
 
 PRBool
@@ -3759,6 +3766,9 @@ tls13_ExtensionAllowed(PRUint16 extension, SSL3HandshakeType message)
             return message == client_hello ||
                    message == server_hello ||
                    message == hello_retry_request;
+        case ExtensionSendHrr:
+            return message == client_hello ||
+                   message == hello_retry_request;
         case ExtensionSendEncrypted:
             return message == client_hello ||
                    message == encrypted_extensions;