Allow revocation checking to be disabled on Mac, overriding/ignoring system settings

net/base/x509_certificate_mac.cc

Index: net/base/x509_certificate_mac.cc
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index a60b2409fc123e8996cd74d2436e474ccc255879..033ddbf4a6ce64f5c51b7d470512412496dfc654 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -5,6 +5,7 @@
 #include "net/base/x509_certificate.h"
 
 #include <CommonCrypto/CommonDigest.h>
+#include <CoreServices/CoreServices.h>
 #include <Security/Security.h>
 #include <time.h>
 
@@ -26,6 +27,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/test_root_certs.h"
 #include "net/base/x509_certificate_known_roots_mac.h"
+#include "third_party/apple_apsl/cssmapplePriv.h"
 #include "third_party/nss/mozilla/security/nss/lib/certdb/cert.h"
 
 using base::mac::ScopedCFTypeRef;
@@ -286,6 +288,84 @@ OSStatus CreatePolicy(const CSSM_OID* policy_OID,
   return noErr;
 }
 
+// Creates a series of SecPolicyRefs to be added to a SecTrustRef used to
+// validate a certificate for an SSL peer. |hostname| contains the name of
+// the SSL peer that the certificate should be verified against. |flags| is

[wtc, 2011/04/20 19:40:33]

Nit: should we change "SSL peer" to "SSL server"?

