Bug 873673: Clean up certificate_status message processing. r=wtc.
authorBrian Smith <bsmith@mozilla.com>
Tue, 21 May 2013 19:01:08 -0700
changeset 10783 722814555d1dd95af97995e5596ffa5f2855f3cb
parent 10782 d197ed6c2d5a214eeef44d0de18770ceca9958fd
child 10784 60da07951c1702a35e3b8a3bab4f9a6beadc9742
push id92
push userwtc@google.com
push dateWed, 22 May 2013 02:01:15 +0000
reviewerswtc
bugs873673
Bug 873673: Clean up certificate_status message processing. r=wtc.
lib/ssl/ssl3con.c
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -35,16 +35,17 @@
 #include "zlib.h"
 #endif
 
 #ifndef PK11_SETATTRS
 #define PK11_SETATTRS(x,id,v,l) (x)->type = (id); \
 		(x)->pValue=(v); (x)->ulValueLen = (l);
 #endif
 
+static SECStatus ssl3_AuthCertificate(sslSocket *ss);
 static void      ssl3_CleanupPeerCerts(sslSocket *ss);
 static PK11SymKey *ssl3_GenerateRSAPMS(sslSocket *ss, ssl3CipherSpec *spec,
                                        PK11SlotInfo * serverKeySlot);
 static SECStatus ssl3_DeriveMasterSecret(sslSocket *ss, PK11SymKey *pms);
 static SECStatus ssl3_DeriveConnectionKeysPKCS11(sslSocket *ss);
 static SECStatus ssl3_HandshakeFailure(      sslSocket *ss);
 static SECStatus ssl3_InitState(             sslSocket *ss);
 static SECStatus ssl3_SendCertificate(       sslSocket *ss);
@@ -8515,17 +8516,24 @@ ssl3_CleanupPeerCerts(sslSocket *ss)
  * Caller must hold Handshake and RecvBuf locks.
  * This is always called before ssl3_HandleCertificate, even if the Certificate
  * message is sent first.
  */
 static SECStatus
 ssl3_HandleCertificateStatus(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
 {
     PRInt32 status, len;
-    PORT_Assert(ss->ssl3.hs.ws == wait_certificate_status);
+
+    if (ss->ssl3.hs.ws != wait_certificate_status) {
+        (void)SSL3_SendAlert(ss, alert_fatal, unexpected_message);
+        PORT_SetError(SSL_ERROR_RX_UNEXPECTED_CERT_STATUS);
+        return SECFailure;
+    }
+
+    PORT_Assert(!ss->sec.isServer);
 
     /* Consume the CertificateStatusType enum */
     status = ssl3_ConsumeHandshakeNumber(ss, 1, &b, &length);
     if (status != 1 /* ocsp */) {
        goto format_loser;
     }
 
     len = ssl3_ConsumeHandshakeNumber(ss, 3, &b, &length);
@@ -8548,24 +8556,23 @@ ssl3_HandleCertificateStatus(sslSocket *
     if (!ss->sec.ci.sid->peerCertStatus.items[0].data) {
         SECITEM_FreeArray(&ss->sec.ci.sid->peerCertStatus, PR_FALSE);
         return SECFailure;
     }
 
     PORT_Memcpy(ss->sec.ci.sid->peerCertStatus.items[0].data, b, length);
     ss->sec.ci.sid->peerCertStatus.items[0].len = length;
     ss->sec.ci.sid->peerCertStatus.items[0].type = siBuffer;
-    return SECSuccess;
+
+    return ssl3_AuthCertificate(ss);
 
 format_loser:
     return ssl3_DecodeError(ss);
 }
 
-static SECStatus ssl3_AuthCertificate(sslSocket *ss);
-
 /* Called from ssl3_HandleHandshakeMessage() when it has deciphered a complete
  * ssl3 Certificate message.
  * Caller must hold Handshake and RecvBuf locks.
  */
 static SECStatus
 ssl3_HandleCertificate(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
 {
     ssl3CertNode *   c;
@@ -9542,40 +9549,28 @@ ssl3_HandleHandshakeMessage(sslSocket *s
 
 	/* The message body */
 	rv = ssl3_UpdateHandshakeHashes(ss, b, length);
 	if (rv != SECSuccess) return rv;	/* err code already set. */
     }
 
     PORT_SetError(0);	/* each message starts with no error. */
 
-    /* The CertificateStatus message is optional. We process the message if we
-     * get one when it is allowed, but otherwise we just carry on.
-     */
-    if (ss->ssl3.hs.ws == wait_certificate_status) {
-        /* We must process any CertificateStatus message before we call
-         * ssl3_AuthCertificate, as ssl3_AuthCertificate needs any stapled
-         * OCSP response we get.
-         */
-        if (ss->ssl3.hs.msg_type == certificate_status) {
-            rv = ssl3_HandleCertificateStatus(ss, b, length);
-            if (rv != SECSuccess)
-                return rv;
-            if (IS_DTLS(ss)) {
-                /* Increment the expected sequence number */
-                ss->ssl3.hs.recvMessageSeq++;
-            }
-        }
-
-        /* Regardless of whether we got a CertificateStatus message, we must
-         * authenticate the cert before we handle any more handshake messages.
+    if (ss->ssl3.hs.ws == wait_certificate_status &&
+        ss->ssl3.hs.msg_type != certificate_status) {
+        /* If we negotiated the certificate_status extension then we deferred
+         * certificate validation until we get the CertificateStatus messsage.
+         * But the CertificateStatus message is optional. If the server did
+         * not send it then we need to validate the certificate now. If the
+         * server does send the CertificateStatus message then we will
+         * authenticate the certificate in ssl3_HandleCertificateStatus.
          */
         rv = ssl3_AuthCertificate(ss); /* sets ss->ssl3.hs.ws */
         PORT_Assert(rv != SECWouldBlock);
-        if (rv != SECSuccess || ss->ssl3.hs.msg_type == certificate_status) {
+        if (rv != SECSuccess) {
             return rv;
         }
     }
 
     switch (ss->ssl3.hs.msg_type) {
     case hello_request:
 	if (length != 0) {
 	    (void)ssl3_DecodeError(ss);
@@ -9612,20 +9607,18 @@ ssl3_HandleHandshakeMessage(sslSocket *s
 	    return SECFailure;
 	}
 	rv = dtls_HandleHelloVerifyRequest(ss, b, length);
 	break;
     case certificate:
 	rv = ssl3_HandleCertificate(ss, b, length);
 	break;
     case certificate_status:
-	/* The good case is handled above */
-	(void)SSL3_SendAlert(ss, alert_fatal, unexpected_message);
-	PORT_SetError(SSL_ERROR_RX_UNEXPECTED_CERT_STATUS);
-	return SECFailure;
+	rv = ssl3_HandleCertificateStatus(ss, b, length);
+	break;
     case server_key_exchange:
 	if (ss->sec.isServer) {
 	    (void)SSL3_SendAlert(ss, alert_fatal, unexpected_message);
 	    PORT_SetError(SSL_ERROR_RX_UNEXPECTED_SERVER_KEY_EXCH);
 	    return SECFailure;
 	}
 	rv = ssl3_HandleServerKeyExchange(ss, b, length);
 	break;