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) {
+  CFMutableArrayRef local_policies =
+      CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks);
+  if (!local_policies)
+    return memFullErr;
+  ScopedCFTypeRef<CFMutableArrayRef> scoped_local_policies(local_policies);

[wtc, 2011/04/21 22:04:38]

Please rewrite this as:
  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(scoped_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. If no revocation policy is specified,
+  // the Apple TP module will add whatever the system settings are, which is
+  // not desirable here.

[wtc, 2011/04/21 22:04:38]

It would be nice to stress that we need to specify a revocation
policy (an OCSP policy that suppresses access to both
network and cache) to disable revocation checking.

[Ryan Sleevi, 2011/04/21 23:59:17]

Wording has been updated.

   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.
-    tp_ocsp_options.Flags = CSSM_TP_ACTION_OCSP_DISABLE_NET;
-    // The default CrlFlags will ensure only cached CRLs are used.
+    // If the Apple TP believes the certificate being verified may be an EV
+    // certificate, it will attempt to force OCSP checking. In order to
+    // reliably disable revocation checking, disable access to both the
+    // network and the cache. Note that when this happens, the Apple TP will
+    // report an error error that OCSP was unavailable, but this will be

[wtc, 2011/04/21 22:04:38]

Typo: error error => error

+    // 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(scoped_local_policies, ocsp_policy);
+  CFRelease(ocsp_policy);
+
+  if (!(flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED)) {
+    policies->reset(scoped_local_policies.release());
+    return noErr;

[wtc, 2011/04/21 22:04:38]

Nit: you can put lines 357-370 inside a
  if (flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED) {
  }
block, then we only need one instance of
  policies->reset(scoped_local_policies.release());
  return noErr;
at the end of this function.

+  }
+
+  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;
+  // 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;
 
   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);
+  CFArrayAppendValue(scoped_local_policies, crl_policy);
+  CFRelease(crl_policy);
 
-  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;
-
-  policies->reset(policy_array);
+  policies->reset(scoped_local_policies.release());

[wtc, 2011/04/21 22:04:38]

We can use 'swap' here.  Would that be better?

[Ryan Sleevi, 2011/04/21 23:59:17]

We can't. ScopedCFType<CFArrayRef> is a different type than
ScopedCFType<CFMutableArrayRef>, even though the two CF types are exchangeable. 

   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