Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ekr committed May 28, 2018
1 parent fe5fd11 commit bacf088
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 6 deletions.
2 changes: 2 additions & 0 deletions cpputil/scoped_ptrs.h
Expand Up @@ -45,6 +45,7 @@ struct ScopedDelete {
void operator()(SEC_PKCS12DecoderContext* dcx) {
SEC_PKCS12DecoderFinish(dcx);
}
void operator()(CERTDistNames* names) { CERT_FreeDistNames(names); }
};

template <class T>
Expand Down Expand Up @@ -78,6 +79,7 @@ SCOPED(PK11Context);
SCOPED(PK11GenericObject);
SCOPED(SSLResumptionTokenInfo);
SCOPED(SEC_PKCS12DecoderContext);
SCOPED(CERTDistNames);

#undef SCOPED

Expand Down
17 changes: 13 additions & 4 deletions gtests/ssl_gtest/tls_agent.cc
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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)) {
Expand Down
9 changes: 7 additions & 2 deletions lib/ssl/ssl3con.c
Expand Up @@ -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;
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}

Expand Down
5 changes: 5 additions & 0 deletions lib/util/nssutil.def
Expand Up @@ -322,4 +322,9 @@ _NSSUTIL_UTF8ToWide;-
_NSSUTIL_Access;-
;- local:
;- *;
;-NSSUTIL_3.38 { # NSS Utilities 3.38 release
;- global:
SECITEM_MakeItem;
;- local:
;- *;
;-};
9 changes: 9 additions & 0 deletions lib/util/secitem.c
Expand Up @@ -75,6 +75,15 @@ SECITEM_AllocItem(PLArenaPool *arena, SECItem *item, unsigned int len)
return (NULL);
}

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)
Expand Down
8 changes: 8 additions & 0 deletions lib/util/secitem.h
Expand Up @@ -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.
Expand Down

0 comments on commit bacf088

Please sign in to comment.