bug 950240 - don't do DV fallback for nsIIdentityInfo.isExtendedValidation r=briansmith
authorDavid Keeler <dkeeler@mozilla.com>
Fri, 17 Jan 2014 11:04:09 -0800
changeset 163986 b887641e3983
parent 163985 ea1d1a255701
child 163987 b068b0194348
push id38611
push userdkeeler@mozilla.com
push dateFri, 17 Jan 2014 19:07:06 +0000
treeherdermozilla-inbound@b887641e3983 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbriansmith
bugs950240
milestone29.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 950240 - don't do DV fallback for nsIIdentityInfo.isExtendedValidation r=briansmith
security/manager/ssl/public/nsIX509CertDB.idl
security/manager/ssl/src/CertVerifier.cpp
security/manager/ssl/src/CertVerifier.h
security/manager/ssl/src/nsIdentityChecking.cpp
security/manager/ssl/src/nsNSSCertificateDB.cpp
security/manager/ssl/tests/unit/test_ev_certs.js
--- a/security/manager/ssl/public/nsIX509CertDB.idl
+++ b/security/manager/ssl/public/nsIX509CertDB.idl
@@ -26,17 +26,17 @@ interface nsIOpenSignedJARFileCallback :
                                  in nsIZipReader aZipReader,
                                  in nsIX509Cert3 aSignerCert);
 };
 
 /**
  * This represents a service to access and manipulate 
  * X.509 certificates stored in a database.
  */
