Overlapping fixes for Bug 554369 and Bug 360420. OCSP caching fixes by Adam Langley, r=kaie; Cache injection of OCSP stapling data inside default auth code, by me, r=rrelyea

git-svn-id: svn://10.0.0.236/trunk@264733 18797224-902f-48f8-a5cc-f745e15eee43

mozilla/security/nss/lib/certhigh/ocsp.c

@@ -6,7 +6,7 @@
  * Implementation of OCSP services, for both client and server.
  * (XXX, really, mostly just for client right now, but intended to do both.)
  *
- * $Id: ocsp.c,v 1.77 2013-01-23 23:05:50 kaie%kuix.de Exp $
+ * $Id: ocsp.c,v 1.78 2013-02-15 17:53:24 kaie%kuix.de Exp $
  */
 
 #include "prerror.h"
@@ -124,9 +124,9 @@ ocsp_CacheEncodedOCSPResponse(CERTCertDBHandle *handle,
 			      CERTCertificate *cert,
 			      int64 time,
 			      void *pwArg,
-			      SECItem *encodedResponse,
+			      const SECItem *encodedResponse,
+			      PRBool cacheInvalid,
 			      PRBool *certIDWasConsumed,
-			      PRBool cacheNegative,
 			      SECStatus *rv_ocsp);
 
 static SECStatus
