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: properly handle errSecInvalidTrustSettings in macOS roots #38888

Closed
FiloSottile opened this issue May 5, 2020 · 14 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Darwin
Milestone

Comments

@FiloSottile
Copy link
Contributor

Looks like when a trust setting is invalid it drops trust in the certificate, overriding wider domains. All of this is documented nowhere else than in the macOS sources, so it will be a pain to figure out.

This came up in a report by @henvic while testing CL 227037. The tests still passed because the cgo and the direct call implementations behave the same, but they used to disagree with the exec one, which for once looks like it might have been the correct one.

https://gist.github.com/henvic/ab28a19631d18135ade7f9507c67feda
https://gist.github.com/henvic/68d9d64bd0120cb74464c5df53c692c0

Not fixing it in CL 227037 and deferring to Go 1.16 because it's been like this forever, and I'd like to focus on getting the port from cgo right, without changing behaviors at the same time.

@FiloSottile FiloSottile added OS-Darwin NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 5, 2020
@FiloSottile FiloSottile added this to the Go1.16 milestone May 5, 2020
@FiloSottile
Copy link
Contributor Author

Likewise, we should figure out what to do when there are both user policies and admin policies, but none of the user policy dictionaries apply.

	// If trust settings are present, but none of them match the policy...
	// the docs don't tell us what to do.
	//
	// "Trust settings for a given use apply if any of the dictionaries in the
	// certificate’s trust settings array satisfies the specified use." suggests
	// that it's as if there were no trust settings at all, so we should maybe
	// fallback to the admin trust settings? TODO.

@rolandshoemaker
Copy link
Member

rolandshoemaker commented May 9, 2020

@henvic out of interest, how did you distrust these certificates on your system? When I use Keychain Access to manually de-trust a root (setting "When using this certificate: Never Trust") my /Library/Security/Trust\ Settings/admin.plist looks rather different. Noticeably I see the kSecTrustSettingsPolicyName field in most of my trustSettings dicts, among a few other minor differences. If I import any of your trust settings I get similar "The Trust Settings Record was corrupted." errors, which may suggest this particular problem is a red herring due to broken trust settings.

That said there does seem to still be a mismatch between the behavior in the no-cgo and cgo implementations here. When the no-cgo implementation hits this weird parsing issue it returns errSecInvalidTrustSettings, whereas the cgo version is returning kSecTrustSettingsResultUnspecified.

@rolandshoemaker
Copy link
Member

Ah, I see what is happening here, cgo sslTrustSettingsResult is returning kSecTrustSettingsResultUnspecified on err != errSecSuccess || trustSettings == NULL from SecTrustSettingsCopyTrustSettings whereas no-cgo is directly returning the actual error. The cgo approach seems incorrect, I'd expect it to return kSecTrustSettingsResultInvalid perhaps? or... the error, ¯_(ツ)_/¯. Need to look at the apple source.

@rolandshoemaker
Copy link
Member

rolandshoemaker commented May 9, 2020

I've done some spelunking in the apple sources with some help from @sleevi. Various comments on the comments in the CGO version (which have mostly been copied into the direct call version):

// sslTrustSettingsResult obtains the final kSecTrustSettingsResult value
// for a certificate in the user or admin domain, combining usage constraints
// for the SSL SecTrustSettingsPolicy, ignoring SecTrustSettingsKeyUsage and
// kSecTrustSettingsAllowedError.
// https://developer.apple.com/documentation/security/1400261-sectrustsettingscopytrustsetting

Ignoring kSecTrustSettingsAllowedError seems somewhat dangerous. This dict key implies that there are certain errors that appear during chain validation that should be ignored, but the cert should not be wholesale trusted. Since golang doesn't understand how to apply this policy during chain building it seems prudent to skip any certs that have this in their SSL usage constraints dict. Relatedly it seems like kSecTrustSettingsKeyUsage could be applied without much pain, i.e. if we are adding certs to the root pool, and the kSecTrustSettingsResult is Trust or TrustAsRoot, it might make sense to check that the SignCert bit is set.

// According to Apple's SecTrustServer.c, "user trust settings overrule admin trust settings",
// but the rules of the override are unclear. Let's assume admin trust settings are applicable
// if and only if user trust settings fail to load or are NULL.

SecTrustServer.c is from old-macOS, the current policy is described in https://opensource.apple.com/source/Security/Security-59306.41.2/trust/headers/SecTrustSettings.h.auto.html and this specific quote appears to be from SecLegacyAnchorSourceCopyUsageConstraints in https://opensource.apple.com/source/Security/Security-59306.41.2/trust/trustd/SecCertificateSource.c.auto.html. The source matches the interpretation in the comment, if there are user settings they override admin settings.

