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: root_cgo_darwin and root_nocgo_darwin omit some system certs [1.11 backport] #26039

Closed
gopherbot opened this issue Jun 25, 2018 · 14 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 25, 2018

@FiloSottile requested issue #24652 to be considered for backport to the next 1.10 minor release.

There are multiple issues with our macOS root discovery.

The cgo path is unaware of defaults, documented at https://developer.apple.com/documentation/security/1400261-sectrustsettingscopytrustsetting, so it will omit the following certificate.

Cert 1: mkcert development CA
   Number of trust settings : 0

CL 104735 is an incomplete fix, because if trustSettings are present but don't have a kSecTrustSettingsResult value, it defaults to trustRoot. So it will omit the following certificate.

Cert 1: mkcert development CA
   Number of trust settings : 1
   Trust Setting 0:
      Policy OID            : SSL

The nocgo path, on the other hand, asks security verify-cert to use the default verification policy, basic, so it will omit the following certificate.

Cert 1: mkcert development CA
   Number of trust settings : 1
   Trust Setting 0:
      Policy OID            : SSL
      Result Type           : kSecTrustSettingsResultTrustRoot

Finally, the cgo path is checking if any policy (ssl or any other explicitly set) has a kSecTrustSettingsResult value (ignoring the defaults, see above), with the last one in the array winning, omitting the following certificate (!!).

Cert 1: mkcert development CA
   Number of trust settings : 2
   Trust Setting 0:
      Policy OID            : SSL
      Result Type           : kSecTrustSettingsResultTrustRoot
   Trust Setting 1:
      Policy OID            : Code Signing
      Result Type           : kSecTrustSettingsResultDeny

And I didn't even get into allowed errors.

It's fairly late in the freeze, but I'm inclined to fix these, and maybe even backport them, because ignoring the policy types can lead to inclusion of roots that are not supposed to be trusted for TLS, and although crypto/x509 is not TLS-specific, it is meant to serve the WebPKI. @agl agree?

@gopherbot please open the backport tracking issues.

@katiehockman
Copy link
Member

@katiehockman katiehockman commented Oct 30, 2018

Approved since this is a security patch. Please follow the instructions at https://github.com/golang/go/wiki/MinorReleases to create the cherrypick CL.

I will defer to @FiloSottile as to which CLs should be backported.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Nov 2, 2018

Moving to Go 1.10.6 milestone as this isn't ready for 1.10.5.

@dmitshur dmitshur modified the milestones: Go1.10.5, Go1.10.6 Nov 2, 2018
@dmitshur dmitshur modified the milestones: Go1.10.6, Go1.10.7 Dec 13, 2018
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Dec 14, 2018

I want this to get tested in beta before backporting it, so I expect it will not reach 1.10, but only 1.11 later on.

@FiloSottile FiloSottile changed the title crypto/x509: root_cgo_darwin and root_nocgo_darwin omit some system certs [1.10 backport] crypto/x509: root_cgo_darwin and root_nocgo_darwin omit some system certs [1.11 backport] Dec 14, 2018
@FiloSottile FiloSottile modified the milestones: Go1.10.7, Go1.11.4 Dec 14, 2018
@FiloSottile FiloSottile modified the milestones: Go1.11.4, Go1.11.5 Dec 15, 2018
@katiehockman
Copy link
Member

@katiehockman katiehockman commented Jan 4, 2019

@FiloSottile any updates on beta testing? Should this still be be backported?

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jan 8, 2019

I'd like this to bake maybe another week before backporting, also because I'll get to talk with Apple engineers at RWC.

@julieqiu julieqiu modified the milestones: Go1.11.5, Go1.11.6 Jan 23, 2019
@bcmills
Copy link
Member

@bcmills bcmills commented Jan 30, 2019

I'd like this to bake maybe another week before backporting,

@FiloSottile, any new information (from the beta or from Apple) to inform the backport decision?

@andybons
Copy link
Member

@andybons andybons commented Feb 5, 2019

Ping

@katiehockman
Copy link
Member

@katiehockman katiehockman commented Feb 15, 2019

@FiloSottile ping. It has definitely been a week of bake-in time for this. Want this cherrypicked into 1.11 now?

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Feb 15, 2019

@katiehockman yes, I feel good enough about this now. I'm thinking we don't need it in 1.10 after all though. I'll close the 1.10 issue and make the 1.11 cherry-pick.

@katiehockman
Copy link
Member

@katiehockman katiehockman commented Feb 15, 2019

Thanks! I'll change the status to approved then.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Feb 15, 2019

Change https://golang.org/cl/162860 mentions this issue: [release-branch.go1.11] crypto/x509: fix root CA extraction on macOS (cgo path)

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Feb 15, 2019

Change https://golang.org/cl/162861 mentions this issue: [release-branch.go1.11] crypto/x509: fix root CA extraction on macOS (no-cgo path)

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Feb 22, 2019

Closed by merging 688dc85 to release-branch.go1.11.

@gopherbot gopherbot closed this Feb 22, 2019
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Feb 22, 2019

Closed by merging 3705d34 to release-branch.go1.11.

gopherbot pushed a commit that referenced this issue Feb 22, 2019
…(cgo path)

The cgo path was not taking policies into account, using the last
security setting in the array whatever it was. Also, it was not aware of
the defaults for empty security settings, and for security settings
without a result type. Finally, certificates restricted to a hostname
were considered roots.

The API docs for this code are partial and not very clear, so this is a
best effort, really.

Updates #24652
Updates #26039

Change-Id: I8fa2fe4706f44f3d963b32e0615d149e997b537d
Reviewed-on: https://go-review.googlesource.com/c/128056
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@golang.org>
(cherry picked from commit f6be1cf)
Reviewed-on: https://go-review.googlesource.com/c/162860
Reviewed-by: Andrew Bonventre <andybons@golang.org>
gopherbot pushed a commit that referenced this issue Feb 22, 2019
…(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
Fixes #26039

Change-Id: Ife8b3d2f4026daa1223aa81fac44aeeb4f96528a
Reviewed-on: https://go-review.googlesource.com/c/128116
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@golang.org>
(cherry picked from commit aa24158)
Reviewed-on: https://go-review.googlesource.com/c/162861
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@golang golang locked and limited conversation to collaborators Feb 22, 2020
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
7 participants
You can’t perform that action at this time.