Fixing BoringSSL on iOS

net/cert/cert_verify_proc_ios.cc

Index: net/cert/cert_verify_proc_ios.cc
diff --git a/net/cert/cert_verify_proc_ios.cc b/net/cert/cert_verify_proc_ios.cc
index 5f2884f8a8411a15cba982047888ed378026bf3b..c5e97365bfb3f109ec1cf59d1a910703679b7a0a 100644
--- a/net/cert/cert_verify_proc_ios.cc
+++ b/net/cert/cert_verify_proc_ios.cc
@@ -140,6 +140,10 @@ void GetCertChainInfo(CFArrayRef cert_chain, CertVerifyResult* verify_result) {
     CC_SHA256(spki_bytes.data(), spki_bytes.size(), sha256.data());
     verify_result->public_key_hashes.push_back(sha256);
 
+    // Ignore the signature algorithm for the root (self-signed) certificate.

[Ryan Sleevi, 2016/04/08 20:01:58]

1) s/root (self-signed) certificate/trust anchor/ (no guarantee a TA is
self-signed)
2) I think we only want to ignore if it was trusted; if it was untrusted, we may
have an incomplete chain.

[svaldez, 2016/04/08 20:22:47]

This is only for setting the has_XXX bits on the verification result. Adding it
to the chain and copying the key hashes for verification are done earlier.

[Ryan Sleevi, 2016/04/08 20:44:21]

Yes, but has_sha1 is valid if, for example, the intermediate has SHA-1 and we
couldn't build a path to the root, so the intermediate is (count - 1)

For example, see the logic here -
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/cert_veri...
- or
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/cert_veri...

+    if (i == count - 1)
+      continue;
+
     int sig_alg = OBJ_obj2nid(x509_cert->sig_alg->algorithm);
     if (sig_alg == NID_md2WithRSAEncryption) {
       verify_result->has_md2 = true;
@@ -210,16 +214,50 @@ int CertVerifyProcIOS::VerifyInternal(
   GetCertChainInfo(final_chain, verify_result);
 
   // TODO(sleevi): Support CRLSet revocation.
-  // TODO(svaldez): Add specific error codes for trust errors resulting from
-  // expired/not-yet-valid certs.
   switch (trust_result) {
     case kSecTrustResultUnspecified:
     case kSecTrustResultProceed:
       break;
     case kSecTrustResultDeny:
       verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID;
+      break;
     default:
-      verify_result->cert_status |= CERT_STATUS_INVALID;
+      CFArrayRef properties = SecTrustCopyProperties(trust_ref);
+      if (properties && CFArrayGetCount(properties) != 0) {

[Ryan Sleevi, 2016/04/08 20:01:58]

Can you expand documentation comment to explain this (and why it's brittle but
acceptable). This is non-intuitive (although I totally understand) and deserves
a comment.

Also, is this enough logic that we should break it into a helper function (which
would help readability)?

[svaldez, 2016/04/08 20:22:48]

Done.

+        CFBundleRef bundle =
+            CFBundleGetBundleWithIdentifier(CFSTR("com.apple.Security"));
+        CFStringRef date_string = CFSTR(
+            "One or more certificates have expired or are not valid yet.");
+        CFStringRef date_error = CFBundleCopyLocalizedString(
+            bundle, date_string, date_string, CFSTR("SecCertificate"));
+        CFStringRef trust_string = CFSTR("Root certificate is not trusted.");
+        CFStringRef trust_error = CFBundleCopyLocalizedString(
+            bundle, trust_string, trust_string, CFSTR("SecCertificate"));
+        CFStringRef weak_string =
+            CFSTR("One or more certificates is using a weak key size.");
+        CFStringRef weak_error = CFBundleCopyLocalizedString(
+            bundle, weak_string, weak_string, CFSTR("SecCertificate"));
+        const CFIndex properties_length = CFArrayGetCount(properties);

[Ryan Sleevi, 2016/04/08 20:01:58]

newline between 239 & 240 would help readability here.

[svaldez, 2016/04/08 20:22:47]

Done.

+        for (CFIndex i = 0; i < properties_length; ++i) {
+          CFDictionaryRef dict =
+              (CFDictionaryRef)CFArrayGetValueAtIndex(properties, i);

[Ryan Sleevi, 2016/04/08 20:01:58]

C-style casts aren't allowed ;(

[svaldez, 2016/04/08 20:22:47]

Done.

+          CFStringRef error =
+              (CFStringRef)CFDictionaryGetValue(dict, CFSTR("value"));

[Ryan Sleevi, 2016/04/08 20:01:58]

Ditto

[svaldez, 2016/04/08 20:22:47]

Done.

+          if (CFStringCompare(error, date_error, 0) == kCFCompareEqualTo) {

[Ryan Sleevi, 2016/04/08 20:01:58]

Why not CFEqual for these?

[svaldez, 2016/04/08 20:22:48]

Done.

+            verify_result->cert_status |= CERT_STATUS_DATE_INVALID;
+          } else if (CFStringCompare(error, trust_error, 0) ==
+                     kCFCompareEqualTo) {
+            verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID;
+          } else if (CFStringCompare(error, weak_error, 0) ==
+                     kCFCompareEqualTo) {
+            verify_result->cert_status |= CERT_STATUS_WEAK_KEY;
+          } else {
+            verify_result->cert_status |= CERT_STATUS_INVALID;
+          }
+        }
+      } else {
+        verify_result->cert_status |= CERT_STATUS_INVALID;
+      }
   }
 
   // Perform hostname verification independent of SecTrustEvaluate.