CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path pruning loop.

net/cert/cert_verify_proc_mac.cc

 // Copyright (c) 2012 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/cert/cert_verify_proc_mac.h"
 
 #include <CommonCrypto/CommonDigest.h>
 #include <CoreServices/CoreServices.h>
 #include <Security/Security.h>
 
@@ skipping 10 matching lines @@
 #include "base/synchronization/lock.h"
 #include "crypto/mac_security_services_lock.h"
 #include "crypto/sha2.h"
 #include "net/base/hash_value.h"
 #include "net/base/net_errors.h"
 #include "net/cert/asn1_util.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/cert/crl_set.h"
+#include "net/cert/test_keychain_search_list_mac.h"
 #include "net/cert/test_root_certs.h"
 #include "net/cert/x509_certificate.h"
 #include "net/cert/x509_util_mac.h"
 
 // CSSM functions are deprecated as of OSX 10.7, but have no replacement.
 // https://bugs.chromium.org/p/chromium/issues/detail?id=590914#c1
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
 
 // From 10.7.2 libsecurity_keychain-55035/lib/SecTrustPriv.h, for use with
@@ skipping 100 matching lines @@
       // specific status for (such as basic constraints violation, or
       // unknown critical extension)
       OSSTATUS_LOG(WARNING, status)
           << "Unknown error mapped to CERT_STATUS_INVALID";
       return CERT_STATUS_INVALID;
     }
   }
 }
 
 // Creates a series of SecPolicyRefs to be added to a SecTrustRef used to
