Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Milestone
Go1.12

Comments

@adamdecaf
Copy link
Contributor

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 commented Feb 23, 2018 •
edited
Loading

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 commented Feb 24, 2018 •
edited
Loading

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
}

@adamdecaf adamdecaf mentioned this issue Feb 24, 2018
Closed
@titanous titanous added this to the Go1.11 milestone Feb 24, 2018
@titanous titanous added OS-Darwin NeedsFix The path to resolution is known, but the work has not been done. labels Feb 25, 2018
@gopherbot
Copy link
Contributor

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

@adamdecaf adamdecaf mentioned this issue Apr 3, 2018
Closed
@odeke-em
Copy link
Member

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
Contributor

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
Contributor

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

@gopherbot
Copy link
Contributor

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
@adamdecaf @bradfitz
crypto/x509: better debug output for verify-cert calls
1a3957b 
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

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

@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
5 participants
@titanous @adamdecaf @FiloSottile @odeke-em @gopherbot