From: Filippo Valsorda Date: Tue, 21 May 2019 19:00:50 +0000 (-0400) Subject: crypto/x509: fix and cleanup loadSystemRoots on macOS X-Git-Tag: go1.13beta1~227 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=2326a668781a664707f5775d896879668ab378e8;p=gostls13.git 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 --- diff --git a/src/crypto/x509/root_cgo_darwin.go b/src/crypto/x509/root_cgo_darwin.go index 1c20f26acb..e8fc1665f6 100644 --- 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 1165a97e20..0a1529e833 100644 --- 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() } }