Make OS X path "building" more buildy, less breaky

net/cert/cert_verify_proc_mac.cc

Index: net/cert/cert_verify_proc_mac.cc
diff --git a/net/cert/cert_verify_proc_mac.cc b/net/cert/cert_verify_proc_mac.cc
index 709922598eaf20985dd67a50661dd9692ab33efc..8316cc549039fb314bfcad82de601216dea4cd4c 100644
--- a/net/cert/cert_verify_proc_mac.cc
+++ b/net/cert/cert_verify_proc_mac.cc
@@ -175,12 +175,21 @@ OSStatus CreateTrustPolicies(const std::string& hostname,
   return noErr;
 }
 
-// Saves some information about the certificate chain |cert_chain| in
-// |*verify_result|. The caller MUST initialize |*verify_result| before
-// calling this function.
+// Stores the constructed certificate chain |cert_chain| and information about
+// the signature algorithms used into |*verify_result|. If the leaf cert in
+// |cert_chain| contains a weak (MD2, MD4, MD5, SHA-1) signature, stores that
+// in |*leaf_is_weak|.
 void GetCertChainInfo(CFArrayRef cert_chain,
                       CSSM_TP_APPLE_EVIDENCE_INFO* chain_info,
-                      CertVerifyResult* verify_result) {
+                      CertVerifyResult* verify_result,
+                      bool* leaf_is_weak) {

[Ryan Sleevi, 2015/01/31 02:23:03]

This is just so I can shortcircuit the loop below. If the leaf is weak, no
amount of lopping certs off will change the result that the overall chain is
weak.

[davidben, 2015/02/02 22:13:55]

("This" is referring to leaf_is_week and not the resets at the bottom, right?)

[Ryan Sleevi, 2015/02/02 22:36:00]

Right, sorry. Yes, adding leaf is weak was to make the shortcircuiting easier.

+  *leaf_is_weak = false;
+  verify_result->verified_cert = nullptr;
+  verify_result->has_md2 = false;
+  verify_result->has_md4 = false;
+  verify_result->has_md5 = false;
+  verify_result->has_sha1 = false;
+
   SecCertificateRef verified_cert = NULL;
   std::vector<SecCertificateRef> verified_chain;
   for (CFIndex i = 0, count = CFArrayGetCount(cert_chain); i < count; ++i) {
@@ -223,10 +232,16 @@ void GetCertChainInfo(CFArrayRef cert_chain,
     const CSSM_OID* alg_oid = &sig_algorithm->algorithm;
     if (CSSMOIDEqual(alg_oid, &CSSMOID_MD2WithRSA)) {
       verify_result->has_md2 = true;
+      if (i == 0)
+        *leaf_is_weak = true;
     } else if (CSSMOIDEqual(alg_oid, &CSSMOID_MD4WithRSA)) {
       verify_result->has_md4 = true;
+      if (i == 0)
+        *leaf_is_weak = true;
     } else if (CSSMOIDEqual(alg_oid, &CSSMOID_MD5WithRSA)) {
       verify_result->has_md5 = true;
+      if (i == 0)
+        *leaf_is_weak = true;
     } else if (CSSMOIDEqual(alg_oid, &CSSMOID_SHA1WithRSA) ||
                CSSMOIDEqual(alg_oid, &CSSMOID_SHA1WithRSA_OIW) ||
                CSSMOIDEqual(alg_oid, &CSSMOID_SHA1WithDSA) ||
@@ -234,6 +249,8 @@ void GetCertChainInfo(CFArrayRef cert_chain,
                CSSMOIDEqual(alg_oid, &CSSMOID_SHA1WithDSA_JDK) ||
                CSSMOIDEqual(alg_oid, &CSSMOID_ECDSA_WithSHA1)) {
       verify_result->has_sha1 = true;
+      if (i == 0)
+        *leaf_is_weak = true;
     }
   }
   if (!verified_cert)
@@ -446,80 +463,6 @@ int BuildAndEvaluateSecTrustRef(CFArrayRef cert_array,
   return OK;
 }
 
-// OS X ships with both "GTE CyberTrust Global Root" and "Baltimore CyberTrust
-// Root" as part of its trusted root store. However, a cross-certified version
-// of the "Baltimore CyberTrust Root" exists that chains to "GTE CyberTrust
-// Global Root". When OS X/Security.framework attempts to evaluate such a
-// certificate chain, it disregards the "Baltimore CyberTrust Root" that exists
-// within Keychain and instead attempts to terminate the chain in the "GTE
-// CyberTrust Global Root". However, the GTE root is scheduled to be removed in
-// a future OS X update (for sunsetting purposes), and once removed, such
-// chains will fail validation, even though a trust anchor still exists.
-//
-// Rather than over-generalizing a solution that may mask a number of TLS
-// misconfigurations, attempt to specifically match the affected
-// cross-certified certificate and remove it from certificate chain processing.

[Ryan Sleevi, 2015/01/31 02:23:03]

Sadly, overgeneralizing is the "right" answer here :(

-bool IsBadBaltimoreGTECertificate(SecCertificateRef cert) {
-  // Matches the GTE-signed Baltimore CyberTrust Root
-  // https://cacert.omniroot.com/Baltimore-to-GTE-04-12.pem
-  static const SHA1HashValue kBadBaltimoreHashNew =
-    { { 0x4D, 0x34, 0xEA, 0x92, 0x76, 0x4B, 0x3A, 0x31, 0x49, 0x11,
-        0x99, 0x52, 0xF4, 0x19, 0x30, 0xCA, 0x11, 0x34, 0x83, 0x61 } };
-  // Matches the legacy GTE-signed Baltimore CyberTrust Root
-  // https://cacert.omniroot.com/gte-2-2025.pem
-  static const SHA1HashValue kBadBaltimoreHashOld =
-    { { 0x54, 0xD8, 0xCB, 0x49, 0x1F, 0xA1, 0x6D, 0xF8, 0x87, 0xDC,
-        0x94, 0xA9, 0x34, 0xCC, 0x83, 0x6B, 0xDA, 0xA8, 0xA3, 0x69 } };
-
-  SHA1HashValue fingerprint = X509Certificate::CalculateFingerprint(cert);
-
-  return fingerprint.Equals(kBadBaltimoreHashNew) ||
-         fingerprint.Equals(kBadBaltimoreHashOld);
-}
-
-// Attempts to re-verify |cert_array| after adjusting the inputs to work around
-// known issues in OS X. To be used if BuildAndEvaluateSecTrustRef fails to
-// return a positive result for verification.
-//
-// This function should only be called while the Mac Security Services lock is
-// held.
-void RetrySecTrustEvaluateWithAdjustedChain(
-    CFArrayRef cert_array,
-    CFArrayRef trust_policies,
-    int flags,
-    ScopedCFTypeRef<SecTrustRef>* trust_ref,
-    SecTrustResultType* trust_result,
-    ScopedCFTypeRef<CFArrayRef>* verified_chain,
-    CSSM_TP_APPLE_EVIDENCE_INFO** chain_info) {
-  CFIndex count = CFArrayGetCount(*verified_chain);
-  CFIndex slice_point = 0;
-
-  for (CFIndex i = 1; i < count; ++i) {
-    SecCertificateRef cert = reinterpret_cast<SecCertificateRef>(
-        const_cast<void*>(CFArrayGetValueAtIndex(*verified_chain, i)));
-    if (cert == NULL)
-      return;  // Strange times; can't fix things up.
-
-    if (IsBadBaltimoreGTECertificate(cert)) {
-      slice_point = i;
-      break;
-    }
-  }
-  if (slice_point == 0)
-    return;  // Nothing to do.
-
-  ScopedCFTypeRef<CFMutableArrayRef> adjusted_cert_array(
-      CFArrayCreateMutable(NULL, 0, &kCFTypeArrayCallBacks));
-  // Note: This excludes the certificate at |slice_point|.
-  CFArrayAppendArray(adjusted_cert_array, cert_array,
-                     CFRangeMake(0, slice_point));
-
-  // Ignore the result; failure will preserve the old verification results.
-  BuildAndEvaluateSecTrustRef(
-      adjusted_cert_array, trust_policies, flags, trust_ref, trust_result,
-      verified_chain, chain_info);
-}
-
 }  // namespace
 
 CertVerifyProcMac::CertVerifyProcMac() {}
@@ -547,7 +490,8 @@ int CertVerifyProcMac::VerifyInternal(
   // array of certificates, the first of which is the certificate we're
   // verifying, and the subsequent (optional) certificates are used for
   // chain building.
-  ScopedCFTypeRef<CFArrayRef> cert_array(cert->CreateOSCertChainForCert());
+  ScopedCFTypeRef<CFMutableArrayRef> cert_array(CFArrayCreateMutableCopy(
+      kCFAllocatorDefault, 0, 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.
@@ -557,17 +501,90 @@ int CertVerifyProcMac::VerifyInternal(
   SecTrustResultType trust_result = kSecTrustResultDeny;
   ScopedCFTypeRef<CFArrayRef> completed_chain;
   CSSM_TP_APPLE_EVIDENCE_INFO* chain_info = NULL;
-
-  int rv = BuildAndEvaluateSecTrustRef(
-      cert_array, trust_policies, flags, &trust_ref, &trust_result,
-      &completed_chain, &chain_info);
-  if (rv != OK)
-    return rv;
-  if (trust_result != kSecTrustResultUnspecified &&
-      trust_result != kSecTrustResultProceed) {
-    RetrySecTrustEvaluateWithAdjustedChain(
-        cert_array, trust_policies, flags, &trust_ref, &trust_result,
-        &completed_chain, &chain_info);
+  bool candidate_untrusted = true;
+  bool candidate_weak = 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
+  //   all clients to fetch C (via AIA) if they trust D, and not all clients
+  //   (notably, Firefox and Android) will do this, thus breaking them.
+  //   An example of this is the Verizon Business Services root - GTE CyberTrust
+  //   and Baltimore CyberTrust roots represent old and new roots that cause
+  //   issues depending on which version of OS X being used.
+  //
+  // - OS X users may have installed an intermediate certificate that has
+  //   expired. In this case, a non-expired version may be fetched via AIA,
+  //   but depending on the chain sent by the server, this may not happen,
+  //   causing validation to fail.
+  //   An example of this is DigiCert, as described at
+  //   https://blog.digicert.com/expired-intermediate-certificate/

[davidben, 2015/02/02 22:13:55]

So, this was an example of DigiCert being now in the trust store, but previously
they had an intermediate, now expired, signed by some other key? I could be
misremembering, but I think the sites in question would send

   Leaf -> Something -> DigiCert (with DigiCert omitted on the wire because it's
a self-signed root)

But OS X would try to build something like

  Leaf -> Something -> DigiCert(expired) -> SomethingElse

so it used the DigiCert(expired) signed by SomethingElse in its store rather
than the self-signed DigiCert root also in the store.

(Is that correct?)

In that case, how does chopping off the end help there? Is it that once you chop
down to the leaf, OS X would redundantly fetch Something and then use different
logic to end up at the right one?

[Ryan Sleevi, 2015/02/02 22:36:01]

If I recall correctly (and I sent out a ping to my contacts at DigiCert), the
chain is something like

Leaf -> Something(A) -> DigiCert(expired) -> DigiCert [old?] root
Leaf -> Something(B) -> DigiCert(other intermediate) -> DigiCert [new?] root

The lopping here forces a fetch of Something via Leaf's AIA, which picks up the
new path.

+  //
+  // - When OS X trusts both C and D (simultaneously), it's possible that the
+  //   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.

[davidben, 2015/02/02 22:13:54]

I'm not sure I understand how this case relates to the loop. Is it that the
server sends[*] A -> B -> C -> D where C is signed by D with SHA-1, and OS X
doesn't know to stop? Or is the server still sending the A -> B -> C chain and
OS X is adding an intermediate it saw somewhere else? If the former, I would
have thought the solution would be for the server to send a different chain. If
the latter, why does chopping bits off the chain help?

[*] Assume "sends" here accounts for you wanting to omit the self-signed root,
so "sending" A -> B -> C -> D means you send only three certs, sign_B(A),
sign_C(B), sign_D(C).

[Ryan Sleevi, 2015/02/02 22:36:01]

if I understood the question correctly -
If OS X trusts both C and D, and the server sends sign_D(C), then it will build
the path to D, even if it could have stopped with C.

This results in the signature for sign_D(C) affecting the security calculation.
By chopping of sign_D(C) from the inputs, it looks in the trust store when
evaluating sign_C(B), finds C, and stops there.

Now, the reason why you need to send sign_D(C) is for legacy/non-AIA devices
that trust D - e.g. Android, Firefox. If you just sent A -> B and they only
trust D, they would fail to build a path. However, they're also smart enough
that if you're on a new Android/Firefox, and you send sign_D(C), they'll ignore
it in favour of sign_C(C) [aka the new root]

[davidben, 2015/02/03 22:39:52]

Okay. So this is another variant of the first example, but with SHA-1 instead of
1024-bit roots? They couldn't get a new sign_D(C) issued that used SHA-256
instead?

+  //
+  // - 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.
+  //
+  // 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.

[davidben, 2015/02/02 22:13:55]

Safari's strategy is to chop off all but the leaf, right? Are there known cases
where AIA chasing from the leaf fails while this does?

I'm not totally clear on how chopping certs off the end interacts with AIA. Is
the general behavior that OS X AIA chases from the end of the supplied chain and
never backtracks?

[Ryan Sleevi, 2015/02/02 22:36:00]

Yup. Our unittest for GTE CyberTrust is an example where the leaf doesn't have
an AIA.
Correct. OS X also has a little logic, in that it will attempt to lop off any
expired certs explicitly sent by the peer.

+  //
+  // 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 ((candidate_untrusted || candidate_weak) &&
+         CFArrayGetCount(cert_array) > 0) {

[davidben, 2015/02/02 22:13:54]

Think it's worth UMA to measure how usage in the wild of this crazy looks?  Not
sure what you'd most usefully measure though.

[Ryan Sleevi, 2015/02/02 22:36:01]

Eh, once we have our own chain building, we could look to avoid this crazy. As
it stands, the Apple folks know my gripe about this behaviour since 10.4, so I'm
not sure what we'd do different.

+    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,
+                                         &temp_ref, &temp_trust_result,
+                                         &temp_chain, &temp_chain_info);
+    if (rv != OK)
+      return rv;

[davidben, 2015/02/02 22:13:55]

A network error in AIA chasing will cause this to fail even if we had a chain
from a previous iteration, right? That seems not ideal if the old chain just had
SHA-1 but otherwise was still fine, but that might be more trouble than is
worth---the SHA-1 case has a time limit and all other cases would have failed
anyway.

[Ryan Sleevi, 2015/02/02 22:36:01]

No. rv would == OK, temp_trust_result == kSecTrustResultUnspecified, and
temp_chain_info would have status bits of AIA failure.

+
+    CertVerifyResult temp_verify_result;
+    bool leaf_is_weak = false;
+    GetCertChainInfo(temp_chain, temp_chain_info, &temp_verify_result,
+                     &leaf_is_weak);
+
+    bool untrusted = (temp_trust_result != kSecTrustResultUnspecified &&

[davidben, 2015/02/02 22:13:55]

[Confirmed that we consider Proceed and Unspecified as the same elsewhere. (Is
that what Safari does too?)]

[Ryan Sleevi, 2015/02/02 22:36:01]

https://developer.apple.com/library/mac/qa/qa1360/_index.html

kSecTrustResultUnspecified = no user explicit trust settings set, but it's
trusted
kSecTrustResultProceed = explicit user trust settings to always trust

+                      temp_trust_result != kSecTrustResultProceed);
+    bool 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);
+    if (!trust_ref || (!untrusted && (candidate_untrusted ||
+                                      (candidate_weak && !weak_chain)))) {

[davidben, 2015/02/02 22:13:54]

This condition was slightly hard to follow. Not sure if an English language
comment would help though.

+      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;
+    }
+    if (!untrusted && !weak_chain)

[davidben, 2015/02/02 22:13:54]

Isn't this redundant with the while loop condition? It just saves one CFArray
operation.

If our new thing is trusted and not weak then, we should have set that to the
candidate. And so candidate_untrusted and candidate_weak are false, which breaks
out of the loop. Or, in logic, that condition turns into:

  if (!trust_ref || (true && (candidate_untrusted || (candidate_weak && true))))
  if (!trust_ref || candidate_untrusted || candidate_weak)

So either candidate_untrusted and candidate_weak are already both false (in
which case we would have broken out of the loop already anyway), or we'll set
them to untrusted/weak_chain and they'll become both false. So we'll break out
of the loop.

[Ryan Sleevi, 2015/02/02 22:36:01]

Yeah. Just saves a CFArray operation. I originally had much more explicit
conditionals (with duplicated code), then coalesced to avoid the redundancy.

[davidben, 2015/02/03 22:39:52]

Maybe remove the condition from the top, if you really want to save that
CFArrayRemoveValueAtIndex call? :-) It's confusing to have the same thing
checked twice (especially two equivalent spellings of the same thing).

+      break;
+    CFArrayRemoveValueAtIndex(cert_array, CFArrayGetCount(cert_array) - 1);
   }
 
   if (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED)
@@ -576,7 +593,9 @@ int CertVerifyProcMac::VerifyInternal(
   if (crl_set && !CheckRevocationWithCRLSet(completed_chain, crl_set))
     verify_result->cert_status |= CERT_STATUS_REVOKED;
 
-  GetCertChainInfo(completed_chain, chain_info, verify_result);
+  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