PKI library Mac trust store integration

net/cert/internal/trust_store_mac.cc

Index: net/cert/internal/trust_store_mac.cc
diff --git a/net/cert/internal/trust_store_mac.cc b/net/cert/internal/trust_store_mac.cc
new file mode 100644
index 0000000000000000000000000000000000000000..83c4474731301f61845b63b68e9dd3ab0331206f
--- /dev/null
+++ b/net/cert/internal/trust_store_mac.cc
@@ -0,0 +1,353 @@
+// Copyright 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/internal/trust_store_mac.h"
+
+#include <Security/Security.h>
+
+#include "base/logging.h"
+#include "base/mac/foundation_util.h"
+#include "base/mac/mac_logging.h"
+#include "base/memory/ptr_util.h"
+#include "base/synchronization/lock.h"
+#include "crypto/cssm_init.h"  // for LogCSSMError.
+#include "crypto/mac_security_services_lock.h"
+#include "net/cert/internal/cert_errors.h"
+#include "net/cert/internal/parse_name.h"
+#include "net/cert/internal/parsed_certificate.h"
+#include "net/cert/test_keychain_search_list_mac.h"
+#include "net/cert/x509_certificate.h"
+
+namespace net {
+
+namespace {
+
+// The rules for interpreting trust settings are documented at:
+// https://developer.apple.com/reference/security/1400261-sectrustsettingscopytrustsetting?language=objc
+
+// Returns true if a usage constraints dictionary |trust_dict| for a certificate
+// that |is_self_signed| should be treated as a trust root.
+bool IsTrustDictionaryTrustedForPolicy(CFDictionaryRef trust_dict,
+                                       bool is_self_signed,
+                                       const CFStringRef target_policy_oid) {
+  DVLOG(2) << "IsTrustDictionaryTrustedForPolicy?";

[Ryan Sleevi, 2017/02/14 19:26:16]

I'm not sure the purpose of this trace; is it purely debugging?

You could use __func__ as an alternative, which has the benefit of (potential)
string pooling, but that's up to you if this is still necessary.

[mattm, 2017/02/16 22:06:08]

Yeah the DVLOGs were just for debugging/development. I never really know how
much logging to leave in since it does sometimes come in handy later. But I went
ahead and removed it.

+  // An empty trust dict should be interpreted as
+  // kSecTrustSettingsResultTrustRoot. This is handled by falling through all
+  // the conditions below with the default value of |trust_settings_result|.
+
+  // Ignore application-specific trust settings.

[Ryan Sleevi, 2017/02/14 19:26:17]

Comment suggestion: More detail

// Trust settings may be scoped to a single application, by checking that the
// code signing identity of the current application matches the serialized code
// signing identity in the kSecTrustSettingsApplication key.
// As this is not presently supported, skip any trust settings scoped to the
// application.

[mattm, 2017/02/16 22:06:07]

Done.

+  if (CFDictionaryContainsKey(trust_dict, kSecTrustSettingsApplication)) {
+    DVLOG(2) << "ignored trust dictionary with kSecTrustSettingsApplication";

[Ryan Sleevi, 2017/02/14 19:26:16]

Necessary?

[mattm, 2017/02/16 22:06:08]

Done.

+    return false;
+  }

[Ryan Sleevi, 2017/02/14 19:26:16]

newline between 43/44

[mattm, 2017/02/16 22:06:09]

Done.

+  // Ignore trust settings that contain policy-specific constraints. (E.g.,
+  // for SSL, settings that apply to a single hostname.)

[Ryan Sleevi, 2017/02/14 19:26:16]

// Trust settings may be scoped using policy-specific constraints. For example,
// SSL trust settings might be scoped to a single hostname, or EAP settings
specific
// to a particular WiFi network.
// As this is not presently supported, skip any policy-specific trust settings.

[mattm, 2017/02/16 22:06:08]

Done.

+  if (CFDictionaryContainsKey(trust_dict, kSecTrustSettingsPolicyString)) {
+    DVLOG(2) << "ignored trust dictionary with kSecTrustSettingsPolicyString";
+    return false;
+  }
+
+  // There doesn't seem to be a need to handle kSecTrustSettingsKeyUsage.

[Ryan Sleevi, 2017/02/14 19:26:16]

The way this comment is worded seems more conversational than documentational.
Perhaps this could be

// Ignoring kSecTrustSettingsKeyUsage for now; it does not seem relevant to the
// TLS case.

[mattm, 2017/02/16 22:06:07]

Done.

+
+  CFTypeRef policy_typeref =

[Ryan Sleevi, 2017/02/14 19:26:16]

It's probably worth documenting this block (namely, line 53-86)

// If the trust settings are scoped to a specific policy (via
kSecTrustSettingsPolicy),
// ensure that the policy is the same policy as |target_policy_oid|. If there is
// no kSecTrustSettingsPolicy key, it's considered a match for all policies.

[mattm, 2017/02/16 22:06:08]

Done.

+      CFDictionaryGetValue(trust_dict, kSecTrustSettingsPolicy);
+  SecPolicyRef policy_ref = nullptr;
+  if (policy_typeref) {
+    if (CFGetTypeID(policy_typeref) != SecPolicyGetTypeID()) {
+      DVLOG(2) << "kSecTrustSettingsPolicy not a SecPolicyRef";
+      return false;
+    }
+    policy_ref =
+        reinterpret_cast<SecPolicyRef>(const_cast<void*>(policy_typeref));
+  }

[Ryan Sleevi, 2017/02/14 19:26:16]

If you modify //base/mac/foundation_util.h to CF_CAST_DECL(SecPolicyRef), then
you could use

SecPolicyRef policy_ref =
base::mac::GetValueFromDictionary<SecPolicyRef>(trust_dic,
kSecTrustSettingsPolicy);

[mattm, 2017/02/16 22:06:09]

Done.</noideawhatimdoing>

+  if (policy_ref) {
+    CFDictionaryRef policy_dict = SecPolicyCopyProperties(policy_ref);
+    CFTypeRef value = CFDictionaryGetValue(policy_dict, kSecPolicyOid);
+    // Having a kSecTrustSettingsPolicy without a kSecPolicyOid element is an
+    // error.

[Ryan Sleevi, 2017/02/14 19:26:16]

Is it necessary to handle this condition? The OS API guarantees that this will
be populated and it will be a CFStringRef.

Note: You can also do

CFStringRef = base::mac::GetValueFromDictionary<CFStringRef>(policy_dict,
kSecPolicyOid);

[mattm, 2017/02/16 22:06:07]

Done.

+    if (!value) {
+      DVLOG(2) << "kSecTrustSettingsPolicy with no kSecPolicyOid!";
+      return false;
+    }
+    CFStringRef policy_oid = base::mac::CFCast<CFStringRef>(value);
+    if (!policy_oid) {
+      DVLOG(2) << "kSecPolicyOid value not a CFStringRef!";
+      return false;
+    }
+    base::ScopedCFTypeRef<CFDictionaryRef> scoped_policy_dict(policy_dict);

[Ryan Sleevi, 2017/02/14 19:26:16]

Is this right? You copy the properties on 65, which means you leak this on 71
and 76 with this being placed here

[mattm, 2017/02/16 22:06:09]

... I have no excuse. Fixed.

+    if (!CFEqual(policy_oid, target_policy_oid)) {
+      DVLOG(2) << "kSecPolicyOid does not match target policy_oid";
+      return false;
+    }
+    DVLOG(2) << "kSecPolicyOid matches target policy_oid";
+
+    // XXX care about kSecPolicyName, kSecPolicyClient, kSecPolicyKU_<foo> ?

[Ryan Sleevi, 2017/02/14 19:26:16]

It's unclear whether you're asking if this needs to be held to meet this CLs
goals, or for the future. If the future, YAGNI. If for now, no, we don't need to
handle that.

[mattm, 2017/02/16 22:06:09]

Done.

+  }
+
+  // If kSecTrustSettingsResult is not present in the trust dict,
+  // kSecTrustSettingsResultTrustRoot is assumed.
+  int trust_settings_result = kSecTrustSettingsResultTrustRoot;
+  CFNumberRef trust_settings_result_ref =
+      base::mac::GetValueFromDictionary<CFNumberRef>(trust_dict,
+                                                     kSecTrustSettingsResult);
+
+  if (trust_settings_result_ref) {
+    if (!CFNumberGetValue(trust_settings_result_ref, kCFNumberIntType,
+                          &trust_settings_result)) {
+      DVLOG(2) << "CFNumberGetValue fail";
+      return false;
+    }
+    DVLOG(2) << "kSecTrustSettingsResult = " << trust_settings_result;
+  }
+
+  // TODO: handle distrust (kSecTrustSettingsResultDeny)

[Ryan Sleevi, 2017/02/14 19:26:17]

Do you have thoughts on how to handle this with the trust store API?

[mattm, 2017/02/16 22:06:09]

I've thought about, but I still don't really have a solid answer..

One approach, which we discussed in the past, would be to have a separate
IsCertDistrusted sort of method, that would get called in a separate step like
how CRLSets are checked.

The other, would be to get rid of the separation between CertIssuerSource and
TrustStore, have a single interface for finding issuers which also returns a
tri-state enum with each one for trusted/unspecified/distrusted. This seems
simpler at first glance but I guess it could have issues if the same cert is
found from multiple sources and some mark it as distrusted and others don't. 

Or some combination of the two where looking up issuers (whether trust anchors
or not) from the keychain is one step, and then a separate step is just like
GetCertTrustValue which could return trusted/unspecified/distrusted. I kinda
like that since the current api seems a little off to me.

+
+  // kSecTrustSettingsResultTrustRoot can only be applied to root(self-signed)
+  // certs.
+  if (is_self_signed)
+    return trust_settings_result == kSecTrustSettingsResultTrustRoot;
+
+  // kSecTrustSettingsResultTrustAsRoot can only be applied to non-root certs.
+  return trust_settings_result == kSecTrustSettingsResultTrustAsRoot;
+}
+
+// Returns true if the trust settings array |trust_settings| for a certificate
+// that |is_self_signed| should be treated as a trust root.

[Ryan Sleevi, 2017/02/14 19:26:16]

s/root/anchor/

[mattm, 2017/02/16 22:06:07]

Done.

+bool IsTrustSettingsTrustedForPolicy(CFArrayRef trust_settings,
+                                     bool is_self_signed,
+                                     const CFStringRef policy_oid) {
+  DVLOG(2) << "IsTrustSettingsTrustedForPolicy?";
+  // An empty trust settings array (that is, the trustSettings parameter returns

[Ryan Sleevi, 2017/02/14 19:26:16]

s/trustSettings/trust_settings/

[mattm, 2017/02/16 22:06:08]

Done.

+  // a valid but empty CFArray) means "always trust this certificate” with an

[Ryan Sleevi, 2017/02/14 19:26:16]

note the trailing quote is a smart quote, not "

[mattm, 2017/02/16 22:06:08]

Done.

+  // overall trust setting for the certificate of
+  // kSecTrustSettingsResultTrustRoot.
+  if (CFArrayGetCount(trust_settings) == 0 && is_self_signed) {
+    DVLOG(2) << "Empty trust settings array and self-signed cert = trusted";
+    return true;
+  }
+
+  for (CFIndex i = 0, settings_count = CFArrayGetCount(trust_settings);
+       i < settings_count; ++i) {
+    DVLOG(2) << "IsTrustSettingsTrustedForPolicy i=" << i << " of "
+             << settings_count;
+    CFDictionaryRef trust_dict = reinterpret_cast<CFDictionaryRef>(
+        const_cast<void*>(CFArrayGetValueAtIndex(trust_settings, i)));
+    if (IsTrustDictionaryTrustedForPolicy(trust_dict, is_self_signed,
+                                          policy_oid))
+      return true;
+  }
+  return false;
+}
+
+// Returns true if the certificate |cert_handle| is trusted for the policy
+// |policy_oid|.
+bool IsSecCertificateTrustedForPolicy(SecCertificateRef cert_handle,
+                                      const CFStringRef policy_oid) {
+  const bool is_self_signed = X509Certificate::IsSelfSigned(cert_handle);
+  // Evaluate trust domains in user, admin, system order. Admin settings can
+  // override system ones, and user settings can override both admin and system.
+  for (const auto& trust_domain :
+       {kSecTrustSettingsDomainUser, kSecTrustSettingsDomainAdmin,
+        kSecTrustSettingsDomainSystem}) {
+    DVLOG(2) << "IsSecCertificateTrustedForPolicy domain = " << trust_domain;
+    CFArrayRef trust_settings;
+    OSStatus err = SecTrustSettingsCopyTrustSettings(cert_handle, trust_domain,
+                                                     &trust_settings);

[Ryan Sleevi, 2017/02/14 19:26:16]

You can instead use

base::ScopedCFTypeRef<CFArrayRef> trust_settings;
OSStatus err = SecTrustSettingsCopyTrustSettings(cert_handle, trust_domain,
trust_settings.InitializeInto());

[mattm, 2017/02/16 22:06:08]

Done.

+    if (err == errSecItemNotFound) {
+      DVLOG(2) << "no trust settings for that domain";
+      // No trust settings for that domain.. try the next.
+      continue;
+    }
+    if (err) {
+      OSSTATUS_LOG(ERROR, err) << "SecTrustSettingsCopyTrustSettings error";
+      continue;
+    }
+    base::ScopedCFTypeRef<CFArrayRef> scoped_trust_settings(trust_settings);
+    if (IsTrustSettingsTrustedForPolicy(trust_settings, is_self_signed,
+                                        policy_oid))
+      return true;
+  }
+
+  // No trust settings, or none of the settings were for the correct policy, or
+  // had the correct trust result.
+  return false;
+}
+
+}  // namespace
+
+TrustStoreMac::TrustStoreMac(const void* policy_oid)
+    : policy_oid_(base::mac::CFCast<CFStringRef>(policy_oid)) {

[Ryan Sleevi, 2017/02/14 19:26:16]

CFStrictCast ?

[mattm, 2017/02/16 22:06:08]

Well, it does DCHECK(policy_oid_), but I guess a strict cast would distinguish
between wrong type and nullptr...

+  DCHECK(policy_oid_);
+}
+
+TrustStoreMac::~TrustStoreMac() = default;
+
+void TrustStoreMac::FindTrustAnchorsForCert(
+    const scoped_refptr<ParsedCertificate>& cert,
+    TrustAnchors* out_anchors) const {
+  DVLOG(2) << "FindTrustAnchorsForCert";
+
+  base::ScopedCFTypeRef<CFDataRef> name_data = GetMacNormalizedIssuer(cert);
+
+  FindTrustAnchorsByMacNormalizedSubject(name_data, out_anchors);
+}
+
+// static
+base::ScopedCFTypeRef<CFArrayRef>
+TrustStoreMac::FindMatchingCertificatesForMacNormalizedSubject(
+    CFDataRef name_data) {
+  base::ScopedCFTypeRef<CFArrayRef> matching_items;
+  base::ScopedCFTypeRef<CFMutableDictionaryRef> query(
+      CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks,

[Ryan Sleevi, 2017/02/14 19:26:15]

nullptr

[mattm, 2017/02/16 22:06:08]

Done.

+                                &kCFTypeDictionaryValueCallBacks));
+
+  CFDictionarySetValue(query, kSecClass, kSecClassCertificate);
+  CFDictionarySetValue(query, kSecReturnRef, kCFBooleanTrue);
+  CFDictionarySetValue(query, kSecMatchLimit, kSecMatchLimitAll);
+  CFDictionarySetValue(query, kSecAttrSubject, name_data);
+
+  base::ScopedCFTypeRef<CFArrayRef> scoped_alternate_keychain_search_list;
+  if (TestKeychainSearchList::HasInstance()) {
+    CFArrayRef alternate_keychain_search_list;
+    OSStatus status = TestKeychainSearchList::GetInstance()->CopySearchList(
+        &alternate_keychain_search_list);

[Ryan Sleevi, 2017/02/14 19:26:15]

Note .InitializeInto as well

(although perhaps our API for TestKeychainSearchList should take a
base::ScopedCFTypeRef<> explicitly if it's caller-free semantics)

[mattm, 2017/02/16 22:06:08]

Done.
Yeah, it should. I can make a cl for that too.

+    if (status) {
+      OSSTATUS_LOG(ERROR, status)
+          << "TestKeychainSearchList::CopySearchList error";
+      return matching_items;
+    }
+    scoped_alternate_keychain_search_list.reset(alternate_keychain_search_list);
+  }
+  // If a TestKeychainSearchList is present, it will have already set

[Ryan Sleevi, 2017/02/14 19:26:16]

newline between 221/222

[mattm, 2017/02/16 22:06:08]

Done.

+  // |scoped_alternate_keychain_search_list|, which will be used as the
+  // basis for reordering the keychain. Otherwise, get the current keychain
+  // search list and use that.
+  if (!scoped_alternate_keychain_search_list) {
+    CFArrayRef keychain_search_list;
+    OSStatus status = SecKeychainCopySearchList(&keychain_search_list);

[Ryan Sleevi, 2017/02/14 19:26:16]

.InitializeInto as well

[mattm, 2017/02/16 22:06:08]

Done.

+    if (status) {
+      OSSTATUS_LOG(ERROR, status) << "SecKeychainCopySearchList error";
+      return matching_items;
+    }
+    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) {
+    LOG(ERROR) << "CFArrayCreateMutableCopy";
+    return matching_items;
+  }
+  scoped_alternate_keychain_search_list.reset(mutable_keychain_search_list);
+  SecKeychainRef roots_keychain;
+  // The System Roots keychain is not normally searched by SecItemCopyMatching.
+  // Get a reference to it and include in the keychain search list.
+  OSStatus status = SecKeychainOpen(
+      "/System/Library/Keychains/SystemRootCertificates.keychain",
+      &roots_keychain);

[Ryan Sleevi, 2017/02/14 19:26:16]

.InitializeInto :)