There is a mismatch here between the CGO (and apple) implementations and the direct call implementation. Errors when retrieving the user or admin trust settings appear to just be thrown away, and a NULL CFArrayRef is returned (causing kSecTrustSettingsResultUnspecified for the certificate in the cgo impl and kSecTrustSettingsResultInvalid in the apple impl) rather than the error being surfaced. It seems like the direct call implementation should match this (in practice because kSecTrustSettingsResultUnspecified isn't actually implemented in the go version, because this is a root pool, not an intermediate pool, it has the same effect in the end).

// > no trust settings [...] means "this certificate must be verified to a known trusted certificate”
// (Should this cause a fallback from user to admin domain? It's unclear.)

This comment appears to be in the wrong place, the fallback to admin domain has already happened if the user settings are NULL, if there are neither user nor admin trust settings kSecTrustSettingsResultInvalid appears to be appropriate.

// If multiple dictionaries match, we are supposed to "OR" them,
// the semantics of which are not clear. Since TrustRoot and TrustAsRoot
// are mutually exclusive, Deny should probably override, and Invalid and
// Unspecified be overridden, approximate this by stopping at the first
// TrustRoot, TrustAsRoot or Deny.

It's hard to pin down how apple treats this, but it appears to just return the first constraint that applies (see SecPVCGetTrustSettingsResult in https://opensource.apple.com/source/Security/Security-59306.41.2/trust/trustd/SecPolicyServer.c.auto.html). My understanding is that while there are cases where there may be multiple dicts in the CFArrayRef where kSecTrustSettingsPolicy == SSL, those dicts are likely to have extra policy attached to them (i.e. kSecTrustSettingsAllowedError or kSecTrustSettingsKeyUsage or something completely different). That said it might be prudent for kSecTrustSettingsResultDeny to override the other results, since we are not sure how to interpret the extra policy.

A more high level comment: probably we should be ignoring dicts that contain keys we don't parse/don't know how to parse, since they are likely to contain policy we cannot apply in go.

// If trust settings are present, but none of them match the policy...
// the docs don't tell us what to do.
//
// "Trust settings for a given use apply if any of the dictionaries in the
// certificate’s trust settings array satisfies the specified use." suggests
// that it's as if there were no trust settings at all, so we should probably
// fallback to the admin trust settings. TODO.

Referring back, trust settings are attached to the cert independent of it's evaluation, so there doesn't appear to be any fallback from one domain to another if no settings apply to the cert. Apple source seems to imply that in the case that no trust settings apply to a cert then kSecTrustSettingsResultInvalid is returned rather than kSecTrustSettingsResultUnspecified (see SecPVCGetTrustSettingsResult again in https://opensource.apple.com/source/Security/Security-59306.41.2/trust/trustd/SecPolicyServer.c.auto.html) but again since we don't actually do anything with certs marked kSecTrustSettingsResultUnspecified it's functionally the same (for now).

Generally I think both implementations are mostly matching the apple approach, although in a few places the golang implementation feels a bit more liberal than it perhaps should be.

@rolandshoemaker
Copy link
Member

Sidenote: there are a handful of times in the comments that references are made to places in the apple source, but without any links (or even file names) it's very hard to track down what they are talking about. It would be extremely valuable to add explicit references to what is being talked about.

@henvic
Copy link
Contributor

henvic commented May 9, 2020

@rolandshoemaker, I distrusted them in Keychain probably the same way you did a long time ago after opening it as a regular (non-sudoer) user (at some point I was prompted for authentication with an admin account).

@rolandshoemaker
Copy link
Member

rolandshoemaker commented May 9, 2020

Hrm, interesting. I wonder if this is a case of Apple pulling the rug out from beneath themselves by changing the schema at some point and not properly migrating it or something... ¯\_(ツ)_/¯ Thanks for the info!

@rolandshoemaker
Copy link
Member

@henvic you might want to try to re-distrust those specific certs if you really don't want them used, it's possible apple is sticking them in the intermediate pool for chain validation, which could cause very strange chains using them to be built...

@gopherbot
Copy link

Change https://golang.org/cl/233360 mentions this issue: crypto/x509: match darwin cgo loadSystemRoots in new implementation

gopherbot pushed a commit that referenced this issue Jun 5, 2020
… roots

This change makes the direct call darwin loadSystemRoots implementation
match the existing cgo implementation, which in turn _mostly_ matches
the Apple implementation. The main change here is that when
SecTrustSettingsCopyTrustSettings the error is ignored, and can either
cause a fallback to check admin trust settings, or cause the
certificate to be marked kSecTrustSettingsResultUnspecified.

As well as updating the implementation to match the cgo one, this
change also updates the documentation of how the fallbacks work and
how they match the Apple implementations. References are made to the
Apple source where appropriate. This change does not update the
existing comments in the cgo implementation, since the goal is to
delete that code once the direct call implementation is matured.

Updates #38888

Change-Id: Id0344ea9d2eede3b715f341e9cbd3c1c661b7a90
Reviewed-on: https://go-review.googlesource.com/c/go/+/233360
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
@odeke-em
Copy link
Member

Kindly paging @katiehockman @rolandshoemaker @FiloSottile, about this issue. Should we perhaps move this to Go1.17?

@rolandshoemaker
Copy link
Member

Yep, definitely not happening for 1.16, thanks!

@odeke-em
Copy link
Member

odeke-em commented Feb 1, 2021

Thanks @rolandshoemaker for the update! Moving it to Go1.17.

@FiloSottile
Copy link
Contributor Author

Moving to backlog as it will probably be cleared by #46287.

@FiloSottile FiloSottile modified the milestones: Go1.17, Backlog Jun 17, 2021
@gopherbot
Copy link

Change https://golang.org/cl/353132 mentions this issue: crypto/x509: use platform verifier on darwin

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Darwin
Projects
None yet
Development

No branches or pull requests

5 participants