Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/x509: parse macOS keychain policies instead of using verify-cert #24084

Closed
adamdecaf opened this issue Feb 23, 2018 · 8 comments
Closed

crypto/x509: parse macOS keychain policies instead of using verify-cert #24084

adamdecaf opened this issue Feb 23, 2018 · 8 comments

Comments

@adamdecaf
Copy link
Contributor

@adamdecaf adamdecaf commented Feb 23, 2018

On Darwin systems there's a non-cgo path using the provided security cli tool for gathering certificates to use. (e.g. x509.SystemCertPool()) The current code calls security verify-cert which searches keychains on the filesystem for policies related to the certificate in an attempt to respect trust settings applied.

One can apply a trust setting that would disable "ssl", but Go currently doesn't parse that out of the keychain.

sudo /usr/bin/security add-trusted-cert -d -r deny -p ssl -k /Library/Keychains/System.keychain cert.pem

The problem is that security verify-cert doesn't return kSecTrustSettingsResultDeny if the certificate is only denied for ssl. The flag -p is defaulted to basic (instead of say ssl or codeSign).

This can be verified with the following patch in src/crypto/x509/root_darwin.go.

-    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")

This doesn't solve the problem outside of TLS though. There are multiple policies which can be applied to selectively disable certificates for. The trust policies are better stated in the output of security trust-settings-export, which is already called, but the response not fully parsed.

@adamdecaf
Copy link
Contributor Author

@adamdecaf adamdecaf commented Feb 23, 2018

I talked with @agl and he suggested we could look into attaching the extracted policies onto each certificate and then check those in Verify.

A certificate with Never Trust set only for ssl looks like the following from security trust-settings-export. I'm working on xml parsing logic for this, but it's not pretty.

		<key>D1EB23A46D17D68FD92564C2F1F1601764D8E349</key>
		<dict>
			<key>issuerName</key>
			<data>
			MHsxCzAJBgNVBAYTAkdCMRswGQYDVQQIDBJHcmVhdGVyIE1hbmNo
			ZXN0ZXIxEDAOBgNVBAcMB1NhbGZvcmQxGjAYBgNVBAoMEUNvbW9k
			byBDQSBMaW1pdGVkMSEwHwYDVQQDDBhBQUEgQ2VydGlmaWNhdGUg
			U2VydmljZXM=
			</data>
			<key>modDate</key>
			<date>2018-02-23T18:45:32Z</date>
			<key>serialNumber</key>
			<data>
			AQ==
			</data>
			<key>trustSettings</key>
			<array>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147409654</integer>
					<key>kSecTrustSettingsPolicy</key>
					<data>
					KoZIhvdjZAED
					</data>
					<key>kSecTrustSettingsPolicyName</key>
					<string>sslServer</string>
					<key>kSecTrustSettingsResult</key>
					<integer>3</integer>
				</dict>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147408896</integer>
					<key>kSecTrustSettingsPolicy</key>
					<data>
					KoZIhvdjZAED
					</data>
					<key>kSecTrustSettingsPolicyName</key>
					<string>sslServer</string>
					<key>kSecTrustSettingsResult</key>
					<integer>3</integer>
				</dict>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147409654</integer>
					<key>kSecTrustSettingsPolicy</key>
					<data>
					KoZIhvdjZAEI
					</data>
					<key>kSecTrustSettingsPolicyName</key>
					<string>SMIME</string>
					<key>kSecTrustSettingsResult</key>
					<integer>4</integer>
				</dict>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147408872</integer>
					<key>kSecTrustSettingsPolicy</key>
					<data>
					KoZIhvdjZAEI
					</data>
					<key>kSecTrustSettingsPolicyName</key>
					<string>SMIME</string>
					<key>kSecTrustSettingsResult</key>
					<integer>4</integer>
				</dict>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147409654</integer>
					<key>kSecTrustSettingsPolicy</key>
					<data>
					KoZIhvdjZAEJ
					</data>
					<key>kSecTrustSettingsPolicyName</key>
					<string>eapServer</string>
					<key>kSecTrustSettingsResult</key>
					<integer>4</integer>
				</dict>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147409654</integer>
					<key>kSecTrustSettingsPolicy</key>
					<data>
					KoZIhvdjZAEL
					</data>
					<key>kSecTrustSettingsPolicyName</key>
					<string>ipsecServer</string>
					<key>kSecTrustSettingsResult</key>
					<integer>4</integer>
				</dict>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147409654</integer>
					<key>kSecTrustSettingsPolicy</key>
					<data>
					KoZIhvdjZAEQ
					</data>
					<key>kSecTrustSettingsPolicyName</key>
					<string>CodeSigning</string>
					<key>kSecTrustSettingsResult</key>
					<integer>4</integer>
				</dict>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147409654</integer>
					<key>kSecTrustSettingsPolicy</key>
					<data>
					KoZIhvdjZAEU
					</data>
					<key>kSecTrustSettingsPolicyName</key>
					<string>AppleTimeStamping</string>
					<key>kSecTrustSettingsResult</key>
					<integer>4</integer>
				</dict>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147409654</integer>
					<key>kSecTrustSettingsPolicy</key>
					<data>
					KoZIhvdjZAEC
					</data>
					<key>kSecTrustSettingsPolicyName</key>
					<string>basicX509</string>
					<key>kSecTrustSettingsResult</key>
					<integer>4</integer>
				</dict>
				<dict>
					<key>kSecTrustSettingsAllowedError</key>
					<integer>-2147409654</integer>
					<key>kSecTrustSettingsResult</key>
					<integer>4</integer>
				</dict>
			</array>
		</dict>

