Bug 1350602 - TLS 1.3 draft-19 - CertificateRequest, r=ekr

I ended up changing the extension handler function a little to deal
with messages that allow unknown extensions better.  I also changed
tls13_ExtensionAllowed to only deal with those extensions that TLS 1.3
cares about.  We now don't send point formats in TLS 1.3; I considered
doing the same for renegotiation info, but decided against it.

Differential Revision: https://nss-review.dev.mozaws.net/D280

--HG--
branch : NSS_TLS13_DRAFT19_BRANCH
extra : rebase_source : 41533ac1e794b11925fc3b8b0f9424700fdc0f8b

gtests/ssl_gtest/ssl_extension_unittest.cc

@@ -1037,12 +1037,16 @@ class TlsBogusExtensionTest13 : public TlsBogusExtensionTest {
       return;
     }
 
+    FailWithAlert(kTlsAlertUnsupportedExtension);
+  }
+
+  void FailWithAlert(uint8_t alert) {
     client_->StartConnect();
     server_->StartConnect();
     client_->Handshake();  // ClientHello
     server_->Handshake();  // ServerHello
 
-    client_->ExpectSendAlert(kTlsAlertUnsupportedExtension);
+    client_->ExpectSendAlert(alert);
     client_->Handshake();
     if (mode_ == STREAM) {
       server_->ExpectSendAlert(kTlsAlertBadRecordMac);
@@ -1067,9 +1071,12 @@ TEST_P(TlsBogusExtensionTest13, AddBogusExtensionCertificate) {
   Run(kTlsHandshakeCertificate);
 }
 
+// It's perfectly valid to set unknown extensions in CertificateRequest.
 TEST_P(TlsBogusExtensionTest13, AddBogusExtensionCertificateRequest) {
   server_->RequestClientAuth(false);
-  Run(kTlsHandshakeCertificateRequest);
+  AddFilter(kTlsHandshakeCertificateRequest, 0xff);
+  FailWithAlert(kTlsAlertDecryptError);
+  client_->CheckErrorCode(SEC_ERROR_BAD_SIGNATURE);
 }
 
 TEST_P(TlsBogusExtensionTest13, AddBogusExtensionHelloRetryRequest) {

gtests/ssl_gtest/tls_filter.cc

@@ -448,13 +448,6 @@ static bool FindCertReqExtensions(TlsParser* parser,
   if (!parser->SkipVariable(1)) {  // request context
     return false;
   }
-  // TODO remove the next two for -19
-  if (!parser->SkipVariable(2)) {  // signature_algorithms
-    return false;
-  }
-  if (!parser->SkipVariable(2)) {  // certificate_authorities
-    return false;
-  }
   return true;
 }
 

lib/ssl/ssl3con.c

@@ -6442,8 +6442,8 @@ ssl3_PickServerSignatureScheme(sslSocket *ss)
 
     /* Sets error code, if needed. */
     return ssl_PickSignatureScheme(ss, keyPair->pubKey, keyPair->privKey,
-                                   ss->xtnData.clientSigSchemes,
-                                   ss->xtnData.numClientSigScheme,
+                                   ss->xtnData.sigSchemes,
+                                   ss->xtnData.numSigSchemes,
                                    PR_FALSE /* requireSha1 */);
 }
 
@@ -7303,7 +7303,7 @@ typedef struct dnameNode {
  */
 SECStatus
 ssl3_ParseCertificateRequestCAs(sslSocket *ss, SSL3Opaque **b, PRUint32 *length,
-                                PLArenaPool *arena, CERTDistNames *ca_list)
+                                CERTDistNames *ca_list)
 {
     PRUint32 remaining;
     int nnames = 0;
@@ -7318,7 +7318,7 @@ ssl3_ParseCertificateRequestCAs(sslSocket *ss, SSL3Opaque **b, PRUint32 *length,
     if (remaining > *length)
         goto alert_loser;
 
-    ca_list->head = node = PORT_ArenaZNew(arena, dnameNode);
+    ca_list->head = node = PORT_ArenaZNew(ca_list->arena, dnameNode);
     if (node == NULL)
         goto no_mem;
 
@@ -7344,14 +7344,14 @@ ssl3_ParseCertificateRequestCAs(sslSocket *ss, SSL3Opaque **b, PRUint32 *length,
         if (remaining <= 0)
             break; /* success */
 
-        node->next = PORT_ArenaZNew(arena, dnameNode);
+        node->next = PORT_ArenaZNew(ca_list->arena, dnameNode);
         node = node->next;
         if (node == NULL)
             goto no_mem;
     }
 
     ca_list->nnames = nnames;
-    ca_list->names = PORT_ArenaNewArray(arena, SECItem, nnames);
+    ca_list->names = PORT_ArenaNewArray(ca_list->arena, SECItem, nnames);
     if (nnames > 0 && ca_list->names == NULL)
         goto no_mem;
 
@@ -7495,7 +7495,7 @@ ssl3_HandleCertificateRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
         }
     }
 
-    rv = ssl3_ParseCertificateRequestCAs(ss, &b, &length, arena, &ca_list);
+    rv = ssl3_ParseCertificateRequestCAs(ss, &b, &length, &ca_list);
     if (rv != SECSuccess)
         goto done; /* alert sent in ssl3_ParseCertificateRequestCAs */
 
@@ -9635,10 +9635,10 @@ ssl3_SendCertificateRequest(sslSocket *ss)
     const PRUint8 *certTypes;
     SECStatus rv;
     int length;
-    SECItem *names;
+    const SECItem *names;
     unsigned int calen;
     unsigned int nnames;
-    SECItem *name;
+    const SECItem *name;
     int i;
     int certTypesLength;
     PRUint8 sigAlgs[MAX_SIGNATURE_SCHEMES * 2];
@@ -10184,8 +10184,8 @@ ssl3_SendEmptyCertificate(sslSocket *ss)
     const SECItem *context;
 
     if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
-        PORT_Assert(ss->ssl3.hs.certificateRequest);
-        context = &ss->ssl3.hs.certificateRequest->context;
+        PORT_Assert(ss->ssl3.hs.clientCertRequested);
+        context = &ss->xtnData.certReqContext;
         len = context->len + 1;
         isTLS13 = PR_TRUE;
     }
@@ -10414,8 +10414,8 @@ ssl3_SendCertificate(sslSocket *ss)
     if (isTLS13) {
         contextLen = 1; /* Size of the context length */
         if (!ss->sec.isServer) {
-            PORT_Assert(ss->ssl3.hs.certificateRequest);
-            context = ss->ssl3.hs.certificateRequest->context;
+            PORT_Assert(ss->ssl3.hs.clientCertRequested);
+            context = ss->xtnData.certReqContext;
             contextLen += context.len;
         }
     }
@@ -12889,7 +12889,6 @@ ssl3_InitState(sslSocket *ss)
     ss->ssl3.hs.serverHsTrafficSecret = NULL;
     ss->ssl3.hs.clientTrafficSecret = NULL;
     ss->ssl3.hs.serverTrafficSecret = NULL;
-    ss->ssl3.hs.certificateRequest = NULL;
     PR_INIT_CLIST(&ss->ssl3.hs.cipherSpecs);
 
     PORT_Assert(!ss->ssl3.hs.messages.buf && !ss->ssl3.hs.messages.space);
@@ -13231,11 +13230,6 @@ ssl3_DestroySSL3Info(sslSocket *ss)
     SECITEM_FreeItem(&ss->ssl3.hs.newSessionTicket.ticket, PR_FALSE);
     SECITEM_FreeItem(&ss->ssl3.hs.srvVirtName, PR_FALSE);
 
-    if (ss->ssl3.hs.certificateRequest) {
-        PORT_FreeArena(ss->ssl3.hs.certificateRequest->arena, PR_FALSE);
-        ss->ssl3.hs.certificateRequest = NULL;
-    }
-
     /* free up the CipherSpecs */
     ssl3_DestroyCipherSpec(&ss->ssl3.specs[0], PR_TRUE /*freeSrvName*/);
     ssl3_DestroyCipherSpec(&ss->ssl3.specs[1], PR_TRUE /*freeSrvName*/);

lib/ssl/ssl3ext.c

@@ -31,7 +31,7 @@ static const ssl3ExtensionHandler clientHelloHandlers[] = {
     { ssl_app_layer_protocol_xtn, &ssl3_ServerHandleAppProtoXtn },
     { ssl_use_srtp_xtn, &ssl3_ServerHandleUseSRTPXtn },
     { ssl_cert_status_xtn, &ssl3_ServerHandleStatusRequestXtn },
-    { ssl_signature_algorithms_xtn, &ssl3_ServerHandleSigAlgsXtn },
+    { ssl_signature_algorithms_xtn, &ssl3_HandleSigAlgsXtn },
     { ssl_extended_master_secret_xtn, &ssl3_HandleExtendedMasterSecretXtn },
     { ssl_signed_cert_timestamp_xtn, &ssl3_ServerHandleSignedCertTimestampXtn },
     { ssl_tls13_key_share_xtn, &tls13_ServerHandleKeyShareXtn },
@@ -88,6 +88,9 @@ static const ssl3ExtensionHandler serverCertificateHandlers[] = {
 };
 
 static const ssl3ExtensionHandler certificateRequestHandlers[] = {
+    { ssl_signature_algorithms_xtn, &ssl3_HandleSigAlgsXtn },
+    { ssl_tls13_certificate_authorities_xtn,
+      &tls13_ClientHandleCertAuthoritiesXtn },
     { -1, NULL }
 };
 
@@ -101,7 +104,7 @@ static const ssl3ExtensionHandler certificateRequestHandlers[] = {
  * the client hello is empty (for example, the extended master secret
  * extension, if it were listed last). See bug 1243641.
  */
-static const ssl3HelloExtensionSender clientHelloSendersTLS[SSL_MAX_EXTENSIONS] =
+static const ssl3HelloExtensionSender clientHelloSendersTLS[] =
     {
       { ssl_server_name_xtn, &ssl3_SendServerNameXtn },
       { ssl_extended_master_secret_xtn, &ssl3_SendExtendedMasterSecretXtn },
@@ -122,19 +125,19 @@ static const ssl3HelloExtensionSender clientHelloSendersTLS[SSL_MAX_EXTENSIONS]
        * signature_algorithms at the end. See bug 1243641. */
       { ssl_tls13_supported_versions_xtn, &tls13_ClientSendSupportedVersionsXtn },
       { ssl_tls13_short_header_xtn, &tls13_SendShortHeaderXtn },
-      { ssl_signature_algorithms_xtn, &ssl3_ClientSendSigAlgsXtn },
+      { ssl_signature_algorithms_xtn, &ssl3_SendSigAlgsXtn },
       { 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 }    */
+      { 0, NULL }
     };
 
-static const ssl3HelloExtensionSender clientHelloSendersSSL3[SSL_MAX_EXTENSIONS] = {
-    { ssl_renegotiation_info_xtn, &ssl3_SendRenegotiationInfoXtn }
-    /* any extra entries will appear as { 0, NULL }    */
+static const ssl3HelloExtensionSender clientHelloSendersSSL3[] = {
+    { ssl_renegotiation_info_xtn, &ssl3_SendRenegotiationInfoXtn },
+    { 0, NULL }
 };
 
 static PRBool
@@ -258,6 +261,11 @@ ssl3_HandleParsedExtensions(sslSocket *ss,
      * do so, but we weren't entirely sure. TODO(ekr@rtfm.com). */
     PRBool isTLS13 = (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) ||
                      (handshakeMessage == hello_retry_request);
+    /* The following messages can include extensions that were not included in
+     * the original ClientHello. */
+    PRBool allowNotOffered = (handshakeMessage == client_hello) ||
+                             (handshakeMessage == certificate_request) ||
+                             (handshakeMessage == new_session_ticket);
     PRCList *cursor;
 
     switch (handshakeMessage) {
@@ -302,25 +310,35 @@ ssl3_HandleParsedExtensions(sslSocket *ss,
         const ssl3ExtensionHandler *handler;
 
         /* Check whether the server sent an extension which was not advertised
-         * in the ClientHello */
-        if (!ss->sec.isServer &&
-            !ssl3_ClientExtensionAdvertised(ss, extension->type) &&
-            (handshakeMessage != new_session_ticket) &&
-            (extension->type != ssl_tls13_cookie_xtn)) {
+         * in the ClientHello.
+         *
+         * Note that a TLS 1.3 server should check if CertificateRequest
+         * extensions were sent.  But the extensions used for CertificateRequest
+         * do not have any response, so we rely on
+         * ssl3_ClientExtensionAdvertised to return false on the server.  That
+         * results in the server only rejecting any extension. */
+        if (!allowNotOffered && (extension->type != ssl_tls13_cookie_xtn) &&
+            !ssl3_ClientExtensionAdvertised(ss, extension->type)) {
             (void)SSL3_SendAlert(ss, alert_fatal, unsupported_extension);
             PORT_SetError(SSL_ERROR_RX_UNEXPECTED_EXTENSION);
             return SECFailure;
         }
 
         /* Check that this is a legal extension in TLS 1.3 */
-        if (isTLS13 && !tls13_ExtensionAllowed(extension->type, handshakeMessage)) {
-            if (handshakeMessage == client_hello) {
-                /* Skip extensions not used in TLS 1.3 */
-                continue;
+        if (isTLS13) {
+            switch (tls13_ExtensionStatus(extension->type, handshakeMessage)) {
+                case tls13_extension_allowed:
+                    break;
+                case tls13_extension_unknown:
+                    if (allowNotOffered) {
+                        continue; /* Skip over unknown extensions. */
+                    }
+                /* Fall through. */
+                case tls13_extension_disallowed:
+                    tls13_FatalError(ss, SSL_ERROR_EXTENSION_DISALLOWED_FOR_VERSION,
+                                     unsupported_extension);
+                    return SECFailure;
             }
-            tls13_FatalError(ss, SSL_ERROR_EXTENSION_DISALLOWED_FOR_VERSION,
-                             unsupported_extension);
-            return SECFailure;
         }
 
         /* Special check for this being the last extension if it's
@@ -350,6 +368,7 @@ ssl3_HandleParsedExtensions(sslSocket *ss,
                     }
                     return SECFailure;
                 }
+                break;
             }
         }
     }
@@ -390,14 +409,21 @@ ssl3_RegisterExtensionSender(const sslSocket *ss,
     if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
         sender = &xtnData->serverHelloSenders[0];
     } else {
-        if (tls13_ExtensionAllowed(ex_type, server_hello)) {
-            PORT_Assert(!tls13_ExtensionAllowed(ex_type, encrypted_extensions));
+        if (tls13_ExtensionStatus(ex_type, server_hello) ==
+            tls13_extension_allowed) {
+            PORT_Assert(tls13_ExtensionStatus(ex_type, encrypted_extensions) ==
+                        tls13_extension_disallowed);
             sender = &xtnData->serverHelloSenders[0];
-        } else if (tls13_ExtensionAllowed(ex_type, certificate)) {
+        } else if (tls13_ExtensionStatus(ex_type, encrypted_extensions) ==
+                   tls13_extension_allowed) {
+            sender = &xtnData->encryptedExtensionsSenders[0];
+        } else if (tls13_ExtensionStatus(ex_type, certificate) ==
+                   tls13_extension_allowed) {
             sender = &xtnData->certificateSenders[0];
         } else {
-            PORT_Assert(tls13_ExtensionAllowed(ex_type, encrypted_extensions));
-            sender = &xtnData->encryptedExtensionsSenders[0];
+            PORT_Assert(0);
+            PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
+            return SECFailure;
         }
     }
     for (i = 0; i < SSL_MAX_EXTENSIONS; ++i, ++sender) {
@@ -424,7 +450,6 @@ ssl3_CallHelloExtensionSenders(sslSocket *ss, PRBool append, PRUint32 maxBytes,
                                const ssl3HelloExtensionSender *sender)
 {
     PRInt32 total_exten_len = 0;
-    int i;
 
     if (!sender) {
         if (ss->vrange.max > SSL_LIBRARY_VERSION_3_0) {
@@ -434,14 +459,14 @@ ssl3_CallHelloExtensionSenders(sslSocket *ss, PRBool append, PRUint32 maxBytes,
         }
     }
 
-    for (i = 0; i < SSL_MAX_EXTENSIONS; ++i, ++sender) {
-        if (sender->ex_sender) {
-            PRInt32 extLen = (*sender->ex_sender)(ss, &ss->xtnData, append, maxBytes);
-            if (extLen < 0)
-                return -1;
-            maxBytes -= extLen;
-            total_exten_len += extLen;
+    while (sender->ex_sender) {
+        PRInt32 extLen = (*sender->ex_sender)(ss, &ss->xtnData, append, maxBytes);
+        if (extLen < 0) {
+            return -1;
         }
+        maxBytes -= extLen;
+        total_exten_len += extLen;
+        ++sender;
     }
     return total_exten_len;
 }
@@ -475,9 +500,14 @@ ssl3_ResetExtensionData(TLSExtensionData *xtnData)
 {
     /* Clean up. */
     ssl3_FreeSniNameArray(xtnData);
-    PORT_Free(xtnData->clientSigSchemes);
+    PORT_Free(xtnData->sigSchemes);
     SECITEM_FreeItem(&xtnData->nextProto, PR_FALSE);
     tls13_DestroyKeyShares(&xtnData->remoteKeyShares);
+    SECITEM_FreeItem(&xtnData->certReqContext, PR_FALSE);
+    if (xtnData->certReqAuthorities.arena) {
+        PORT_FreeArena(xtnData->certReqAuthorities.arena, PR_FALSE);
+        xtnData->certReqAuthorities.arena = NULL;
+    }
 
     /* Now reinit. */
     ssl3_InitExtensionData(xtnData);

lib/ssl/ssl3ext.h

@@ -86,10 +86,13 @@ struct TLSExtensionDataStr {
     PRBool peerSupportsFfdheGroups; /* if the peer supports named ffdhe groups */
 
     /* clientSigAndHash contains the contents of the signature_algorithms
-     * extension (if any) from the client. This is only valid for TLS 1.2
-     * or later. */
-    SSLSignatureScheme *clientSigSchemes;
-    unsigned int numClientSigScheme;
+     * extension (if any) the other side supports. This is only valid for TLS
+     * 1.2 or later. In TLS 1.3, it is also used for CertificateRequest. */
+    SSLSignatureScheme *sigSchemes;
+    unsigned int numSigSchemes;
+
+    SECItem certReqContext;
+    CERTDistNames certReqAuthorities;
 
     /* In a client: if the server supports Next Protocol Negotiation, then
      * this is the protocol that was negotiated.