+// a bitwise-OR of VerifyFlags that can further alter how trust is
+// validated, such as how revocation is checked. If successful, returns
+// noErr, and stores the resultant array of SecPolicyRefs in |policies|.
+OSStatus CreateTrustPolicies(const std::string& hostname, int flags,
+                             ScopedCFTypeRef<CFArrayRef>* policies) {
+  // Create an SSL SecPolicyRef, and configure it to perform hostname
+  // validation. The hostname check does 99% of what we want, with the
+  // exception of dotted IPv4 addreses, which we handle ourselves below.
+  CSSM_APPLE_TP_SSL_OPTIONS tp_ssl_options = {
+    CSSM_APPLE_TP_SSL_OPTS_VERSION,
+    hostname.size(),
+    hostname.data(),
+    0
+  };
+  SecPolicyRef ssl_policy;
+  OSStatus status = CreatePolicy(&CSSMOID_APPLE_TP_SSL, &tp_ssl_options,
+                                 sizeof(tp_ssl_options), &ssl_policy);
+  if (status)
+    return status;
+  ScopedCFTypeRef<SecPolicyRef> scoped_ssl_policy(ssl_policy);
+
+  // Manually add OCSP and CRL policies. If neither an OCSP or CRL policy is
+  // specified, the Apple TP module will add whatever the system settings
+  // are, which is not desirable here.
+  //
+  // Note that this causes any locally configured OCSP responder URL to be
+  // ignored.

[wtc, 2011/04/20 19:40:33]

Is the locally configured OCSP responder URL a system setting?
If so, we don't need to mention that here because we no longer
honor the system revocation settings.

Note that NSS supports a locally configured OCSP responder
URL, and Firefox exposes that in its UI.  I don't know how
many Firefox users use that feature.

[Ryan Sleevi, 2011/04/20 22:46:51]

It is a system setting. I documented this because of the difference between the
Windows implementation.

On Windows, revocation checking is handled by whichever DLLs have registered as
the CryptoAPI CertDllVerifyRevocation implementation. Any settings (including
settings for the default implementation, cryptnet.dll), are respected. The only
setting we don't respect with regards to system (IE) settings is whether or not
to check revocation.

Here, on Mac, we don't respect /any/ system settings - whether or not to check
OCSP settings and whether or not to use a local responder. So there's that
difference.

I've gone and removed the comment though, to avoid confusion.

+  CSSM_APPLE_TP_OCSP_OPTIONS tp_ocsp_options;
+  memset(&tp_ocsp_options, 0, sizeof(tp_ocsp_options));
+  tp_ocsp_options.Version = CSSM_APPLE_TP_OCSP_OPTS_VERSION;
+
+  CSSM_APPLE_TP_CRL_OPTIONS tp_crl_options;
+  memset(&tp_crl_options, 0, sizeof(tp_crl_options));
+  tp_crl_options.Version = CSSM_APPLE_TP_CRL_OPTS_VERSION;
+
+  if (flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED) {
+    // If an OCSP responder is available, use it, and avoid fetching any
+    // CRLs for that certificate if possible, as they may be much larger.
+    tp_ocsp_options.Flags = CSSM_TP_ACTION_OCSP_SUFFICIENT;
+    // Ensure that CRLs can be fetched if a crlDistributionPoint extension
+    // is found. Otherwise, only the local CRL cache will be consulted.
+    tp_crl_options.CrlFlags |= CSSM_TP_ACTION_FETCH_CRL_FROM_NET;
+  } else {
+    // Disable OCSP network fetching, but still permit cached OCSP responses
+    // to be used. This is equivalent to the Windows code's usage of
+    // CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY.

[wtc, 2011/04/20 19:40:33]

Ideally we want to not do any revocation checking in this case.

I got the CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY flag either
from some SDK sample code or at the suggestion of a code
reviewer.  Since I wasn't familiar with CryptoAPI's certificate
verification code, I used the flag.

[Ryan Sleevi, 2011/04/20 22:46:51]

Really? This surprises me a little. I thought the reason for disabling
revocation checking is if a user wishes to avoid potentially privacy-leaking
events (checking for CRLs, pinging an OCSP responder, potentially downloading an
AIA cert...). 

On Windows, and here, if a known-bad response is available, we still respect it
even when revocation checking is disabled. A known bad response can be obtained
by previously having checking enabled OR, at least in the case of Windows
(though I believe with Mac as well), by a system administrator pushing out a
local/organizational CRL revoking one or more certificates (via
HKLM\Software\Policies\Microsoft\SystemCertificates)

I just want to confirm your intent here is that revocation checking /fully/
disables revocation checking, and that known-revoked certificates (potentially
including the Comodo certificates) will be accepted as valid here.

[wtc, 2011/04/20 23:23:54]

Yes.  That's what I expect if I uncheck the "Check for
server certificate revocation" checkbox in our Options UI.
I believe a user unfamiliar with how revocation checking
is done will expect the same thing.

Does IE not behave this way?  Can you test the URL
https://test-ssev.verisign.com:2443/test-SSEV-revoked-verisign.html ?
(I don't have access to a Windows computer now.)  Thanks.

Note: Safari and Mac Chrome do behave this way as you
described.

In any case, either behavior is acceptable, so no need to
change this.

+    tp_ocsp_options.Flags = CSSM_TP_ACTION_OCSP_DISABLE_NET;
+    // The default CrlFlags will ensure only cached CRLs are used.
+  }
+
+  SecPolicyRef ocsp_policy;
+  status = CreatePolicy(&CSSMOID_APPLE_TP_REVOCATION_OCSP, &tp_ocsp_options,
+                        sizeof(tp_ocsp_options), &ocsp_policy);
+  if (status)
+    return status;
+  ScopedCFTypeRef<SecPolicyRef> scoped_ocsp_policy(ocsp_policy);
+
+  SecPolicyRef crl_policy;
+  status = CreatePolicy(&CSSMOID_APPLE_TP_REVOCATION_CRL, &tp_crl_options,
+                        sizeof(tp_crl_options), &crl_policy);
+  if (status)
+    return status;
+  ScopedCFTypeRef<SecPolicyRef> scoped_crl_policy(crl_policy);
+
+  CFTypeRef local_policies[] = { ssl_policy, ocsp_policy, crl_policy };
+  CFArrayRef policy_array = CFArrayCreate(kCFAllocatorDefault, local_policies,
+                                          arraysize(local_policies),
+                                          &kCFTypeArrayCallBacks);

[wtc, 2011/04/20 19:40:33]

Just wanted to confirm that CFArrayCreate will retain
all the CFTypeRefs in local_policies.

[Ryan Sleevi, 2011/04/20 22:46:51]

Yes, per the documentation for the |callBacks| parameter at 
http://developer.apple.com/library/mac/#documentation/CoreFoundation/Referenc...

"The retain callback is used within this function, for example, to retain all of
the new values from the values C array. "

+  if (!policy_array)
+    return memFullErr;
+
+  policies->reset(policy_array);
+  return noErr;
+}
+
 // Gets the issuer for a given cert, starting with the cert itself and
 // including the intermediate and finally root certificates (if any).
 // This function calls SecTrust but doesn't actually pay attention to the trust
@@ -734,23 +814,10 @@ int X509Certificate::Verify(const std::string& hostname, int flags,
     return ERR_CERT_REVOKED;
   }
 
-  // Create an SSL SecPolicyRef, and configure it to perform hostname
-  // validation. The hostname check does 99% of what we want, with the
-  // exception of dotted IPv4 addreses, which we handle ourselves below.
-  CSSM_APPLE_TP_SSL_OPTIONS tp_ssl_options = {
-    CSSM_APPLE_TP_SSL_OPTS_VERSION,
-    hostname.size(),
-    hostname.data(),
-    0
-  };
-  SecPolicyRef ssl_policy;
-  OSStatus status = CreatePolicy(&CSSMOID_APPLE_TP_SSL,
-                                 &tp_ssl_options,
-                                 sizeof(tp_ssl_options),
-                                 &ssl_policy);
+  ScopedCFTypeRef<CFArrayRef> trust_policies;
+  OSStatus status = CreateTrustPolicies(hostname, flags, &trust_policies);
   if (status)
     return NetErrorFromOSStatus(status);
-  ScopedCFTypeRef<SecPolicyRef> scoped_ssl_policy(ssl_policy);
 
   // Create and configure a SecTrustRef, which takes our certificate(s)
   // and our SSL SecPolicyRef. SecTrustCreateWithCertificates() takes an
@@ -773,7 +840,8 @@ int X509Certificate::Verify(const std::string& hostname, int flags,
   base::AutoLock lock(verification_lock_);
 
   SecTrustRef trust_ref = NULL;
-  status = SecTrustCreateWithCertificates(cert_array, ssl_policy, &trust_ref);
+  status = SecTrustCreateWithCertificates(cert_array, trust_policies,
+                                          &trust_ref);
   if (status)
     return NetErrorFromOSStatus(status);
   ScopedCFTypeRef<SecTrustRef> scoped_trust_ref(trust_ref);
@@ -784,34 +852,51 @@ int X509Certificate::Verify(const std::string& hostname, int flags,
       return NetErrorFromOSStatus(status);
   }
 
+  CSSM_APPLE_TP_ACTION_DATA tp_action_data;
+  memset(&tp_action_data, 0, sizeof(tp_action_data));
+  tp_action_data.Version = CSSM_APPLE_TP_ACTION_VERSION;
+  // Allow CSSM to download any missing intermediate certificates if an
+  // authorityInfoAccess extension or issuerAltName extension is present.
+  tp_action_data.ActionFlags = CSSM_TP_ACTION_FETCH_CERT_FROM_NET;

[wtc, 2011/04/20 19:40:33]

Good change!  I didn't know CSSM can do this.  I wonder why
Safari doesn't enable this feature.

+
   if (flags & VERIFY_REV_CHECKING_ENABLED) {
-    // When called with VERIFY_REV_CHECKING_ENABLED, we ask SecTrustEvaluate()
-    // to apply OCSP and CRL checking, but we're still subject to the global
-    // settings, which are configured in the Keychain Access application (in
-    // the Certificates tab of the Preferences dialog). If the user has
-    // revocation disabled (which is the default), then we will get
-    // kSecTrustResultRecoverableTrustFailure back from SecTrustEvaluate()
-    // with one of a number of sub error codes indicating that revocation
-    // checking did not occur. In that case, we'll set our own result to include
+    // Require a positive result from an OCSP responder or a CRL (or both)
+    // for every certificate in the chain. The Apple TP automatically
+    // excludes the self-signed root from this requirement. If a certificate
+    // is missing both a crlDistributionPoints extension and an
+    // authorityInfoAccess extension with an OCSP responder URL, then we
+    // will get a kSecTrustResultRecoverableTrustFailure back from
+    // SecTrustEvaluate(), with a
+    // CSSMERR_APPLETP_INCOMPLETE_REVOCATION_CHECK error code. In that case,
+    // we'll set our own result to include
+    // CERT_STATUS_NO_REVOCATION_MECHANISM. If one or both extensions are
+    // present, and a check fails (server unavailable, OCSP retry later,
+    // signature mismatch), then we'll set our own result to include
     // CERT_STATUS_UNABLE_TO_CHECK_REVOCATION.
-    //
-    // NOTE: This does not apply to EV certificates, which always get
-    // revocation checks regardless of the global settings.
+    tp_action_data.ActionFlags |= CSSM_TP_ACTION_REQUIRE_REV_PER_CERT;
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
-    CSSM_APPLE_TP_ACTION_DATA tp_action_data = { CSSM_APPLE_TP_ACTION_VERSION };
-    tp_action_data.ActionFlags = CSSM_TP_ACTION_REQUIRE_REV_PER_CERT;
-    CFDataRef action_data_ref =
-        CFDataCreate(NULL, reinterpret_cast<UInt8*>(&tp_action_data),
-                     sizeof(tp_action_data));
-    if (!action_data_ref)
-      return ERR_OUT_OF_MEMORY;
-    ScopedCFTypeRef<CFDataRef> scoped_action_data_ref(action_data_ref);
-    status = SecTrustSetParameters(trust_ref, CSSM_TP_ACTION_DEFAULT,
-                                   action_data_ref);
-    if (status)
-      return NetErrorFromOSStatus(status);
+  } else {
+    // EV requires revocation checking.
+    // Note, under the hood, SecTrustEvaluate() will modify the OCSP options
+    // so as to attempt OCSP fetching if it believes a certificate may chain

[wtc, 2011/04/20 19:40:33]

Nit: OCSP fetching => OCSP checking

"fetching" implies fetching from the network to me.

+    // to an EV root. However, because network fetches are disabled in
+    // CreateTrustPolicies() when revocation checking is disabled, these
+    // will only go against the local cache.
+    flags &= ~VERIFY_EV_CERT;
   }
 
+  CFDataRef action_data_ref =
+      CFDataCreateWithBytesNoCopy(kCFAllocatorDefault,
+                                  reinterpret_cast<UInt8*>(&tp_action_data),
+                                  sizeof(tp_action_data), kCFAllocatorNull);
+  if (!action_data_ref)
+    return ERR_OUT_OF_MEMORY;
+  ScopedCFTypeRef<CFDataRef> scoped_action_data_ref(action_data_ref);
+  status = SecTrustSetParameters(trust_ref, CSSM_TP_ACTION_DEFAULT,
+                                 action_data_ref);
+  if (status)
+    return NetErrorFromOSStatus(status);
+
   // Verify the certificate. A non-zero result from SecTrustGetResult()
   // indicates that some fatal error occurred and the chain couldn't be
   // processed, not that the chain contains no errors. We need to examine the