crypto/x509: fix root CA extraction on macOS (cgo path)

The cgo path was not taking policies into account, using the last
security setting in the array whatever it was. Also, it was not aware of
the defaults for empty security settings, and for security settings
without a result type. Finally, certificates restricted to a hostname
were considered roots.

The API docs for this code are partial and not very clear, so this is a
best effort, really.

Updates #24652

Change-Id: I8fa2fe4706f44f3d963b32e0615d149e997b537d
Reviewed-on: https://go-review.googlesource.com/c/128056
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@golang.org>

src/crypto/x509/root_cgo_darwin.go

@@ -16,7 +16,135 @@ package x509
 #include <CoreFoundation/CoreFoundation.h>
 #include <Security/Security.h>
 
-// FetchPEMRoots fetches the system's list of trusted X.509 root certificates.
+static bool isSSLPolicy(SecPolicyRef policyRef) {
+	if (!policyRef) {
+		return false;
+	}
+	CFDictionaryRef properties = SecPolicyCopyProperties(policyRef);
+	if (properties == NULL) {
+		return false;
+	}
+	CFTypeRef value = NULL;
+	if (CFDictionaryGetValueIfPresent(properties, kSecPolicyOid, (const void **)&value)) {
+		CFRelease(properties);
+		return CFEqual(value, kSecPolicyAppleSSL);
+	}
+	CFRelease(properties);
+	return false;
+}
+
+// sslTrustSettingsResult obtains the final kSecTrustSettingsResult value
+// for a certificate in the user or admin domain, combining usage constraints
+// for the SSL SecTrustSettingsPolicy, ignoring SecTrustSettingsKeyUsage and
+// kSecTrustSettingsAllowedError.
+// https://developer.apple.com/documentation/security/1400261-sectrustsettingscopytrustsetting
+static SInt32 sslTrustSettingsResult(SecCertificateRef cert) {
+	CFArrayRef trustSettings = NULL;
+	OSStatus err = SecTrustSettingsCopyTrustSettings(cert, kSecTrustSettingsDomainUser, &trustSettings);
+
+	// According to Apple's SecTrustServer.c, "user trust settings overrule admin trust settings",
+	// but the rules of the override are unclear. Let's assume admin trust settings are applicable
+	// if and only if user trust settings fail to load or are NULL.
+	if (err != errSecSuccess || trustSettings == NULL) {
+		if (trustSettings != NULL) CFRelease(trustSettings);
+		err = SecTrustSettingsCopyTrustSettings(cert, kSecTrustSettingsDomainAdmin, &trustSettings);
+	}
+
+	// > no trust settings [...] means "this certificate must be verified to a known trusted certificate”
+	if (err != errSecSuccess || trustSettings == NULL) {
+		if (trustSettings != NULL) CFRelease(trustSettings);
+		return kSecTrustSettingsResultUnspecified;
+	}
+
+	// > An empty trust settings array means "always trust this certificate” with an
+	// > overall trust setting for the certificate of kSecTrustSettingsResultTrustRoot.
+	if (CFArrayGetCount(trustSettings) == 0) {
+		CFRelease(trustSettings);
+		return kSecTrustSettingsResultTrustRoot;
+	}
+
+	// kSecTrustSettingsResult is defined as CFSTR("kSecTrustSettingsResult"),
+	// but the Go linker's internal linking mode can't handle CFSTR relocations.
+	// Create our own dynamic string instead and release it below.
+	CFStringRef _kSecTrustSettingsResult = CFStringCreateWithCString(
+		NULL, "kSecTrustSettingsResult", kCFStringEncodingUTF8);
+	CFStringRef _kSecTrustSettingsPolicy = CFStringCreateWithCString(
+		NULL, "kSecTrustSettingsPolicy", kCFStringEncodingUTF8);
+	CFStringRef _kSecTrustSettingsPolicyString = CFStringCreateWithCString(
+		NULL, "kSecTrustSettingsPolicyString", kCFStringEncodingUTF8);
+
+	CFIndex m; SInt32 result = 0;
+	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.
+		SecPolicyRef policyRef;
+		if (CFDictionaryGetValueIfPresent(tSetting, _kSecTrustSettingsPolicy, (const void**)&policyRef)) {
+			if (!isSSLPolicy(policyRef)) {
+				continue;
+			}
+		} else {
+			continue;
+		}
+
+		if (CFDictionaryContainsKey(tSetting, _kSecTrustSettingsPolicyString)) {
+			// Restricted to a hostname, not a root.
+			continue;
+		}
+
+		CFNumberRef cfNum;
+		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.
+			result = kSecTrustSettingsResultTrustRoot;
+		}
+
+		break;
+	}
+
+	// If trust settings are present, but none of them match the policy...
+	// the docs don't tell us what to do.
+	//
+	// "Trust settings for a given use apply if any of the dictionaries in the
+	// certificate’s trust settings array satisfies the specified use." suggests
+	// that it's as if there were no trust settings at all, so we should probably
+	// fallback to the admin trust settings. TODO.
+	if (result == 0) {
+		result = kSecTrustSettingsResultUnspecified;
+	}
+
+	CFRelease(_kSecTrustSettingsPolicy);
+	CFRelease(_kSecTrustSettingsPolicyString);
+	CFRelease(_kSecTrustSettingsResult);
+	CFRelease(trustSettings);
+
+	return result;
+}
+
+// isRootCertificate reports whether Subject and Issuer match.
+static Boolean isRootCertificate(SecCertificateRef cert, CFErrorRef *errRef) {
+	CFDataRef subjectName = SecCertificateCopyNormalizedSubjectContent(cert, errRef);
+	if (*errRef != NULL) {
+		return false;
+	}
+	CFDataRef issuerName = SecCertificateCopyNormalizedIssuerContent(cert, errRef);
+	if (*errRef != NULL) {
+		CFRelease(subjectName);
+		return false;
+	}
+	Boolean equal = CFEqual(subjectName, issuerName);
+	CFRelease(subjectName);
+	CFRelease(issuerName);
+	return equal;
+}
+
+// FetchPEMRoots fetches the system's list of trusted X.509 root certificates
+// for the kSecTrustSettingsPolicy SSL.
 //
 // On success it returns 0 and fills pemRoots with a CFDataRef that contains the extracted root
 // certificates of the system. On failure, the function returns -1.
