Fixing BoringSSL on iOS

net/cert/cert_verify_proc_ios.cc

 // Copyright (c) 2016 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_ios.h"
 
 #include <CommonCrypto/CommonDigest.h>
 #include <Security/Security.h>
 
 #include "base/logging.h"
@@ skipping 122 matching lines @@
       continue;
 
     HashValue sha1(HASH_VALUE_SHA1);
     CC_SHA1(spki_bytes.data(), spki_bytes.size(), sha1.data());
     verify_result->public_key_hashes.push_back(sha1);
 
     HashValue sha256(HASH_VALUE_SHA256);
     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;
     } else if (sig_alg == NID_md4WithRSAEncryption) {
       verify_result->has_md4 = true;
     } else if (sig_alg == NID_md5WithRSAEncryption ||
                sig_alg == NID_md5WithRSA) {
       verify_result->has_md5 = true;
     } else if (sig_alg == NID_sha1WithRSAEncryption ||
                sig_alg == NID_dsaWithSHA || sig_alg == NID_dsaWithSHA1 ||
@@ skipping 50 matching lines @@
                                        &final_chain, &trust_result);
   if (status)
     return NetErrorFromOSStatus(status);
 
   if (CFArrayGetCount(final_chain) == 0)
     return ERR_FAILED;
 
   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.
   if (!verify_result->verified_cert->VerifyNameMatch(
           hostname, &verify_result->common_name_fallback_used)) {
     verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
   }
 
   verify_result->is_issued_by_known_root = false;
 
   if (IsCertStatusError(verify_result->cert_status))
     return MapCertStatusToNetError(verify_result->cert_status);
 
   return OK;
 }
 
 }  // namespace net