[mattm, 2017/02/16 22:06:08]

Done.

+  if (status) {
+    OSSTATUS_LOG(ERROR, status) << "SecKeychainOpen error";
+    return matching_items;
+  }
+  base::ScopedCFTypeRef<SecKeychainRef> scoped_roots_keychain(roots_keychain);
+  CFArrayAppendValue(mutable_keychain_search_list, roots_keychain);
+  CFDictionarySetValue(query, kSecMatchSearchList,
+                       scoped_alternate_keychain_search_list.get());
+
+  OSStatus err;
+  {
+    base::AutoLock lock(crypto::GetMacSecurityServicesLock());
+    err = SecItemCopyMatching(
+        query, reinterpret_cast<CFTypeRef*>(matching_items.InitializeInto()));
+  }
+  if (err == errSecItemNotFound) {
+    DVLOG(2) << "SecItemCopyMatching: no matches found";
+    return matching_items;
+  }
+  if (err) {
+    crypto::LogCSSMError("SecItemCopyMatching", err);

[Ryan Sleevi, 2017/02/14 19:26:16]

|err| isn't guaranteed to be in the CSSM error space. Your line 271 is probably
sufficient.

[mattm, 2017/02/16 22:06:08]

Done.

+    OSSTATUS_LOG(ERROR, err) << "SecItemCopyMatching error";
+    return matching_items;
+  }
+  return matching_items;
+}
+
+// static
+void TrustStoreMac::FilterTrustedCertificates(CFArrayRef matching_items,
+                                              const CFStringRef policy_oid,
+                                              TrustAnchors* out_anchors) {
+  for (CFIndex i = 0, item_count = CFArrayGetCount(matching_items);
+       i < item_count; ++i) {
+    DVLOG(2) << "i=" << i << " of " << item_count;
+    SecCertificateRef match_cert_handle = reinterpret_cast<SecCertificateRef>(
+        const_cast<void*>(CFArrayGetValueAtIndex(matching_items, i)));
+
+    if (!IsSecCertificateTrustedForPolicy(match_cert_handle, policy_oid)) {
+      DVLOG(2) << "not trusted for policy";
+      continue;
+    } else {
+      DVLOG(2) << "trusted!";
+    }
+
+    base::ScopedCFTypeRef<CFDataRef> der_data(
+        SecCertificateCopyData(match_cert_handle));
+    if (!der_data) {
+      LOG(ERROR) << "SecCertificateCopyData error";
+      continue;
+    }
+
+    CertErrors errors;
+    ParseCertificateOptions options;
+    options.allow_invalid_serial_numbers = true;

[Ryan Sleevi, 2017/02/14 19:26:16]

Document? What cert(s) are this? If you have the SHA-1/SHA-256 fingerprint, we
could include crt.sh links here in the documentation to explain why.

[mattm, 2017/02/16 22:06:08]

done.

(The other cert that fails was a com.apple.kerberos.kdc cert that has two
Extended Key Usage extensions.)

+    scoped_refptr<ParsedCertificate> anchor_cert = ParsedCertificate::Create(
+        CFDataGetBytePtr(der_data.get()), CFDataGetLength(der_data.get()),
+        options, &errors);
+    if (!anchor_cert) {
+      // TODO(crbug.com/634443): return errors better.
+      LOG(ERROR) << "Error parsing issuer certificate:\n"
+                 << errors.ToDebugString();
+      continue;
+    }
+
+    DVLOG(2) << "added cert to out_anchors";
+    out_anchors->push_back(TrustAnchor::CreateFromCertificateNoConstraints(
+        std::move(anchor_cert)));
+  }
+}
+
+// static
+base::ScopedCFTypeRef<CFDataRef> TrustStoreMac::GetMacNormalizedIssuer(
+    const scoped_refptr<ParsedCertificate>& cert) {
+  base::ScopedCFTypeRef<CFDataRef> name_data;
+  // There does not appear to be any public API to get the normalized version
+  // of a Name without creating a SecCertificate.
+  X509Certificate::OSCertHandle cert_handle =
+      X509Certificate::CreateOSCertHandleFromBytes(
+          cert->der_cert().AsStringPiece().data(), cert->der_cert().Length());
+  if (!cert_handle) {
+    LOG(ERROR) << "CreateOSCertHandleFromBytes";
+    return name_data;
+  }
+  base::ScopedCFTypeRef<SecCertificateRef> scoped_cert_handle(cert_handle);
+  name_data.reset(
+      SecCertificateCopyNormalizedIssuerContent(cert_handle, nullptr));
+  if (!name_data)
+    LOG(ERROR) << "SecCertificateCopyNormalizedIssuerContent";
+  return name_data;
+}
+
+void TrustStoreMac::FindTrustAnchorsByMacNormalizedSubject(
+    CFDataRef name_data,
+    TrustAnchors* out_anchors) const {
+  base::ScopedCFTypeRef<CFArrayRef> scoped_matching_items =
+      FindMatchingCertificatesForMacNormalizedSubject(name_data);
+  if (!scoped_matching_items)
+    return;
+
+  FilterTrustedCertificates(scoped_matching_items.get(), policy_oid_,
+                            out_anchors);
+}
+
+}  // namespace net