kSecTrustSettingsResult looks to be defined as SecTrustSettingsResult and has the following values:

typedef CF_ENUM(uint32_t, SecTrustSettingsResult) {
     kSecTrustSettingsResultInvalid = 0,   /* Never valid in a Trust Settings array or
                                            * in an API call. */
     kSecTrustSettingsResultTrustRoot,     /* Root cert is explicitly trusted */
     kSecTrustSettingsResultTrustAsRoot,   /* Non-root cert is explicitly trusted */
     kSecTrustSettingsResultDeny,          /* Cert is explicitly distrusted */
     kSecTrustSettingsResultUnspecified    /* Neither trusted nor distrusted; evaluation
                                            * proceeds as usual */
};

kSecTrustSettingsPolicyName looks to be a set of magic strings.

@adamdecaf
Copy link
Contributor Author

@adamdecaf adamdecaf commented Feb 24, 2018

I've got the trust-settings-export parsing down pretty well, but I assume it could be made safer by processing each element one at a time. Here's an example:

https://github.com/adamdecaf/plist-parser/blob/9606801b8fea0e91cf12d7e5c66cf30e937e2867/main.go#L84

I think with this parsing we could get rid of verifyCertWithSystem and change getCertsWithTrustPolicy() to getCertsWithTrustPolicy() (map[string][]*trustPolicy, error). Thoughts?

type trustPolicy struct {
	name string // kSecTrustSettingsPolicyName values (e.g. basicX509, sslServer, CodeSigning)
	result string // kSecTrustSettingsResult
}
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 2, 2018

Change https://golang.org/cl/98336 mentions this issue: crypto/x509: parse darwin trust policies

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 3, 2018

Putting this on @FiloSottile's radar just in case he hasn't yet seen it or might be interested in it :)

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 28, 2018

This seems like the right path, but too late to change in 1.11. It should also probably try to reuse some logic from the cgo path.

@FiloSottile FiloSottile modified the milestones: Go1.11, Go1.12 Jun 28, 2018
@FiloSottile FiloSottile changed the title crypto/x509: Darwin keychain policies are not fully parsed crypto/x509: parse macOS keychain policies instead of using verify-cert Jun 28, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 28, 2018

Change https://golang.org/cl/121356 mentions this issue: crypto/x509: verify ssl keychain policy on noncgo path

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 28, 2018

Change https://golang.org/cl/121357 mentions this issue: crypto/x509: better debug output for verify-cert calls

gopherbot pushed a commit that referenced this issue Jun 28, 2018
Now that pkix.Name offers String() we should use that as some CN's are blank.

Updates #24084

Change-Id: I268196f04b98c2bd4d5d0cf1fecd2c9bafeec0f1
Reviewed-on: https://go-review.googlesource.com/121357
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@adamdecaf
Copy link
Contributor Author

@adamdecaf adamdecaf commented Aug 22, 2018

https://go-review.googlesource.com/c/go/+/128116 checks with a trust policy. Closing. (See #24652 also)

@adamdecaf adamdecaf closed this Aug 22, 2018
@golang golang locked and limited conversation to collaborators Aug 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.