-[scriptable, uuid(3c2a5658-466a-11e3-a244-180373d97f23)]
+[scriptable, uuid(6502291d-4477-43a3-9688-4b453d055c1d)]
 interface nsIX509CertDB : nsISupports {
 
   /**
    *  Constants that define which usages a certificate
    *  is trusted for.
    */
   const unsigned long UNTRUSTED       =      0;
   const unsigned long TRUSTED_SSL     = 1 << 0;
@@ -295,33 +295,41 @@ interface nsIX509CertDB : nsISupports {
    *
    * @param certDER The raw DER encoding of a certificate.
    * @param aTrust decoded by CERT_DecodeTrustString. 3 comma separated characters,
    *                indicating SSL, Email, and Obj signing trust
    * @param aName name of the cert for display purposes.
    */
   void addCert(in ACString certDER, in string aTrust, in string aName);
 
+  // Flags for verifyCertNow (these must match the values in CertVerifier.cpp):
+  // Prevent network traffic. Doesn't work with classic verification.
+  const uint32_t FLAG_LOCAL_ONLY = 1 << 0;
+  // Do not fall back to DV verification after attempting EV validation.
+  // Actually does prevent network traffic, but can cause a valid EV
+  // certificate to not be considered valid.
+  const uint32_t FLAG_NO_DV_FALLBACK_FOR_EV = 1 << 1;
+
   /** Warning: This interface is inteded to use only for testing only as:
    *    1. It can create IO on the main thread.
    *    2. It is in constant change, so in/out can change at any release.
    *
    *  Obtain the verification result for a cert given a particular usage.
    *  On success, the call returns 0, the chain built during verification,
    *  and whether the cert is good for EV usage.
    *  On failure, the call returns the PRErrorCode for the verification failure
    *
    *  @param aCert Obtain the stored trust of this certificate
    *  @param aUsage a integer representing the usage from NSS
-   *  @param aLocalOnly prevent network activity for revocation
+   *  @param aFlags flags as described above
    *  @param verifedChain chain of verification up to the root if success
    *  @param aHasEVPolicy bool that signified that the cert was an EV cert
    *  @return 0 if success or the value or the error code for the verification
    *          failure
    */
   int32_t /*PRErrorCode*/
     verifyCertNow(in nsIX509Cert aCert,
                   in int64_t /*SECCertificateUsage*/ aUsage,
-                  in bool aLocalOnly,
+                  in uint32_t aFlags,
                   out nsIX509CertList verifiedChain,
                   out bool aHasEVPolicy);
 
 };
--- a/security/manager/ssl/src/CertVerifier.cpp
+++ b/security/manager/ssl/src/CertVerifier.cpp
@@ -14,16 +14,17 @@ extern PRLogModuleInfo* gPIPNSSLog;
 #endif
 
 namespace mozilla { namespace psm {
 
 extern SECStatus getFirstEVPolicy(CERTCertificate *cert, SECOidTag &outOidTag);
 extern CERTCertList* getRootsForOid(SECOidTag oid_tag);
 
 const CertVerifier::Flags CertVerifier::FLAG_LOCAL_ONLY = 1;
+const CertVerifier::Flags CertVerifier::FLAG_NO_DV_FALLBACK_FOR_EV = 2;
 
 CertVerifier::CertVerifier(missing_cert_download_config mcdc,
                            crl_download_config cdc,
                            ocsp_download_config odc,
                            ocsp_strict_config osc,
                            ocsp_get_config ogc)
   : mMissingCertDownloadEnabled(mcdc == missing_cert_download_on)
   , mCRLDownloadEnabled(cdc == crl_download_allowed)
@@ -151,16 +152,19 @@ CertVerifier::VerifyCert(CERTCertificate
       return SECFailure;
   }
 
   ScopedCERTCertList trustAnchors;
   SECStatus rv;
   SECOidTag evPolicy = SEC_OID_UNKNOWN;
 
 #ifdef NSS_NO_LIBPKIX
+  if (flags & FLAG_NO_DV_FALLBACK_FOR_EV) {
+    return SECSuccess;
+  }
   return ClassicVerifyCert(cert, usage, time, pinArg, validationChain,
                            verifyLog);
 #else
   // Do EV checking only for sslserver usage
   if (usage == certificateUsageSSLServer) {
     SECStatus srv = getFirstEVPolicy(cert, evPolicy);
     if (srv == SECSuccess) {
       if (evPolicy != SEC_OID_UNKNOWN) {
@@ -310,16 +314,22 @@ CertVerifier::VerifyCert(CERTCertificate
       }
       verifyLog->count = 0;
       verifyLog->head = nullptr;
       verifyLog->tail = nullptr;
     }
 
   }
 
+  // If we're here, PKIX EV verification failed.
+  // If requested, don't do DV fallback.
+  if (flags & FLAG_NO_DV_FALLBACK_FOR_EV) {
+    return SECSuccess;
+  }
+
   if (!nsNSSComponent::globalConstFlagUsePKIXVerification){
     // XXX: we do not care about the localOnly flag (currently) as the
     // caller that wants localOnly should disable and reenable the fetching.
     return ClassicVerifyCert(cert, usage, time, pinArg, validationChain,
                              verifyLog);
   }
 
   // The current flags check the chain the same way as the leafs
--- a/security/manager/ssl/src/CertVerifier.h
+++ b/security/manager/ssl/src/CertVerifier.h
@@ -16,18 +16,20 @@ class nsNSSComponent;
 namespace mozilla { namespace psm {
 
 class CertVerifier
 {
 public:
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CertVerifier)
 
   typedef unsigned int Flags;
+  // XXX: FLAG_LOCAL_ONLY is ignored in the classic verification case
   static const Flags FLAG_LOCAL_ONLY;
-  // XXX: The localonly flag is ignored in the classic verification case
+  // Don't perform fallback DV validation on EV validation failure.
+  static const Flags FLAG_NO_DV_FALLBACK_FOR_EV;
 
   // *evOidPolicy == SEC_OID_UNKNOWN means the cert is NOT EV
   // Only one usage per verification is supported.
   SECStatus VerifyCert(CERTCertificate * cert,
                        const SECCertificateUsage usage,
                        const PRTime time,
                        nsIInterfaceRequestor * pinArg,
                        const Flags flags = 0,
--- a/security/manager/ssl/src/nsIdentityChecking.cpp
+++ b/security/manager/ssl/src/nsIdentityChecking.cpp
@@ -1282,20 +1282,22 @@ nsNSSCertificate::hasValidEVOidTag(SECOi
   nssComponent->EnsureIdentityInfoLoaded();
 
   RefPtr<mozilla::psm::CertVerifier> certVerifier(mozilla::psm::GetDefaultCertVerifier());
   NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
 
   validEV = false;
   resultOidTag = SEC_OID_UNKNOWN;
 
+  uint32_t flags = mozilla::psm::CertVerifier::FLAG_LOCAL_ONLY |
+                   mozilla::psm::CertVerifier::FLAG_NO_DV_FALLBACK_FOR_EV;
   SECStatus rv = certVerifier->VerifyCert(mCert,
                                           certificateUsageSSLServer, PR_Now(),
                                           nullptr /* XXX pinarg*/,
-                                          0, nullptr, &resultOidTag);
+                                          flags, nullptr, &resultOidTag);
 
   if (rv != SECSuccess) {
     resultOidTag = SEC_OID_UNKNOWN;
   }
   if (resultOidTag != SEC_OID_UNKNOWN) {
     validEV = true;
   }
   return NS_OK;
--- a/security/manager/ssl/src/nsNSSCertificateDB.cpp
+++ b/security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ -1674,17 +1674,17 @@ nsNSSCertificateDB::GetRecentBadCerts(bo
     NS_ADDREF(*result = mPublicRecentBadCerts);
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNSSCertificateDB::VerifyCertNow(nsIX509Cert* aCert,
                                   int64_t /*SECCertificateUsage*/ aUsage,
-                                  bool aLocalOnly,
+                                  uint32_t aFlags,
                                   nsIX509CertList** verifiedChain,
                                   bool* aHasEVPolicy,
                                   int32_t* /*PRErrorCode*/ _retval )
 {
   NS_ENSURE_ARG_POINTER(aCert);
   NS_ENSURE_ARG_POINTER(aHasEVPolicy);
   NS_ENSURE_ARG_POINTER(verifiedChain);
   NS_ENSURE_ARG_POINTER(_retval);
@@ -1711,25 +1711,24 @@ nsNSSCertificateDB::VerifyCertNow(nsIX50
   if (!x509Cert) {
     return NS_ERROR_INVALID_ARG;
   }
   ScopedCERTCertificate nssCert(x509Cert->GetCert());
 
   RefPtr<CertVerifier> certVerifier(GetDefaultCertVerifier());
   NS_ENSURE_TRUE(certVerifier, NS_ERROR_FAILURE);
 
-  CertVerifier::Flags flags = aLocalOnly ? CertVerifier::FLAG_LOCAL_ONLY : 0;
   CERTCertList* resultChain = nullptr;
   SECOidTag evOidPolicy;
   SECStatus srv;
 
   srv = certVerifier->VerifyCert(nssCert,
                                  aUsage, PR_Now(),
                                  nullptr, // Assume no context
-                                 flags,
+                                 aFlags,
                                  &resultChain,
                                  &evOidPolicy,
                                  nullptr);
 
   PRErrorCode error = PR_GetError();
 
   nsCOMPtr<nsIX509CertList> nssCertList;
   // This adopts the list
--- a/security/manager/ssl/tests/unit/test_ev_certs.js
+++ b/security/manager/ssl/tests/unit/test_ev_certs.js
@@ -167,8 +167,44 @@ add_test(function() {
 
   clearOCSPCache();
   let ocspResponder = start_ocsp_responder(
                         isDebugBuild ? ["int-ev-valid", "ev-valid"]
                                      : ["ev-valid"]);
   check_ee_for_ev("ev-valid", isDebugBuild);
   ocspResponder.stop(run_next_test);
 });
+
+// bug 950240: add FLAG_NO_DV_FALLBACK_FOR_EV to CertVerifier::VerifyCert
+// to prevent spurious OCSP requests that race with OCSP stapling.
+// This has the side-effect of saying an EV certificate is not EV if
+// it hasn't already been verified (e.g. on the verification thread when
+// connecting to a site).
+function check_no_ocsp_requests(cert_name) {
+  clearOCSPCache();
+  let ocspResponder = failingOCSPResponder();
+  let cert = certdb.findCertByNickname(null, cert_name);
+  let hasEVPolicy = {};
+  let verifiedChain = {};
+  let flags = Ci.nsIX509CertDB.FLAG_LOCAL_ONLY |
+              Ci.nsIX509CertDB.FLAG_NO_DV_FALLBACK_FOR_EV;
+  let error = certdb.verifyCertNow(cert, certificateUsageSSLServer, flags,
+                                   verifiedChain, hasEVPolicy);
+  // Since we're not doing OCSP requests, no certificate will be EV.
+  do_check_eq(hasEVPolicy.value, false);
+  do_check_eq(0, error);
+  // Also check that isExtendedValidation doesn't cause OCSP requests.
+  let identityInfo = cert.QueryInterface(Ci.nsIIdentityInfo);
+  do_check_eq(identityInfo.isExtendedValidation, false);
+  ocspResponder.stop(run_next_test);
+}
+
+add_test(function() {
+  check_no_ocsp_requests("ev-valid");
+});
+
+add_test(function() {
+  check_no_ocsp_requests("non-ev-root");
+});
+
+add_test(function() {
+  check_no_ocsp_requests("no-ocsp-url-cert");
+});