@@ -24,26 +152,28 @@ package x509
 //
 // Note: The CFDataRef returned in pemRoots and untrustedPemRoots must
 // be released (using CFRelease) after we've consumed its content.
-int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots) {
+int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugDarwinRoots) {
 	int i;
 
+	if (debugDarwinRoots) {
+		printf("crypto/x509: kSecTrustSettingsResultInvalid = %d\n", kSecTrustSettingsResultInvalid);
+		printf("crypto/x509: kSecTrustSettingsResultTrustRoot = %d\n", kSecTrustSettingsResultTrustRoot);
+		printf("crypto/x509: kSecTrustSettingsResultTrustAsRoot = %d\n", kSecTrustSettingsResultTrustAsRoot);
+		printf("crypto/x509: kSecTrustSettingsResultDeny = %d\n", kSecTrustSettingsResultDeny);
+		printf("crypto/x509: kSecTrustSettingsResultUnspecified = %d\n", kSecTrustSettingsResultUnspecified);
+	}
+
 	// Get certificates from all domains, not just System, this lets
 	// the user add CAs to their "login" keychain, and Admins to add
 	// to the "System" keychain
 	SecTrustSettingsDomain domains[] = { kSecTrustSettingsDomainSystem,
-					     kSecTrustSettingsDomainAdmin,
-					     kSecTrustSettingsDomainUser };
+		kSecTrustSettingsDomainAdmin, kSecTrustSettingsDomainUser };
 
 	int numDomains = sizeof(domains)/sizeof(SecTrustSettingsDomain);
 	if (pemRoots == NULL) {
 		return -1;
 	}
 
