crypto/x509: fix and cleanup loadSystemRoots on macOS

Note how untrustedData is never NULL, so loadSystemRoots was checking
the wrong thing.

Also, renamed the C function to CopyPEMRoots to follow the
CoreFoundation naming convention on ownership.

Finally, redirect all debug output to standard error.

Change-Id: Ie80abefadf8974a75c0646aa02fcfcebcbe3bde8
Reviewed-on: https://go-review.googlesource.com/c/go/+/178538
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 1c20f26acb0c51efd35bfee8b28355adf5e844b0..e8fc1665f6568763a12b1e9285b69d5f3a687715 100644 (file)
--- a/src/crypto/x509/root_cgo_darwin.go
+++ b/src/crypto/x509/root_cgo_darwin.go
@@ -143,7 +143,7 @@ static Boolean isRootCertificate(SecCertificateRef cert, CFErrorRef *errRef) {
        return equal;
 }
 
-// FetchPEMRoots fetches the system's list of trusted X.509 root certificates
+// CopyPEMRoots 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
@@ -152,15 +152,15 @@ static Boolean isRootCertificate(SecCertificateRef cert, CFErrorRef *errRef) {
 //
 // 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, bool debugDarwinRoots) {
+int CopyPEMRoots(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);
+               fprintf(stderr, "crypto/x509: kSecTrustSettingsResultInvalid = %d\n", kSecTrustSettingsResultInvalid);
+               fprintf(stderr, "crypto/x509: kSecTrustSettingsResultTrustRoot = %d\n", kSecTrustSettingsResultTrustRoot);
+               fprintf(stderr, "crypto/x509: kSecTrustSettingsResultTrustAsRoot = %d\n", kSecTrustSettingsResultTrustAsRoot);
+               fprintf(stderr, "crypto/x509: kSecTrustSettingsResultDeny = %d\n", kSecTrustSettingsResultDeny);
+               fprintf(stderr, "crypto/x509: kSecTrustSettingsResultUnspecified = %d\n", kSecTrustSettingsResultUnspecified);
        }
 
        // Get certificates from all domains, not just System, this lets
@@ -170,7 +170,7 @@ int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugD
                kSecTrustSettingsDomainAdmin, kSecTrustSettingsDomainUser };
 
        int numDomains = sizeof(domains)/sizeof(SecTrustSettingsDomain);
-       if (pemRoots == NULL) {
+       if (pemRoots == NULL || untrustedPemRoots == NULL) {
                return -1;
        }
 
@@ -186,8 +186,6 @@ int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugD
 
                CFIndex numCerts = CFArrayGetCount(certs);
                for (j = 0; j < numCerts; j++) {
-                       CFDataRef data = NULL;
-                       CFArrayRef trustSettings = NULL;
                        SecCertificateRef cert = (SecCertificateRef)CFArrayGetValueAtIndex(certs, j);
                        if (cert == NULL) {
                                continue;
@@ -206,7 +204,7 @@ int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugD
                                        CFErrorRef errRef = NULL;
                                        CFStringRef summary = SecCertificateCopyShortDescription(NULL, cert, &errRef);
                                        if (errRef != NULL) {
-                                               printf("crypto/x509: SecCertificateCopyShortDescription failed\n");
+                                               fprintf(stderr, "crypto/x509: SecCertificateCopyShortDescription failed\n");
                                                CFRelease(errRef);
                                                continue;
                                        }
@@ -215,7 +213,7 @@ int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugD
                                        CFIndex maxSize = CFStringGetMaximumSizeForEncoding(length, kCFStringEncodingUTF8) + 1;
                                        char *buffer = malloc(maxSize);
                                        if (CFStringGetCString(summary, buffer, maxSize, kCFStringEncodingUTF8)) {
-                                               printf("crypto/x509: %s returned %d\n", buffer, (int)result);
+                                               fprintf(stderr, "crypto/x509: %s returned %d\n", buffer, (int)result);
                                        }
                                        free(buffer);
                                        CFRelease(summary);
@@ -251,6 +249,7 @@ int FetchPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugD
                                continue;
                        }
 
+                       CFDataRef data = NULL;
                        err = SecItemExport(cert, kSecFormatX509Cert, kSecItemPemArmour, NULL, &data);
                        if (err != noErr) {
                                continue;
@@ -274,22 +273,22 @@ import (
 )
 
 func loadSystemRoots() (*CertPool, error) {
-       roots := NewCertPool()
-
-       var data C.CFDataRef = 0
-       var untrustedData C.CFDataRef = 0
-       err := C.FetchPEMRoots(&data, &untrustedData, C.bool(debugDarwinRoots))
+       var data, untrustedData C.CFDataRef
+       err := C.CopyPEMRoots(&data, &untrustedData, C.bool(debugDarwinRoots))
        if err == -1 {
                return nil, errors.New("crypto/x509: failed to load darwin system roots with cgo")
        }
-
        defer C.CFRelease(C.CFTypeRef(data))
+       defer C.CFRelease(C.CFTypeRef(untrustedData))
+
        buf := C.GoBytes(unsafe.Pointer(C.CFDataGetBytePtr(data)), C.int(C.CFDataGetLength(data)))
+       roots := NewCertPool()
        roots.AppendCertsFromPEM(buf)
-       if untrustedData == 0 {
+
+       if C.CFDataGetLength(untrustedData) == 0 {
                return roots, nil
        }
-       defer C.CFRelease(C.CFTypeRef(untrustedData))
+
        buf = C.GoBytes(unsafe.Pointer(C.CFDataGetBytePtr(untrustedData)), C.int(C.CFDataGetLength(untrustedData)))
        untrustedRoots := NewCertPool()
        untrustedRoots.AppendCertsFromPEM(buf)

diff --git a/src/crypto/x509/root_darwin_test.go b/src/crypto/x509/root_darwin_test.go
index 1165a97e205b95ebe9616b4f62390bdd5608e321..0a1529e833cf97b5e454a1a0b42e9c1fb077a762 100644 (file)
--- a/src/crypto/x509/root_darwin_test.go
+++ b/src/crypto/x509/root_darwin_test.go
@@ -120,12 +120,10 @@ func TestSystemRoots(t *testing.T) {
 
        if t.Failed() && debugDarwinRoots {
                cmd := exec.Command("security", "dump-trust-settings")
-               cmd.Stdout = os.Stdout
-               cmd.Stderr = os.Stderr
+               cmd.Stdout, cmd.Stderr = os.Stderr, os.Stderr
                cmd.Run()
                cmd = exec.Command("security", "dump-trust-settings", "-d")
-               cmd.Stdout = os.Stdout
-               cmd.Stderr = os.Stderr
+               cmd.Stdout, cmd.Stderr = os.Stderr, os.Stderr
                cmd.Run()
        }
 }