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

Certificates without any trust settings might still be in the keychain
(for example if they used to have some, or if they are intermediates for
offline verification), but they are not to be trusted. The only ones we
can trust unconditionally are the ones in the system roots store.

Moreover, the verify-cert invocation was not specifying the ssl policy,
defaulting instead to the basic one. We have no way of communicating
different usages in a CertPool, so stick to the WebPKI use-case as the
primary one for crypto/x509.

Updates #24652

Change-Id: Ife8b3d2f4026daa1223aa81fac44aeeb4f96528a
Reviewed-on: https://go-review.googlesource.com/c/128116Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@golang.org>

src/crypto/x509/root_darwin.go

@@ -39,8 +39,8 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate
 //
 // 1. Run "security trust-settings-export" and "security
 //    trust-settings-export -d" to discover the set of certs with some
-//    user-tweaked trust policy. We're too lazy to parse the XML (at
-//    least at this stage of Go 1.8) to understand what the trust
+//    user-tweaked trust policy. We're too lazy to parse the XML
+//    (Issue 26830) to understand what the trust
 //    policy actually is. We just learn that there is _some_ policy.
 //
 // 2. Run "security find-certificate" to dump the list of system root
@@ -58,21 +58,20 @@ func execSecurityRoots() (*CertPool, error) {
 		return nil, err
 	}
 	if debugDarwinRoots {
-		println(fmt.Sprintf("crypto/x509: %d certs have a trust policy", len(hasPolicy)))
+		fmt.Printf("crypto/x509: %d certs have a trust policy\n", len(hasPolicy))
 	}
 
-	args := []string{"find-certificate", "-a", "-p",
-		"/System/Library/Keychains/SystemRootCertificates.keychain",
-		"/Library/Keychains/System.keychain",
-	}
+	keychains := []string{"/Library/Keychains/System.keychain"}
 
