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.

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

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
Closed

crypto/x509: root_cgo_darwin and root_nocgo_darwin omit some system certs [1.11 backport] #26039

gopherbot opened this issue Jun 25, 2018 · 14 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.11.6

Comments

@gopherbot
Copy link

@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.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Jun 25, 2018
@gopherbot gopherbot mentioned this issue Jun 25, 2018
Closed
@gopherbot gopherbot added this to the Go1.10.4 milestone Jun 25, 2018
@andybons andybons modified the milestones: Go1.10.4, Go1.10.5 Aug 24, 2018
@katiehockman katiehockman added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Oct 30, 2018
@katiehockman
Copy link
Contributor

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
Contributor

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
Contributor

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 added CherryPickCandidate Used during the release process for point releases and removed CherryPickApproved Used during the release process for point releases labels Dec 14, 2018
@FiloSottile FiloSottile modified the milestones: Go1.11.4, Go1.11.5 Dec 15, 2018
@Aeon Aeon mentioned this issue Dec 29, 2018
Closed
@katiehockman
Copy link
Contributor

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

@FiloSottile
Copy link
Contributor

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
Contributor

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 commented Feb 5, 2019

Ping

@katiehockman
Copy link
Contributor

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

@FiloSottile
Copy link
Contributor

@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
Contributor

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

@katiehockman katiehockman added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Feb 15, 2019
@gopherbot
Copy link
Author

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

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

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

@gopherbot
Copy link
Author

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

gopherbot pushed a commit that referenced this issue Feb 22, 2019
@FiloSottile
[release-branch.go1.11] crypto/x509: fix root CA extraction on macOS …
688dc85 
…(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
@FiloSottile
[release-branch.go1.11] crypto/x509: fix root CA extraction on macOS …
3705d34 
…(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.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.11.6
Development

No branches or pull requests
7 participants
@andybons @FiloSottile @dmitshur @bcmills @julieqiu @gopherbot @katiehockman