-// 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) {
+// validate a certificate for an SSL server. |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(int flags, ScopedCFTypeRef<CFArrayRef>* policies) {
   ScopedCFTypeRef<CFMutableArrayRef> local_policies(
       CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
   if (!local_policies)
     return memFullErr;
 
   SecPolicyRef ssl_policy;
-  OSStatus status = x509_util::CreateSSLServerPolicy(hostname, &ssl_policy);
+  OSStatus status =
+      x509_util::CreateSSLServerPolicy(std::string(), &ssl_policy);
   if (status)
     return status;
   CFArrayAppendValue(local_policies, ssl_policy);
   CFRelease(ssl_policy);
 
   // Explicitly add revocation policies, in order to override system
   // revocation checking policies and instead respect the application-level
   // revocation preference.
   status = x509_util::CreateRevocationPolicies(
       (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED),
@@ skipping 194 matching lines @@
 // failure, no output parameters are modified.
 //
 // Note: An OK return does not mean that |cert_array| is trusted, merely that
 // verification was performed successfully.
 //
 // This function should only be called while the Mac Security Services lock is
 // held.
 int BuildAndEvaluateSecTrustRef(CFArrayRef cert_array,
                                 CFArrayRef trust_policies,
                                 int flags,
+                                CFArrayRef keychain_search_list,
                                 ScopedCFTypeRef<SecTrustRef>* trust_ref,
                                 SecTrustResultType* trust_result,
                                 ScopedCFTypeRef<CFArrayRef>* verified_chain,
                                 CSSM_TP_APPLE_EVIDENCE_INFO** chain_info) {
   SecTrustRef tmp_trust = NULL;
   OSStatus status = SecTrustCreateWithCertificates(cert_array, trust_policies,
                                                    &tmp_trust);
   if (status)
     return NetErrorFromOSStatus(status);
   ScopedCFTypeRef<SecTrustRef> scoped_tmp_trust(tmp_trust);
 
   if (TestRootCerts::HasInstance()) {
     status = TestRootCerts::GetInstance()->FixupSecTrustRef(tmp_trust);
     if (status)
       return NetErrorFromOSStatus(status);
   }
 
+  if (keychain_search_list) {
+    status = SecTrustSetKeychains(tmp_trust, keychain_search_list);
+    if (status)
+      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 |
                                CSSM_TP_ACTION_TRUST_SETTINGS;
 
   // Note: For EV certificates, the Apple TP will handle setting these flags
   // as part of EV evaluation.
@@ skipping 126 matching lines @@
 
 int CertVerifyProcMac::VerifyInternal(
     X509Certificate* cert,
     const std::string& hostname,
     const std::string& ocsp_response,
     int flags,
     CRLSet* crl_set,
     const CertificateList& additional_trust_anchors,
     CertVerifyResult* verify_result) {
   ScopedCFTypeRef<CFArrayRef> trust_policies;
-  OSStatus status = CreateTrustPolicies(hostname, flags, &trust_policies);
+  OSStatus status = CreateTrustPolicies(flags, &trust_policies);
   if (status)
     return NetErrorFromOSStatus(status);
 
-  // Create and configure a SecTrustRef, which takes our certificate(s)
-  // and our SSL SecPolicyRef. SecTrustCreateWithCertificates() takes an
-  // array of certificates, the first of which is the certificate we're
-  // verifying, and the subsequent (optional) certificates are used for
-  // chain building.
-  ScopedCFTypeRef<CFMutableArrayRef> cert_array(
-      cert->CreateOSCertChainForCert());
-
   // Serialize all calls that may use the Keychain, to work around various
   // issues in OS X 10.6+ with multi-threaded access to Security.framework.
   base::AutoLock lock(crypto::GetMacSecurityServicesLock());
 
   ScopedCFTypeRef<SecTrustRef> trust_ref;
   SecTrustResultType trust_result = kSecTrustResultDeny;
   ScopedCFTypeRef<CFArrayRef> completed_chain;
   CSSM_TP_APPLE_EVIDENCE_INFO* chain_info = NULL;
   bool candidate_untrusted = true;
   bool candidate_weak = false;
+  bool completed_chain_revoked = false;
 
   // OS X lacks proper path discovery; it will take the input certs and never
   // backtrack the graph attempting to discover valid paths.
   // This can create issues in some situations:
   // - When OS X changes the trust store, there may be a chain
   //     A -> B -> C -> D
   //   where OS X trusts D (on some versions) and trusts C (on some versions).
   //   If a server supplies a chain A, B, C (cross-signed by D), then this chain
   //   will successfully validate on systems that trust D, but fail for systems
   //   that trust C. If the server supplies a chain of A -> B, then it forces
@@ skipping 15 matching lines @@
   //   version of C signed by D is signed using a weak algorithm (e.g. SHA-1),
   //   while the version of C in the trust store's signature doesn't matter.
   //   Since a 'strong' chain exists, it would be desirable to prefer this
   //   chain.
   //
   // - A variant of the above example, it may be that the version of B sent by
   //   the server is signed using a weak algorithm, but the version of B
   //   present in the AIA of A is signed using a strong algorithm. Since a
   //   'strong' chain exists, it would be desirable to prefer this chain.
   //
+  // - A user keychain may contain a less desirable intermediate or root.
+  //   OS X gives the user keychains higher priority than the system keychain,
+  //   so it may build a weak chain.
+  //
   // Because of this, the code below first attempts to validate the peer's
   // identity using the supplied chain. If it is not trusted (e.g. the OS only
   // trusts C, but the version of C signed by D was sent, and D is not trusted),
   // or if it contains a weak chain, it will begin lopping off certificates
   // from the end of the chain and attempting to verify. If a stronger, trusted
   // chain is found, it is used, otherwise, the algorithm continues until only
   // the peer's certificate remains.
   //
+  // If the loop does not find a trusted chain, the loop will be repeated with
+  // the keychain search order altered to give priority to the System Roots
+  // keychain.
+  //
   // This does cause a performance hit for these users, but only in cases where
   // OS X is building weaker chains than desired, or when it would otherwise
   // fail the connection.
-  while (CFArrayGetCount(cert_array) > 0) {
-    ScopedCFTypeRef<SecTrustRef> temp_ref;
-    SecTrustResultType temp_trust_result = kSecTrustResultDeny;
-    ScopedCFTypeRef<CFArrayRef> temp_chain;
-    CSSM_TP_APPLE_EVIDENCE_INFO* temp_chain_info = NULL;
+  for (bool try_reordered_keychain : {false, true}) {

[Ryan Sleevi, 2016/08/12 19:50:17]

NICE! :)

+    ScopedCFTypeRef<CFArrayRef> scoped_alternate_keychain_search_list;
+    if (TestKeychainSearchList::HasInstance()) {
+      CFArrayRef keychain_search_list;
+      status = TestKeychainSearchList::GetInstance()->CopySearchList(
+          &keychain_search_list);
+      if (status)
+        return NetErrorFromOSStatus(status);
+      scoped_alternate_keychain_search_list.reset(keychain_search_list);
+    }

[Ryan Sleevi, 2016/08/12 19:50:17]

Could you add some additional documentation here explaining the relationships
between keychain_search_list, try_reordered_keychain, and
scoped_alternate_keychain_search_list? I struggled a bit to make sense of this
all when coming back to it.

if (TestKeychainSearchList::HasInstance()) {
  // Unit tests need to be able to hermetically simulate situations
  // where a user has an undesirable certificate in a per-user keychain.
  // Adding/Removing a Keychain using [API here] has global side effects,
  // which would break other tests and processes running on the same
  // machine, so instead ... [explain some logic or point to the other
  // explainer].
}

[mattm, 2016/08/16 01:41:34]

Done.

+    if (try_reordered_keychain) {
+      if (!scoped_alternate_keychain_search_list) {
+        CFArrayRef keychain_search_list;
+        status = SecKeychainCopySearchList(&keychain_search_list);
+        if (status)
+          return NetErrorFromOSStatus(status);
+        scoped_alternate_keychain_search_list.reset(keychain_search_list);
+      }
+      CFMutableArrayRef mutable_keychain_search_list = CFArrayCreateMutableCopy(
+          kCFAllocatorDefault,
+          CFArrayGetCount(scoped_alternate_keychain_search_list.get()) + 1,
+          scoped_alternate_keychain_search_list.get());
+      if (!mutable_keychain_search_list)
+        return ERR_OUT_OF_MEMORY;
+      scoped_alternate_keychain_search_list.reset(mutable_keychain_search_list);
 
-    int rv = BuildAndEvaluateSecTrustRef(cert_array, trust_policies, flags,
-                                         &temp_ref, &temp_trust_result,
-                                         &temp_chain, &temp_chain_info);
-    if (rv != OK)
-      return rv;
+      SecKeychainRef keychain;
+      // Get a reference to the System Roots keychain. This is a gross hack,
+      // but the path is known to be valid on OS X 10.9-10.11.
+      status = SecKeychainOpen(
+          "/System/Library/Keychains/SystemRootCertificates.keychain",
+          &keychain);

[Ryan Sleevi, 2016/08/12 19:50:17]

DESIGN: Is it worth adding a unit test for this call as a sentinel for future OS
uprevs (e.g. if this fails, it'll take down some tests with it, but we could
pinpoint it if a specific unittest was failing and was documented as such)

[mattm, 2016/08/16 01:41:34]

Done.

+      if (status)
+        return NetErrorFromOSStatus(status);
+      ScopedCFTypeRef<SecKeychainRef> scoped_keychain(keychain);
 
-    bool untrusted = (temp_trust_result != kSecTrustResultUnspecified &&
-                      temp_trust_result != kSecTrustResultProceed);
-    bool weak_chain = false;
-    if (CFArrayGetCount(temp_chain) == 0) {
-      // If the chain is empty, it cannot be trusted or have recoverable
-      // errors.
-      DCHECK(untrusted);
-      DCHECK_NE(kSecTrustResultRecoverableTrustFailure, temp_trust_result);
-    } else {
-      CertVerifyResult temp_verify_result;
-      bool leaf_is_weak = false;
-      GetCertChainInfo(temp_chain, temp_chain_info, &temp_verify_result,
-                       &leaf_is_weak);
-      weak_chain = !leaf_is_weak &&
-                   (temp_verify_result.has_md2 || temp_verify_result.has_md4 ||
-                    temp_verify_result.has_md5 || temp_verify_result.has_sha1);
+      CFArrayInsertValueAtIndex(mutable_keychain_search_list, 0, keychain);
     }
-    // Set the result to the current chain if:
-    // - This is the first verification attempt. This ensures that if
-    //   everything is awful (e.g. it may just be an untrusted cert), that
-    //   what is reported is exactly what was sent by the server
-    // - If the current chain is trusted, and the old chain was not trusted,
-    //   then prefer this chain. This ensures that if there is at least a
-    //   valid path to a trust anchor, it's preferred over reporting an error.
-    // - If the current chain is trusted, and the old chain is trusted, but
-    //   the old chain contained weak algorithms while the current chain only
-    //   contains strong algorithms, then prefer the current chain over the
-    //   old chain.
-    //
-    // Note: If the leaf certificate itself is weak, then the only
-    // consideration is whether or not there is a trusted chain. That's
-    // because no amount of path discovery will fix a weak leaf.
-    if (!trust_ref || (!untrusted && (candidate_untrusted ||
-                                      (candidate_weak && !weak_chain)))) {
-      trust_ref = temp_ref;
-      trust_result = temp_trust_result;
-      completed_chain = temp_chain;
-      chain_info = temp_chain_info;
 
-      candidate_untrusted = untrusted;
-      candidate_weak = weak_chain;
+    ScopedCFTypeRef<CFMutableArrayRef> cert_array(
+        cert->CreateOSCertChainForCert());
+
+    while (CFArrayGetCount(cert_array) > 0) {

[Ryan Sleevi, 2016/08/12 19:50:17]

I'm a little nervous that this is moved far away from the giant wall of text
documentation, in that now I have to scroll an additional screen's worth up in
order to match implementation and execution.

Perhaps it's suitable just to include a line or two that links this part of the
algorithm with the above implementation documentation, like

// Beginning with the certificate chain as supplied by the server,
// attempt to verify the chain. If a failure is encountered, trim
// a certificate from the end (so long as one remains) and retry, in
// the hope of forcing OS X to find a better path.

[mattm, 2016/08/16 01:41:34]

Done.

+      ScopedCFTypeRef<SecTrustRef> temp_ref;
+      SecTrustResultType temp_trust_result = kSecTrustResultDeny;
+      ScopedCFTypeRef<CFArrayRef> temp_chain;
+      CSSM_TP_APPLE_EVIDENCE_INFO* temp_chain_info = NULL;
+
+      int rv = BuildAndEvaluateSecTrustRef(
+          cert_array, trust_policies, flags,
+          scoped_alternate_keychain_search_list.get(), &temp_ref,
+          &temp_trust_result, &temp_chain, &temp_chain_info);
+      if (rv != OK)
+        return rv;
+
+      bool revoked =
+          (crl_set && !CheckRevocationWithCRLSet(temp_chain, crl_set));

[Ryan Sleevi, 2016/08/12 19:50:17]

I suspect we should do with some documentation here about some of the nuances of
CRLSets. If you're not comfortable writing it up, LMK and I'll try to do
something.

[mattm, 2016/08/16 01:41:34]

That would be great since I'm not sure what the nuances are myself.

[Ryan Sleevi, 2016/08/17 02:28:12]

// Check to see if the path |temp_chain| has been revoked. This is
// less than ideal to perform after path building, rather than during,
// because there may be multiple paths to trust anchors, and only some
// of them are revoked. Ideally, CRLSets would be part of path
// building, which they are when using NSS (Linux) or CryptoAPI (Windows).
//
// The CRLSet checking is performed inside the loop in the hope that
// if a path is revoked, it's an older path, and the only reason it
// was built is because the server forced it (by supplying an older
// or less desirable intermediate) or because the user had installed
// a certificate in their Keychain forcing this path. However, this
// means its still possible for a CRLSet block of an intermediate to
// prevent access, even when there is a 'good' chain. To fully
// remedy this, a solution might be to have CRLSets contain enough
// knowledge about what the 'desired' path might be, but for the
// time being, the implementation is kept as 'simple' as it can be.



Sorry, I should have elaborated more on what I meant by "nuances" here. Mostly,
I was thinking about the risk of why CRLSets post-chain is less than ideal. I'm
not sure if we should reference the unittests here, but I think I could live
with it, and we could say something like "See
//net/data/ssl/scripts/generate-multi-root-test-chains.sh to better understand
how the PKI graph can look."

*shrug*

[mattm, 2016/08/17 03:50:57]

Ah, that. Done.
eh, comment is pretty long already :) I can add it if you want though.

+      bool untrusted = (temp_trust_result != kSecTrustResultUnspecified &&
+                        temp_trust_result != kSecTrustResultProceed) ||
+                       revoked;
+      bool weak_chain = false;
+      if (CFArrayGetCount(temp_chain) == 0) {
+        // If the chain is empty, it cannot be trusted or have recoverable
+        // errors.
+        DCHECK(untrusted);
+        DCHECK_NE(kSecTrustResultRecoverableTrustFailure, temp_trust_result);
+      } else {
+        CertVerifyResult temp_verify_result;
+        bool leaf_is_weak = false;
+        GetCertChainInfo(temp_chain, temp_chain_info, &temp_verify_result,
+                         &leaf_is_weak);
+        weak_chain =
+            !leaf_is_weak &&
+            (temp_verify_result.has_md2 || temp_verify_result.has_md4 ||
+             temp_verify_result.has_md5 || temp_verify_result.has_sha1);
+      }
+      // Set the result to the current chain if:
+      // - This is the first verification attempt. This ensures that if
+      //   everything is awful (e.g. it may just be an untrusted cert), that
+      //   what is reported is exactly what was sent by the server
+      // - If the current chain is trusted, and the old chain was not trusted,
+      //   then prefer this chain. This ensures that if there is at least a
+      //   valid path to a trust anchor, it's preferred over reporting an error.
+      // - If the current chain is trusted, and the old chain is trusted, but
+      //   the old chain contained weak algorithms while the current chain only
+      //   contains strong algorithms, then prefer the current chain over the
+      //   old chain.
+      //
+      // Note: If the leaf certificate itself is weak, then the only
+      // consideration is whether or not there is a trusted chain. That's
+      // because no amount of path discovery will fix a weak leaf.
+      if (!trust_ref || (!untrusted && (candidate_untrusted ||
+                                        (candidate_weak && !weak_chain)))) {
+        trust_ref = temp_ref;
+        trust_result = temp_trust_result;
+        completed_chain = temp_chain;
+        completed_chain_revoked = revoked;
+        chain_info = temp_chain_info;
+
+        candidate_untrusted = untrusted;
+        candidate_weak = weak_chain;
+      }
+      // Short-circuit when a current, trusted chain is found.
+      if (!untrusted && !weak_chain)
+        break;
+      CFArrayRemoveValueAtIndex(cert_array, CFArrayGetCount(cert_array) - 1);
     }
     // Short-circuit when a current, trusted chain is found.
-    if (!untrusted && !weak_chain)
+    if (!candidate_untrusted && !candidate_weak)
       break;
-    CFArrayRemoveValueAtIndex(cert_array, CFArrayGetCount(cert_array) - 1);
   }
 
   if (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED)
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
 
-  if (crl_set && !CheckRevocationWithCRLSet(completed_chain, crl_set))
+  if (completed_chain_revoked)
     verify_result->cert_status |= CERT_STATUS_REVOKED;
 
   if (CFArrayGetCount(completed_chain) > 0) {
     bool leaf_is_weak_unused = false;
     GetCertChainInfo(completed_chain, chain_info, verify_result,
                      &leaf_is_weak_unused);
   }
 
   // As of Security Update 2012-002/OS X 10.7.4, when an RSA key < 1024 bits
   // is encountered, CSSM returns CSSMERR_TP_VERIFY_ACTION_FAILED and adds
@@ skipping 150 matching lines @@
       }
     }
   }
 
   return OK;
 }
 
 }  // namespace net
 
 #pragma clang diagnostic pop  // "-Wdeprecated-declarations"