+	// Note that this results in trusting roots from $HOME/... (the environment
+	// variable), which might not be expected.
 	home, err := os.UserHomeDir()
 	if err != nil {
 		if debugDarwinRoots {
-			println(fmt.Sprintf("crypto/x509: can't get user home directory: %v", err))
+			fmt.Printf("crypto/x509: can't get user home directory: %v\n", err)
 		}
 	} else {
-		args = append(args,
+		keychains = append(keychains,
 			filepath.Join(home, "/Library/Keychains/login.keychain"),
 
 			// Fresh installs of Sierra use a slightly different path for the login keychain
@@ -80,21 +79,19 @@ func execSecurityRoots() (*CertPool, error) {
 		)
 	}
 
-	cmd := exec.Command("/usr/bin/security", args...)
-	data, err := cmd.Output()
-	if err != nil {
-		return nil, err
+	type rootCandidate struct {
+		c      *Certificate
+		system bool
 	}
 
 	var (
 		mu          sync.Mutex
 		roots       = NewCertPool()
 		numVerified int // number of execs of 'security verify-cert', for debug stats
+		wg          sync.WaitGroup
+		verifyCh    = make(chan rootCandidate)
 	)
 
-	blockCh := make(chan *pem.Block)
-	var wg sync.WaitGroup
-
 	// Using 4 goroutines to pipe into verify-cert seems to be
 	// about the best we can do. The verify-cert binary seems to
 	// just RPC to another server with coarse locking anyway, so
@@ -108,31 +105,62 @@ func execSecurityRoots() (*CertPool, error) {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
-			for block := range blockCh {
-				cert, err := ParseCertificate(block.Bytes)
-				if err != nil {
-					continue
-				}
-				sha1CapHex := fmt.Sprintf("%X", sha1.Sum(block.Bytes))
+			for cert := range verifyCh {
+				sha1CapHex := fmt.Sprintf("%X", sha1.Sum(cert.c.Raw))
 
-				valid := true
+				var valid bool
 				verifyChecks := 0
 				if hasPolicy[sha1CapHex] {
 					verifyChecks++
-					if !verifyCertWithSystem(block, cert) {
-						valid = false
-					}
+					valid = verifyCertWithSystem(cert.c)
+				} else {
+					// Certificates not in SystemRootCertificates without user
+					// or admin trust settings are not trusted.
+					valid = cert.system
 				}
 
 				mu.Lock()
 				numVerified += verifyChecks
 				if valid {
-					roots.AddCert(cert)
+					roots.AddCert(cert.c)
 				}
 				mu.Unlock()
 			}
 		}()
 	}
+	err = forEachCertInKeychains(keychains, func(cert *Certificate) {
+		verifyCh <- rootCandidate{c: cert, system: false}
+	})
+	if err != nil {
+		close(verifyCh)
+		return nil, err
+	}
+	err = forEachCertInKeychains([]string{
+		"/System/Library/Keychains/SystemRootCertificates.keychain",
+	}, func(cert *Certificate) {
+		verifyCh <- rootCandidate{c: cert, system: true}
+	})
+	if err != nil {
+		close(verifyCh)
+		return nil, err
+	}
+	close(verifyCh)
+	wg.Wait()
+
+	if debugDarwinRoots {
+		fmt.Printf("crypto/x509: ran security verify-cert %d times\n", numVerified)
+	}
+
+	return roots, nil
+}
+
+func forEachCertInKeychains(paths []string, f func(*Certificate)) error {
+	args := append([]string{"find-certificate", "-a", "-p"}, paths...)
+	cmd := exec.Command("/usr/bin/security", args...)
+	data, err := cmd.Output()
+	if err != nil {
+		return err
+	}
 	for len(data) > 0 {
 		var block *pem.Block
 		block, data = pem.Decode(data)
@@ -142,22 +170,19 @@ func execSecurityRoots() (*CertPool, error) {
 		if block.Type != "CERTIFICATE" || len(block.Headers) != 0 {
 			continue
 		}
-		blockCh <- block
-	}
-	close(blockCh)
-	wg.Wait()
-
-	if debugDarwinRoots {
-		mu.Lock()
-		defer mu.Unlock()
-		println(fmt.Sprintf("crypto/x509: ran security verify-cert %d times", numVerified))
+		cert, err := ParseCertificate(block.Bytes)
+		if err != nil {
+			continue
+		}
+		f(cert)
 	}
-
-	return roots, nil
+	return nil
 }
 
-func verifyCertWithSystem(block *pem.Block, cert *Certificate) bool {
-	data := pem.EncodeToMemory(block)
+func verifyCertWithSystem(cert *Certificate) bool {
+	data := pem.EncodeToMemory(&pem.Block{
+		Type: "CERTIFICATE", Bytes: cert.Raw,
+	})
 
 	f, err := ioutil.TempFile("", "cert")
 	if err != nil {
@@ -173,19 +198,19 @@ func verifyCertWithSystem(block *pem.Block, cert *Certificate) bool {
 		fmt.Fprintf(os.Stderr, "can't write temporary file for cert: %v", err)
 		return false
 	}
-	cmd := exec.Command("/usr/bin/security", "verify-cert", "-c", f.Name(), "-l", "-L")
+	cmd := exec.Command("/usr/bin/security", "verify-cert", "-p", "ssl", "-c", f.Name(), "-l", "-L")
 	var stderr bytes.Buffer
 	if debugDarwinRoots {
 		cmd.Stderr = &stderr
 	}
 	if err := cmd.Run(); err != nil {
 		if debugDarwinRoots {
-			println(fmt.Sprintf("crypto/x509: verify-cert rejected %s: %q", cert.Subject, bytes.TrimSpace(stderr.Bytes())))
+			fmt.Printf("crypto/x509: verify-cert rejected %s: %q\n", cert.Subject, bytes.TrimSpace(stderr.Bytes()))
 		}
 		return false
 	}
 	if debugDarwinRoots {
-		println(fmt.Sprintf("crypto/x509: verify-cert approved %s", cert.Subject))
+		fmt.Printf("crypto/x509: verify-cert approved %s\n", cert.Subject)
 	}
 	return true
 }
@@ -218,7 +243,7 @@ func getCertsWithTrustPolicy() (map[string]bool, error) {
 			// localized on macOS, just interpret any failure to mean that
 			// there are no trust settings.
 			if debugDarwinRoots {
-				println(fmt.Sprintf("crypto/x509: exec %q: %v, %s", cmd.Args, err, stderr.Bytes()))
+				fmt.Printf("crypto/x509: exec %q: %v, %s\n", cmd.Args, err, stderr.Bytes())
 			}
 			return nil
 		}