Address post-review feedback for r81702

net/base/x509_certificate_mac.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/x509_certificate.h"
 
 #include <CommonCrypto/CommonDigest.h>
 #include <CoreServices/CoreServices.h>
 #include <Security/Security.h>
 #include <time.h>
@@ skipping 271 matching lines @@
     err = SecPolicySetValue(*policy, &options_data);
     if (err) {
       CFRelease(*policy);
       return err;
     }
   }
   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
+// validate a certificate for an SSL server. |hostname| contains the name of
+// the SSL server that the certificate should be verified against. |flags| is
 // 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) {
+  ScopedCFTypeRef<CFMutableArrayRef> local_policies(
+      CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
+  if (!local_policies)
+    return memFullErr;
+
   // 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);
+  CFArrayAppendValue(local_policies, ssl_policy);
+  CFRelease(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.
+  // Manually add revocation policies. In order to actually disable revocation
+  // checking, the SecTrustRef must have at least one revocation policy
+  // associated with it. If none are present, the Apple TP will add policies
+  // according to the system preferences, which will enable revocation
+  // checking even if the caller explicitly disabled it. An OCSP policy is
+  // used, rather than a CRL policy, because the Apple TP will force an OCSP
+  // policy to be present and enabled if it believes the certificate may chain
+  // to an EV root. By explicitly disabling network and OCSP cache access,
+  // then even if the Apple TP enables OCSP checking, no revocation checking
+  // will actually succeed.
   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.
+    // The default for the OCSP policy is to fetch responses via the network,
+    // unlike the CRL policy default. The policy is further modified to
+    // prefer OCSP over CRLs, if both are specified on the certificate. This
+    // is because an OCSP response is both sufficient and typically
+    // significantly smaller than the CRL counterpart.
     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.
-    tp_ocsp_options.Flags = CSSM_TP_ACTION_OCSP_DISABLE_NET;
-    // The default CrlFlags will ensure only cached CRLs are used.
+    // Effectively disable OCSP checking by making it impossible to get an
+    // OCSP response. Even if the Apple TP forces OCSP, no checking will
+    // be able to succeed. If this happens, the Apple TP will report an error
+    // that OCSP was unavailable, but this will be handled and suppressed in
+    // X509Certificate::Verify().
+    tp_ocsp_options.Flags = CSSM_TP_ACTION_OCSP_DISABLE_NET |
+                            CSSM_TP_ACTION_OCSP_CACHE_READ_DISABLE;
   }
 
   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);
+  CFArrayAppendValue(local_policies, ocsp_policy);
+  CFRelease(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);
+  if (flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED) {
+    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;
+    tp_crl_options.CrlFlags = CSSM_TP_ACTION_FETCH_CRL_FROM_NET;
 
-  CFTypeRef local_policies[] = { ssl_policy, ocsp_policy, crl_policy };
-  CFArrayRef policy_array = CFArrayCreate(kCFAllocatorDefault, local_policies,
-                                          arraysize(local_policies),
-                                          &kCFTypeArrayCallBacks);
-  if (!policy_array)
-    return memFullErr;
+    SecPolicyRef crl_policy;
+    status = CreatePolicy(&CSSMOID_APPLE_TP_REVOCATION_CRL, &tp_crl_options,
+                          sizeof(tp_crl_options), &crl_policy);
+    if (status)
+      return status;
+    CFArrayAppendValue(local_policies, crl_policy);
+    CFRelease(crl_policy);
+  }
 
-  policies->reset(policy_array);
+  policies->reset(local_policies.release());
   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
 // result: it shouldn't be used to determine trust, just to traverse the chain.
 // Caller is responsible for releasing the value stored into *out_cert_chain.
 OSStatus CopyCertChain(SecCertificateRef cert_handle,
                        CFArrayRef* out_cert_chain) {
@@ skipping 473 matching lines @@
     // 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.
     tp_action_data.ActionFlags |= CSSM_TP_ACTION_REQUIRE_REV_PER_CERT;
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
   } 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
+    // so as to attempt OCSP checking if it believes a certificate may chain
     // 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);
@@ skipping 466 matching lines @@
   CSSM_DATA cert_data;
   OSStatus status = SecCertificateGetData(cert_handle, &cert_data);
   if (status)
     return false;
 
   return pickle->WriteData(reinterpret_cast<char*>(cert_data.Data),
                            cert_data.Length);
 }
 
 }  // namespace net