-	// kSecTrustSettingsResult is defined as CFSTR("kSecTrustSettingsResult"),
-	// but the Go linker's internal linking mode can't handle CFSTR relocations.
-	// Create our own dynamic string instead and release it below.
-	CFStringRef policy = CFStringCreateWithCString(NULL, "kSecTrustSettingsResult", kCFStringEncodingUTF8);
-
 	CFMutableDataRef combinedData = CFDataCreateMutable(kCFAllocatorDefault, 0);
 	CFMutableDataRef combinedUntrustedData = CFDataCreateMutable(kCFAllocatorDefault, 0);
 	for (i = 0; i < numDomains; i++) {
@@ -57,102 +187,81 @@ int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots) {
 		CFIndex numCerts = CFArrayGetCount(certs);
 		for (j = 0; j < numCerts; j++) {
 			CFDataRef data = NULL;
-			CFErrorRef errRef = NULL;
 			CFArrayRef trustSettings = NULL;
 			SecCertificateRef cert = (SecCertificateRef)CFArrayGetValueAtIndex(certs, j);
 			if (cert == NULL) {
 				continue;
 			}
-			// We only want trusted certs.
-			int untrusted = 0;
-			int trustAsRoot = 0;
-			int trustRoot = 0;
-			if (i == 0) {
-				trustAsRoot = 1;
-			} else {
-				int k;
-				CFIndex m;
 
+			SInt32 result;
+			if (domains[i] == kSecTrustSettingsDomainSystem) {
 				// Certs found in the system domain are always trusted. If the user
 				// configures "Never Trust" on such a cert, it will also be found in the
 				// admin or user domain, causing it to be added to untrustedPemRoots. The
 				// Go code will then clean this up.
-
-				// Trust may be stored in any of the domains. According to Apple's
-				// SecTrustServer.c, "user trust settings overrule admin trust settings",
-				// so take the last trust settings array we find.
-				// Skip the system domain since it is always trusted.
-				for (k = i; k < numDomains; k++) {
-					CFArrayRef domainTrustSettings = NULL;
-					err = SecTrustSettingsCopyTrustSettings(cert, domains[k], &domainTrustSettings);
-					if (err == errSecSuccess && domainTrustSettings != NULL) {
-						if (trustSettings) {
-							CFRelease(trustSettings);
-						}
-						trustSettings = domainTrustSettings;
-					}
-				}
-				if (trustSettings == NULL) {
-					// "this certificate must be verified to a known trusted certificate"; aka not a root.
+				result = kSecTrustSettingsResultTrustRoot;
+			} else {
+				result = sslTrustSettingsResult(cert);
+				if (debugDarwinRoots) {
+					CFErrorRef errRef = NULL;
+					CFStringRef summary = SecCertificateCopyShortDescription(NULL, cert, &errRef);
+					if (errRef != NULL) {
+						printf("crypto/x509: SecCertificateCopyShortDescription failed\n");
+						CFRelease(errRef);
 						continue;
 					}
-				for (m = 0; m < CFArrayGetCount(trustSettings); m++) {
-					CFNumberRef cfNum;
-					CFDictionaryRef tSetting = (CFDictionaryRef)CFArrayGetValueAtIndex(trustSettings, m);
-					if (CFDictionaryGetValueIfPresent(tSetting, policy, (const void**)&cfNum)){
-						SInt32 result = 0;
-						CFNumberGetValue(cfNum, kCFNumberSInt32Type, &result);
-						// TODO: The rest of the dictionary specifies conditions for evaluation.
-						if (result == kSecTrustSettingsResultDeny) {
-							untrusted = 1;
-						} else if (result == kSecTrustSettingsResultTrustAsRoot) {
-							trustAsRoot = 1;
-						} else if (result == kSecTrustSettingsResultTrustRoot) {
-							trustRoot = 1;
-						}
+
+					CFIndex length = CFStringGetLength(summary);
+					CFIndex maxSize = CFStringGetMaximumSizeForEncoding(length, kCFStringEncodingUTF8) + 1;
+					char *buffer = malloc(maxSize);
+					if (CFStringGetCString(summary, buffer, maxSize, kCFStringEncodingUTF8)) {
+						printf("crypto/x509: %s returned %d\n", buffer, result);
 					}
+					free(buffer);
+					CFRelease(summary);
 				}
-				CFRelease(trustSettings);
 			}
 
-			if (trustRoot) {
-				// We only want to add Root CAs, so make sure Subject and Issuer Name match
-				CFDataRef subjectName = SecCertificateCopyNormalizedSubjectContent(cert, &errRef);
-				if (errRef != NULL) {
-					CFRelease(errRef);
+			CFMutableDataRef appendTo;
+			// > Note the distinction between the results kSecTrustSettingsResultTrustRoot
+			// > and kSecTrustSettingsResultTrustAsRoot: The former can only be applied to
+			// > root (self-signed) certificates; the latter can only be applied to
+			// > non-root certificates.
+			if (result == kSecTrustSettingsResultTrustRoot) {
+				CFErrorRef errRef = NULL;
+				if (!isRootCertificate(cert, &errRef) || errRef != NULL) {
+					if (errRef != NULL) CFRelease(errRef);
 					continue;
 				}
-				CFDataRef issuerName = SecCertificateCopyNormalizedIssuerContent(cert, &errRef);
-				if (errRef != NULL) {
-					CFRelease(subjectName);
-					CFRelease(errRef);
+
+				appendTo = combinedData;
+			} else if (result == kSecTrustSettingsResultTrustAsRoot) {
+				CFErrorRef errRef = NULL;
+				if (isRootCertificate(cert, &errRef) || errRef != NULL) {
+					if (errRef != NULL) CFRelease(errRef);
 					continue;
 				}
-				Boolean equal = CFEqual(subjectName, issuerName);
-				CFRelease(subjectName);
-				CFRelease(issuerName);
-				if (!equal) {
+
+				appendTo = combinedData;
+			} else if (result == kSecTrustSettingsResultDeny) {
+				appendTo = combinedUntrustedData;
+			} else if (result == kSecTrustSettingsResultUnspecified) {
+				continue;
+			} else {
 				continue;
-				}
 			}
 
 			err = SecItemExport(cert, kSecFormatX509Cert, kSecItemPemArmour, NULL, &data);
 			if (err != noErr) {
 				continue;
 			}
-
 			if (data != NULL) {
-				if (!trustRoot && !trustAsRoot) {
-					untrusted = 1;
-				}
-				CFMutableDataRef appendTo = untrusted ? combinedUntrustedData : combinedData;
 				CFDataAppendBytes(appendTo, CFDataGetBytePtr(data), CFDataGetLength(data));
 				CFRelease(data);
 			}
 		}
 		CFRelease(certs);
 	}
-	CFRelease(policy);
 	*pemRoots = combinedData;
 	*untrustedPemRoots = combinedUntrustedData;
 	return 0;
@@ -169,9 +278,8 @@ func loadSystemRoots() (*CertPool, error) {
 
 	var data C.CFDataRef = 0
 	var untrustedData C.CFDataRef = 0
-	err := C.FetchPEMRoots(&data, &untrustedData)
+	err := C.FetchPEMRoots(&data, &untrustedData, C.bool(debugDarwinRoots))
 	if err == -1 {
-		// TODO: better error message
 		return nil, errors.New("crypto/x509: failed to load darwin system roots with cgo")
 	}
 

src/crypto/x509/root_darwin.go

@@ -21,7 +21,7 @@ import (
 	"sync"
 )
 
-var debugExecDarwinRoots = strings.Contains(os.Getenv("GODEBUG"), "x509roots=1")
+var debugDarwinRoots = strings.Contains(os.Getenv("GODEBUG"), "x509roots=1")
 
 func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate, err error) {
 	return nil, nil
@@ -57,7 +57,7 @@ func execSecurityRoots() (*CertPool, error) {
 	if err != nil {
 		return nil, err
 	}
-	if debugExecDarwinRoots {
+	if debugDarwinRoots {
 		println(fmt.Sprintf("crypto/x509: %d certs have a trust policy", len(hasPolicy)))
 	}
 
@@ -68,8 +68,8 @@ func execSecurityRoots() (*CertPool, error) {
 
 	home, err := os.UserHomeDir()
 	if err != nil {
-		if debugExecDarwinRoots {
-			println("crypto/x509: can't get user home directory: %v", err)
+		if debugDarwinRoots {
+			println(fmt.Sprintf("crypto/x509: can't get user home directory: %v", err))
 		}
 	} else {
 		args = append(args,
@@ -147,7 +147,7 @@ func execSecurityRoots() (*CertPool, error) {
 	close(blockCh)
 	wg.Wait()
 
-	if debugExecDarwinRoots {
+	if debugDarwinRoots {
 		mu.Lock()
 		defer mu.Unlock()
 		println(fmt.Sprintf("crypto/x509: ran security verify-cert %d times", numVerified))
@@ -175,16 +175,16 @@ func verifyCertWithSystem(block *pem.Block, cert *Certificate) bool {
 	}
 	cmd := exec.Command("/usr/bin/security", "verify-cert", "-c", f.Name(), "-l", "-L")
 	var stderr bytes.Buffer
-	if debugExecDarwinRoots {
+	if debugDarwinRoots {
 		cmd.Stderr = &stderr
 	}
 	if err := cmd.Run(); err != nil {
-		if debugExecDarwinRoots {
+		if debugDarwinRoots {
 			println(fmt.Sprintf("crypto/x509: verify-cert rejected %s: %q", cert.Subject, bytes.TrimSpace(stderr.Bytes())))
 		}
 		return false
 	}
-	if debugExecDarwinRoots {
+	if debugDarwinRoots {
 		println(fmt.Sprintf("crypto/x509: verify-cert approved %s", cert.Subject))
 	}
 	return true
@@ -217,7 +217,7 @@ func getCertsWithTrustPolicy() (map[string]bool, error) {
 			// Rather than match on English substrings that are probably
 			// localized on macOS, just interpret any failure to mean that
 			// there are no trust settings.
-			if debugExecDarwinRoots {
+			if debugDarwinRoots {
 				println(fmt.Sprintf("crypto/x509: exec %q: %v, %s", cmd.Args, err, stderr.Bytes()))
 			}
 			return nil