Bug 1457716 - Fix CertificateRequest processing for TLS 1.3. r=mt

Reviewers: mt

Tags: #secure-revision

Bug #: 1457716

Differential Revision: https://phabricator.services.mozilla.com/D1062

6 files changed, 44 insertions, 6 deletions

diff --git a/cpputil/scoped_ptrs.h b/cpputil/scoped_ptrs.h
index 8a0b4f5ab..6ffef4dd3 100644
--- a/cpputil/scoped_ptrs.h
+++ b/cpputil/scoped_ptrs.h
@@ -45,6 +45,7 @@ struct ScopedDelete {
   void operator()(SEC_PKCS12DecoderContext* dcx) {
     SEC_PKCS12DecoderFinish(dcx);
   }
+  void operator()(CERTDistNames* names) { CERT_FreeDistNames(names); }
 };
 
 template <class T>
@@ -78,6 +79,7 @@ SCOPED(PK11Context);
 SCOPED(PK11GenericObject);
 SCOPED(SSLResumptionTokenInfo);
 SCOPED(SEC_PKCS12DecoderContext);
+SCOPED(CERTDistNames);
 
 #undef SCOPED
 
diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc
index fc536298c..a7e0acaa2 100644
--- a/gtests/ssl_gtest/tls_agent.cc
+++ b/gtests/ssl_gtest/tls_agent.cc
@@ -192,10 +192,6 @@ bool TlsAgent::EnsureTlsSetup(PRFileDesc* modelSocket) {
     EXPECT_EQ(SECSuccess, rv);
     if (rv != SECSuccess) return false;
 
-    ScopedCERTCertList anchors(CERT_NewCertList());
-    rv = SSL_SetTrustAnchors(ssl_fd(), anchors.get());
-    if (rv != SECSuccess) return false;
-
     rv = SSL_SetMaxEarlyDataSize(ssl_fd(), 1024);
     EXPECT_EQ(SECSuccess, rv);
     if (rv != SECSuccess) return false;
@@ -256,6 +252,17 @@ void TlsAgent::SetupClientAuth() {
                                       reinterpret_cast<void*>(this)));
 }
 
+static void CheckCertReqAgainstDefaultCAs(const CERTDistNames* caNames) {
+  ScopedCERTDistNames expected(CERT_GetSSLCACerts(nullptr));
+
+  ASSERT_EQ(expected->nnames, caNames->nnames);
+
+  for (size_t i = 0; i < static_cast<size_t>(expected->nnames); ++i) {
+    EXPECT_EQ(SECEqual,
+              SECITEM_CompareItem(&(expected->names[i]), &(caNames->names[i])));
+  }
+}
+
 SECStatus TlsAgent::GetClientAuthDataHook(void* self, PRFileDesc* fd,
                                           CERTDistNames* caNames,
                                           CERTCertificate** clientCert,
@@ -264,6 +271,8 @@ SECStatus TlsAgent::GetClientAuthDataHook(void* self, PRFileDesc* fd,
   ScopedCERTCertificate peerCert(SSL_PeerCertificate(agent->ssl_fd()));
   EXPECT_TRUE(peerCert) << "Client should be able to see the server cert";
 
+  CheckCertReqAgainstDefaultCAs(caNames);
+
   ScopedCERTCertificate cert;
   ScopedSECKEYPrivateKey priv;
   if (!TlsAgent::LoadCertificate(agent->name(), &cert, &priv)) {
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 391647c9e..dd280d2ef 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -6948,8 +6948,10 @@ ssl3_ParseCertificateRequestCAs(sslSocket *ss, PRUint8 **b, PRUint32 *length,
             goto alert_loser; /* malformed */
 
         remaining -= 2;
+        if (SECITEM_MakeItem(ca_list->arena, &node->name, *b, len) != SECSuccess) {
+            goto no_mem;
+        }
         node->name.len = len;
-        node->name.data = *b;
         *b += len;
         *length -= len;
         remaining -= len;
@@ -6977,7 +6979,6 @@ ssl3_ParseCertificateRequestCAs(sslSocket *ss, PRUint8 **b, PRUint32 *length,
     return SECSuccess;
 
 no_mem:
-    PORT_SetError(SEC_ERROR_NO_MEMORY);
     return SECFailure;
 
 alert_loser:
@@ -11400,6 +11401,10 @@ ssl3_HandleHandshakeMessage(sslSocket *ss, PRUint8 *b, PRUint32 length,
         /* Increment the expected sequence number */
         ss->ssl3.hs.recvMessageSeq++;
     }
+
+    /* Taint the message so that it's easier to detect UAFs. */
+    PORT_Memset(b, 'N', length);
+
     return rv;
 }
 
diff --git a/lib/util/nssutil.def b/lib/util/nssutil.def
index 936455f6e..c09e8fa80 100644
--- a/lib/util/nssutil.def
+++ b/lib/util/nssutil.def
@@ -322,4 +322,9 @@ _NSSUTIL_UTF8ToWide;-
 _NSSUTIL_Access;-
 ;-    local:
 ;-       *;
+;-NSSUTIL_3.38 {         # NSS Utilities 3.38 release
+;-    global:
+SECITEM_MakeItem;
+;-    local:
+;-       *;
 ;-};
diff --git a/lib/util/secitem.c b/lib/util/secitem.c
index 22c5b1f6e..1e505a9af 100644
--- a/lib/util/secitem.c
+++ b/lib/util/secitem.c
@@ -76,6 +76,15 @@ loser:
 }
 
 SECStatus
+SECITEM_MakeItem(PLArenaPool *arena, SECItem *dest, unsigned char *data,
+                 unsigned int len)
+{
+    SECItem it = { siBuffer, data, len };
+
+    return SECITEM_CopyItem(arena, dest, &it);
+}
+
+SECStatus
 SECITEM_ReallocItem(PLArenaPool *arena, SECItem *item, unsigned int oldlen,
                     unsigned int newlen)
 {
diff --git a/lib/util/secitem.h b/lib/util/secitem.h
index 5b9d0e174..4fb123938 100644
--- a/lib/util/secitem.h
+++ b/lib/util/secitem.h
@@ -35,6 +35,14 @@ SEC_BEGIN_PROTOS
 extern SECItem *SECITEM_AllocItem(PLArenaPool *arena, SECItem *item,
                                   unsigned int len);
 
+/* Allocate and make an item with the requested contents.
+ *
+ * We seem to have mostly given up on SECItemType, so the result is
+ * always siBuffer.
+ */
+extern SECStatus SECITEM_MakeItem(PLArenaPool *arena, SECItem *dest,
+                                  unsigned char *data, unsigned int len);
+
 /*
 ** This is a legacy function containing bugs. It doesn't update item->len,
 ** and it has other issues as described in bug 298649 and bug 298938.