crypto/x509: include roots with empty or multiple policies on macOS

To a fifth reading of the relevant docs, it looks like

1) a constraint dictionary with no policy applies to all of them;
2) multiple applying constraint dictionaries should have their results OR'd;
3) untrusted certificates in the keychain should be used for chain building.

This fixes 1), approximates 2) and punts on 3).

Fixes #30672
Fixes #30471

Change-Id: Ibbaabf0b77d267377c0b5de07abca3445c2c2302
Reviewed-on: https://go-review.googlesource.com/c/go/+/178539
Reviewed-by: Adam Langley <agl@golang.org>

diff --git a/src/crypto/x509/root_cgo_darwin.go b/src/crypto/x509/root_cgo_darwin.go
index e8fc1665f6568763a12b1e9285b69d5f3a687715..255a8d3525ee932f9afd0200e0b13d4a88ef0b55 100644 (file)
--- a/src/crypto/x509/root_cgo_darwin.go
+++ b/src/crypto/x509/root_cgo_darwin.go
@@ -51,6 +51,7 @@ static SInt32 sslTrustSettingsResult(SecCertificateRef cert) {
        }
 
        // > no trust settings [...] means "this certificate must be verified to a known trusted certificate”
+       // (Should this cause a fallback from user to admin domain? It's unclear.)
        if (err != errSecSuccess || trustSettings == NULL) {
                if (trustSettings != NULL) CFRelease(trustSettings);
                return kSecTrustSettingsResultUnspecified;
@@ -77,16 +78,12 @@ static SInt32 sslTrustSettingsResult(SecCertificateRef cert) {
        for (m = 0; m < CFArrayGetCount(trustSettings); m++) {
                CFDictionaryRef tSetting = (CFDictionaryRef)CFArrayGetValueAtIndex(trustSettings, m);
 
-               // First, check if this trust setting applies to our policy. We assume
-               // only one will. The docs suggest that there might be multiple applying
-               // but don't explain how to combine them.
+               // First, check if this trust setting is constrained to a non-SSL policy.
                SecPolicyRef policyRef;
                if (CFDictionaryGetValueIfPresent(tSetting, _kSecTrustSettingsPolicy, (const void**)&policyRef)) {
                        if (!isSSLPolicy(policyRef)) {
                                continue;
                        }
-               } else {
-                       continue;
                }
 
                if (CFDictionaryContainsKey(tSetting, _kSecTrustSettingsPolicyString)) {
@@ -98,13 +95,23 @@ static SInt32 sslTrustSettingsResult(SecCertificateRef cert) {
                if (CFDictionaryGetValueIfPresent(tSetting, _kSecTrustSettingsResult, (const void**)&cfNum)) {
                        CFNumberGetValue(cfNum, kCFNumberSInt32Type, &result);
                } else {
-                       // > If the value of the kSecTrustSettingsResult component is not
-                       // > kSecTrustSettingsResultUnspecified for a usage constraints dictionary that has
-                       // > no constraints, the default value kSecTrustSettingsResultTrustRoot is assumed.
+                       // > If this key is not present, a default value of
+                       // > kSecTrustSettingsResultTrustRoot is assumed.
                        result = kSecTrustSettingsResultTrustRoot;
                }
 
-               break;
+               // If multiple dictionaries match, we are supposed to "OR" them,
+               // the semantics of which are not clear. Since TrustRoot and TrustAsRoot
+               // are mutually exclusive, Deny should probably override, and Invalid and
+               // Unspecified be overridden, approximate this by stopping at the first
+               // TrustRoot, TrustAsRoot or Deny.
+               if (result == kSecTrustSettingsResultTrustRoot) {
+                       break;
+               } else if (result == kSecTrustSettingsResultTrustAsRoot) {
+                       break;
+               } else if (result == kSecTrustSettingsResultDeny) {
+                       break;
+               }
        }
 
        // If trust settings are present, but none of them match the policy...
@@ -244,6 +251,10 @@ int CopyPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugDa
                        } else if (result == kSecTrustSettingsResultDeny) {
                                appendTo = combinedUntrustedData;
                        } else if (result == kSecTrustSettingsResultUnspecified) {
+                               // Certificates with unspecified trust should probably be added to a pool of
+                               // intermediates for chain building, or checked for transitive trust and
+                               // added to the root pool (which is an imprecise approximation because it
+                               // cuts chains short) but we don't support either at the moment. TODO.
                                continue;
                        } else {
                                continue;