@@ -140,6 +140,9 @@ ocsp_GetVerifiedSingleResponseForCertID(CERTCertDBHandle *handle,
 static SECStatus
 ocsp_CertRevokedAfter(ocspRevokedInfo *revokedInfo, int64 time);
 
+static CERTOCSPCertID *
+cert_DupOCSPCertID(CERTOCSPCertID *src);
+
 #ifndef DEBUG
 #define OCSP_TRACE(msg)
 #define OCSP_TRACE_TIME(msg, time)
@@ -766,6 +769,9 @@ ocsp_IsCacheItemFresh(OCSPCacheItem *cacheItem)
 /*
  * Status in *certIDWasConsumed will always be correct, regardless of 
  * return value.
+ * If the caller is unable to transfer ownership of certID,
+ * then the caller must set certIDWasConsumed to NULL,
+ * and this function will potentially duplicate the certID object.
  */
 static SECStatus
 ocsp_CreateOrUpdateCacheEntry(OCSPCacheData *cache, 
@@ -777,10 +783,7 @@ ocsp_CreateOrUpdateCacheEntry(OCSPCacheData *cache,
     OCSPCacheItem *cacheItem;
     OCSP_TRACE(("OCSP ocsp_CreateOrUpdateCacheEntry\n"));
   
-    if (!certIDWasConsumed) {
+    if (certIDWasConsumed)
-        PORT_SetError(SEC_ERROR_INVALID_ARGS);
-        return SECFailure;
-    }
     *certIDWasConsumed = PR_FALSE;
   
     PR_EnterMonitor(OCSP_Global.monitor);
@@ -788,23 +791,47 @@ ocsp_CreateOrUpdateCacheEntry(OCSPCacheData *cache,
   
     cacheItem = ocsp_FindCacheEntry(cache, certID);
     if (!cacheItem) {
-        rv = ocsp_CreateCacheItemAndConsumeCertID(cache, certID, 
+        CERTOCSPCertID *myCertID;
+        if (certIDWasConsumed) {
+            myCertID = certID;
+            *certIDWasConsumed = PR_TRUE;
+        } else {
+            myCertID = cert_DupOCSPCertID(certID);
+            if (!myCertID) {
+                PR_ExitMonitor(OCSP_Global.monitor);
+                PORT_SetError(PR_OUT_OF_MEMORY_ERROR);
+                return SECFailure;
+            }
+        }
+
+        rv = ocsp_CreateCacheItemAndConsumeCertID(cache, myCertID,
                                                   &cacheItem);
         if (rv != SECSuccess) {
             PR_ExitMonitor(OCSP_Global.monitor);
             return rv;
         }
-        *certIDWasConsumed = PR_TRUE;
     }
     if (single) {
-        rv = ocsp_SetCacheItemResponse(cacheItem, single);
+        PRTime thisUpdate;
-        if (rv != SECSuccess) {
+        rv = DER_GeneralizedTimeToTime(&thisUpdate, &single->thisUpdate);
-            ocsp_RemoveCacheItem(cache, cacheItem);
+
-            PR_ExitMonitor(OCSP_Global.monitor);
+        if (!cacheItem->haveThisUpdate ||
-            return rv;
+            (rv == SECSuccess && cacheItem->thisUpdate < thisUpdate)) {
+            rv = ocsp_SetCacheItemResponse(cacheItem, single);
+            if (rv != SECSuccess) {
+                ocsp_RemoveCacheItem(cache, cacheItem);
+                PR_ExitMonitor(OCSP_Global.monitor);
+                return rv;
+            }
+        } else {
+            OCSP_TRACE(("Not caching response because the response is not newer than the cache"));
         }
     } else {
         cacheItem->missingResponseError = PORT_GetError();
+        if (cacheItem->certStatusArena) {
+            PORT_FreeArena(cacheItem->certStatusArena, PR_FALSE);
+            cacheItem->certStatusArena = NULL;
+        }
     }
     ocsp_FreshenCacheItemNextFetchAttemptTime(cacheItem);
     ocsp_CheckCacheSize(cache);
@@ -1752,6 +1779,54 @@ CERT_CreateOCSPCertID(CERTCertificate *cert, int64 time)
     return certID;
 }
 
+static CERTOCSPCertID *
+cert_DupOCSPCertID(CERTOCSPCertID *src)
+{
+    CERTOCSPCertID *dest;
+    PRArenaPool *arena = NULL;
+
+    if (!src) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return NULL;
+    }
+
+    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
+    if (!arena)
+        goto loser;
+
+    dest = PORT_ArenaZNew(arena, CERTOCSPCertID);
+      if (!dest)
+        goto loser;
+
+#define DUPHELP(element) \
+    if (src->element.data) {  \
+        if (SECITEM_CopyItem(arena, &dest->element, &src->element) \
+            != SECSuccess) \
+            goto loser;     \
+    }
+
+    DUPHELP(hashAlgorithm.algorithm)
+    DUPHELP(hashAlgorithm.parameters)
+    DUPHELP(issuerNameHash)
+    DUPHELP(issuerKeyHash)
+    DUPHELP(serialNumber)
+    DUPHELP(issuerSHA1NameHash)
+    DUPHELP(issuerMD5NameHash)
+    DUPHELP(issuerMD2NameHash)
+    DUPHELP(issuerSHA1KeyHash)
+    DUPHELP(issuerMD5KeyHash)
+    DUPHELP(issuerMD2KeyHash)
+
+    dest->poolp = arena;
+    return dest;
+
+loser:
+    if (arena)
+        PORT_FreeArena(arena, PR_FALSE);
+    PORT_SetError(PR_OUT_OF_MEMORY_ERROR);
+    return NULL;
+}
+
 /*
  * Callback to set Extensions in request object
  */
@@ -2535,7 +2610,7 @@ ocsp_DecodeResponseBytes(PRArenaPool *arena, ocspResponseBytes *rbytes)
  *   or a low-level or internal error occurred).
  */
 CERTOCSPResponse *
-CERT_DecodeOCSPResponse(SECItem *src)
+CERT_DecodeOCSPResponse(const SECItem *src)
 {
     PRArenaPool *arena = NULL;
     CERTOCSPResponse *response = NULL;
@@ -4817,15 +4892,58 @@ SECStatus
 CERT_CacheOCSPResponseFromSideChannel(CERTCertDBHandle *handle,
 				      CERTCertificate *cert,
 				      int64 time,
-				      SECItem *encodedResponse,
+				      const SECItem *encodedResponse,
 				      void *pwArg)
 {
-    CERTOCSPCertID *certID;
+    CERTOCSPCertID *certID = NULL;
     PRBool certIDWasConsumed = PR_FALSE;
     SECStatus rv = SECFailure;
     SECStatus rvOcsp;
     SECErrorCodes dummy_error_code; /* we ignore this */
 
+    /* The OCSP cache can be in three states regarding this certificate:
+     *    + Good (cached, timely, 'good' response, or revoked in the future)
+     *    + Revoked (cached, timely, but doesn't fit in the last category)
+     *    + Miss (no knowledge)
+     *
+     * Likewise, the side-channel information can be
+     *    + Good (timely, 'good' response, or revoked in the future)
+     *    + Revoked (timely, but doesn't fit in the last category)
+     *    + Invalid (bad syntax, bad signature, not timely etc)
+     *
+     * The common case is that the cache result is Good and so is the
+     * side-channel information. We want to save processing time in this case
+     * so we say that any time we see a Good result from the cache we return
+     * early.
+     *
+     *                       Cache result
+     *      | Good             Revoked               Miss
+     *   ---+--------------------------------------------
+     *    G |  noop           Cache more           Cache it
+     * S    |                 recent result
+     * i    |
+     * d    |
+     * e    |
+     *    R |  noop           Cache more           Cache it
+     * C    |                 recent result
+     * h    |
+     * a    |
+     * n    |
+     * n  I |  noop           Noop                  Noop
+     * e    |
+     * l    |
+     *
+     * When we fetch from the network we might choose to cache a negative
+     * result when the response is invalid. This saves us hammering, uselessly,
+     * at a broken responder. However, side channels are commonly attacker
+     * controlled and so we must not cache a negative result for an Invalid
+     * side channel.
+     */
+
+    if (!cert) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
     certID = CERT_CreateOCSPCertID(cert, time);
     if (!certID)
         return SECFailure;
@@ -4833,22 +4951,18 @@ CERT_CacheOCSPResponseFromSideChannel(CERTCertDBHandle *handle,
         certID, time, PR_FALSE, /* ignoreGlobalOcspFailureSetting */
         &rvOcsp, &dummy_error_code);
     if (rv == SECSuccess && rvOcsp == SECSuccess) {
-	/* The cached value is good. We don't want to waste time validating
+        /* The cached value is good. We don't want to waste time validating
-	 * this OCSP response. */
+         * this OCSP response. This is the first column in the table above. */
         CERT_DestroyOCSPCertID(certID);
         return rv;
     }
 
-    /* Since the OCSP response came from a side channel it is attacker
+    /* The logic for caching the more recent response is handled in
-     * controlled. The attacker can have chosen any valid OCSP response,
+     * ocsp_CreateOrUpdateCacheEntry, which is called by this function. */
-     * including responses from the past. In this case,
+    rv = ocsp_CacheEncodedOCSPResponse(handle, certID, cert, time,
-     * ocsp_GetVerifiedSingleResponseForCertID will fail. If we recorded a
+                                       pwArg, encodedResponse,
-     * negative cache entry in this case, then the attacker would have
+                                       PR_FALSE /* don't cache if invalid */,
-     * 'poisoned' our cache (denial of service), so we don't record negative
+                                       &certIDWasConsumed,
-     * results. */
-    rv = ocsp_CacheEncodedOCSPResponse(handle, certID, cert, time, pwArg,
-                                       encodedResponse, &certIDWasConsumed,
-                                       PR_FALSE /* don't cache failures */,
                                        &rvOcsp);
     if (!certIDWasConsumed) {
         CERT_DestroyOCSPCertID(certID);
@@ -4936,8 +5050,9 @@ ocsp_GetOCSPStatusFromNetwork(CERTCertDBHandle *handle,
     }
 
     rv = ocsp_CacheEncodedOCSPResponse(handle, certID, cert, time, pwArg,
-	                               encodedResponse, certIDWasConsumed,
+                                       encodedResponse,
-	                               PR_TRUE /* cache failures */, rv_ocsp);
+                                       PR_TRUE /* cache if invalid */,
+                                       certIDWasConsumed, rv_ocsp);
 
 loser:
     if (request != NULL)
@@ -4975,6 +5090,9 @@ loser:
  *     the opaque argument to the password prompting function.
  *   SECItem *encodedResponse
  *     the DER encoded bytes of the OCSP response
+ *   PRBool cacheInvalid
+ *     If true then invalid responses will cause a negative cache entry to be
+ *     created. (Invalid means bad syntax, bad signature etc)
  *   PRBool *certIDWasConsumed
  *     (output) on return, this is true iff |certID| was consumed by this
  *     function.
@@ -4990,9 +5108,9 @@ ocsp_CacheEncodedOCSPResponse(CERTCertDBHandle *handle,
 			      CERTCertificate *cert,
 			      int64 time,
 			      void *pwArg,
-			      SECItem *encodedResponse,
+			      const SECItem *encodedResponse,
+                              PRBool cacheInvalid,
 			      PRBool *certIDWasConsumed,
-			      PRBool cacheNegative,
 			      SECStatus *rv_ocsp)
 {
     CERTOCSPResponse *response = NULL;
@@ -5051,7 +5169,8 @@ ocsp_CacheEncodedOCSPResponse(CERTCertDBHandle *handle,
     *rv_ocsp = ocsp_SingleResponseCertHasGoodStatus(single, time);
 
 loser:
-    if (cacheNegative || *rv_ocsp == SECSuccess) {
+    /* If single == NULL here then the response was invalid. */
+    if (single != NULL || cacheInvalid) {
 	PR_EnterMonitor(OCSP_Global.monitor);
 	if (OCSP_Global.maxCacheEntries >= 0) {
 	    /* single == NULL means: remember response failure */

mozilla/security/nss/lib/certhigh/ocsp.h

@@ -5,7 +5,7 @@
 /*
  * Interface to the OCSP implementation.
  *
- * $Id: ocsp.h,v 1.24 2012-12-12 16:03:44 wtc%google.com Exp $
+ * $Id: ocsp.h,v 1.25 2013-02-15 17:53:24 kaie%kuix.de Exp $
  */
 
 #ifndef _OCSP_H_
@@ -300,7 +300,7 @@ CERT_DestroyOCSPRequest(CERTOCSPRequest *request);
  *   or a low-level or internal error occurred).
  */
 extern CERTOCSPResponse *
-CERT_DecodeOCSPResponse(SECItem *src);
+CERT_DecodeOCSPResponse(const SECItem *src);
 
 /*
  * FUNCTION: CERT_DestroyOCSPResponse
@@ -551,7 +551,7 @@ extern SECStatus
 CERT_CacheOCSPResponseFromSideChannel(CERTCertDBHandle *handle,
 				      CERTCertificate *cert,
 				      PRTime time,
-				      SECItem *encodedResponse,
+				      const SECItem *encodedResponse,
 				      void *pwArg);
 
 /*

mozilla/security/nss/lib/ssl/sslauth.c

@@ -1,13 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-/* $Id: sslauth.c,v 1.18 2012-04-25 14:50:12 gerv%gerv.net Exp $ */
+/* $Id: sslauth.c,v 1.19 2013-02-15 17:53:24 kaie%kuix.de Exp $ */
 #include "cert.h"
 #include "secitem.h"
 #include "ssl.h"
 #include "sslimpl.h"
 #include "sslproto.h"
 #include "pk11func.h"
+#include "ocsp.h"
 
 /* NEED LOCKS IN HERE.  */
 CERTCertificate *
@@ -214,6 +215,7 @@ SSL_AuthCertificate(void *arg, PRFileDesc *fd, PRBool checkSig, PRBool isServer)
     sslSocket *        ss;
     SECCertUsage       certUsage;
     const char *             hostname    = NULL;
+    PRTime             now = PR_Now();
     
     ss = ssl_FindSocket(fd);
     PORT_Assert(ss != NULL);
@@ -223,11 +225,20 @@ SSL_AuthCertificate(void *arg, PRFileDesc *fd, PRBool checkSig, PRBool isServer)
 
     handle = (CERTCertDBHandle *)arg;
 
+    if (ss->sec.ci.sid->peerCertStatus.len) {
+        unsigned int i;
+        CERT_CacheOCSPResponseFromSideChannel(handle,
+                                              ss->sec.peerCert,
+                                              now,
+                                              &ss->sec.ci.sid->peerCertStatus,
+                                              arg);
+    }
+
     /* this may seem backwards, but isn't. */
     certUsage = isServer ? certUsageSSLClient : certUsageSSLServer;
 
-    rv = CERT_VerifyCertNow(handle, ss->sec.peerCert, checkSig, certUsage,
+    rv = CERT_VerifyCert(handle, ss->sec.peerCert, checkSig, certUsage,
-			    ss->pkcs11PinArg);
+			 now, ss->pkcs11PinArg, NULL);
 
     if ( rv != SECSuccess || isServer )
 	return rv;