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: potentially anomalous path building results #65085

Open
woodruffw opened this issue Jan 12, 2024 · 19 comments
Open

crypto/x509: potentially anomalous path building results #65085

woodruffw opened this issue Jan 12, 2024 · 19 comments
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Security
Milestone

Comments

@woodruffw
Copy link

Go version

go1.21.5 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/runner/.cache/go-build'
GOENV='/home/runner/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/runner/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/runner/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/hostedtoolcache/go/1.21.5/x64'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/hostedtoolcache/go/1.21.5/x64/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.5'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1019109943=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Hi there! I'm opening this to root-cause a handful of potentially anomalous path building results with crypto/x509, which we found with x509-limbo.

First, a disclaimer: I suspect that many of the results above are expected or are non-issues from crypto/x509's perspective, since many are "pedantic" checks against a strict reading of RFC 5280. For example, rfc5280::nc::not-allowed-in-ee-critical is technically disallowed under RFC 5280, but all other implementations appear to broadly ignore it.

There are, however, some discrepancies for which crypto/x509 is the outlier. A sampling of anomalous positives (chains built where other implementations fail):

There are also some anomalous negatives (chains that fail to build where success is expected), but crypto/x509 is largely in consensus with other implementations around them (the main outlier is OpenSSL).

What did you see happen?

The full rendering of potentially anomalous results is here: https://x509-limbo.com/anomalous-results/gocryptox509-go1.21.5/

As mentioned above, some of these are almost certainly intended behavior on crypto/x509's part as part of handling real-world chains, especially outside of the Web PKI. However, I could find a document or docstring anywhere that explicitly enumerated these (in absolute fairness, nobody does this 🙂), so I figured I would open this up for general review and consideration.

What did you expect to see?

In cases involving critical or out-of-policy extensions that crypto/x509 appears to otherwise ignore, I expected to see chain build failures rather than successes. However, see qualifications above 🙂

@seankhliao seankhliao added Security NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 12, 2024
@seankhliao
Copy link
Member

cc @golang/security

@rolandshoemaker rolandshoemaker self-assigned this Jan 25, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/561615 mentions this issue: crypto/x509: rfc 5280 alignments

@rolandshoemaker
Copy link
Member

@woodruffw it looks like the anomalous findings page has disappeared, are those still recorded somewhere?

@woodruffw
Copy link
Author

@woodruffw it looks like the anomalous findings page has disappeared, are those still recorded somewhere?

My apologies for that -- it looks like our CI hiccuped somewhere and didn't render the latest Go results. I'll look into that now.

@woodruffw
Copy link
Author

Sorry again for that! We've fixed it with C2SP/x509-limbo#193, and the render is here: https://x509-limbo.com/anomalous-results/gocryptox509-go1.21.6/

(NB the URL has changed slightly, since we've gone from go1.21.5 to go1.21.6.)

@rolandshoemaker
Copy link
Member

Perfect, thanks for the (extremely) quick fix!

@rolandshoemaker
Copy link
Member

Ignoring the CABF stuff, did some quick searching for publicly trusted certificates that exhibit the various issues. This is how many certificates I found, listed from least to most, with comments where applicable:


? rfc5280::ee-critical-aia-invalid (seems reasonable to reject)
? rfc5280::nc::permitted-dns-match-noncritical (probably want to do this anyway, mistakingly marking non-crit seems acceptable?)
? rfc5280::pc::ica-noncritical-pc (seems plausible we can reject)
? rfc5280::root-non-critical-basic-constraints (seems unlikely)
0 rfc5280::ca-empty-subject
0 rfc5280::ee-empty-issuer
0 rfc5280::nc::not-allowed-in-ee-critical
0 rfc5280::nc::not-allowed-in-ee-noncritical
0 rfc5280::root-inconsistent-ca-extensions
0 rfc5280::san::noncritical-with-empty-subject
0 rfc5280::ski::critical-ski
3 rfc5280::aki::cross-signed-root-missing-aki
7 rfc5280::leaf-ku-keycertsign
7 rfc5280::serial::too-long (we tried to fix this previously)
8 rfc5280::ski::root-missing-ski (various trusted verisign roots)
9 rfc5280::ski::intermediate-missing-ski(various trusted intermediates)
46 rfc5280::serial::zero (many important roots)
130 rfc5280::aki::intermediate-missing-aki
1,196 rfc5280::aki::leaf-missing-aki

@alex
Copy link
Contributor

alex commented Feb 7, 2024

@rolandshoemaker what's the dataset this is based on, CT?

@rolandshoemaker
Copy link
Member

I used the censys dataset, so mostly CT.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562343 mentions this issue: crypto/x509: reject negative serial numbers

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562341 mentions this issue: crypto/x509: reject critical AKI

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562342 mentions this issue: crypto/x509: reject critical AIA extensions

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562344 mentions this issue: crypto/x509: reject critical SKI extensions

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562975 mentions this issue: crypto/x509: reject serial numbers longer than 20 octets

@woodruffw
Copy link
Author

We've added another testcase to x509-limbo that has a standout anomalous result for crypto/x509: https://x509-limbo.com/testcases/webpki/#webpkisansan-wildcard-only-tld

In particular, when a leaf certificate has a SAN with DNS:*, most implementations reject that cert for any domain, while crypto/x509 appears to reject it for TLDs and other single-label hosts. crypto/x509 should probably be rejecting this instead (like OpenSSL, etc.).

(In terms of standards justification, DNS:* is neither a valid DNS name nor a valid wildcard pattern per RFC 6125, at least according to my reading, since RFC 6125 requires at least one label.)

gopherbot pushed a commit that referenced this issue May 9, 2024
A DNS name prefixed with an empty label should be considered invalid
when checking constraints (i.e. ".example.com" does not satisfy a
constraint of "example.com").

Updates #65085

Change-Id: I42919dc06abedc0e242ff36b2a42b583b14857b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/561615
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue May 9, 2024
Updates #65085

Change-Id: I86d8a85130286e1ec2aca3249808ec1dc8ec97ca
Reviewed-on: https://go-review.googlesource.com/c/go/+/562342
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue May 9, 2024
Updates #65085

Change-Id: I8a00fff6b2af4e55bcb88456813b5ee1f7b1c01d
Reviewed-on: https://go-review.googlesource.com/c/go/+/562344
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue May 9, 2024
Updates #65085

Change-Id: I8cc60990737d582edf4f7f85ec871f5e42f82b78
Reviewed-on: https://go-review.googlesource.com/c/go/+/562341
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
@woodruffw
Copy link
Author

Gentle ping on this finding as well: #65085 (comment) -- I believe this one is also worthy of a patch 🙂

@rolandshoemaker
Copy link
Member

Thanks for the nudge, agreed this should get into the next release.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585076 mentions this issue: crypto/x509: don't match bare wildcard

gopherbot pushed a commit that referenced this issue May 22, 2024
There is only one trusted certificate I could find in the web pki which
has a negative serial number. Removing this exception seems reasonable.

Updates #65085

Change-Id: I55435b3d75479dcb41d523383e4ff7894a1496ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/562343
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
gopherbot pushed a commit that referenced this issue May 22, 2024
Updates #65085

Change-Id: I8e5fb6c77c54f07247b30afea9fe8c548bf6d0be
Reviewed-on: https://go-review.googlesource.com/c/go/+/562975
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
gopherbot pushed a commit that referenced this issue May 22, 2024
When verifying the name "test", a SAN with a bare wildcard ("*") should
not constitute a match.

Updates #65085

Change-Id: I02151761e2f29f3e358708a3f723af32b0d79288
Reviewed-on: https://go-review.googlesource.com/c/go/+/585076
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@seankhliao seankhliao added this to the Unplanned milestone Jul 13, 2024
@woodruffw
Copy link
Author

We've added another x509-limbo case that has an anomalous result in Go, and may be worth addressing: https://x509-limbo.com/testcases/rfc5280/#rfc5280ekuee-eku-empty

In particular, RFC 5280 says that an EKU, if present, has one or more usages listed in it, but crypto/x509 allows an empty EKU to satisfy during path validation (seemingly treating it the same as "no EKU present"). PyCA Cryptography and the various Rust X.509 validators reject this, while OpenSSL doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Security
Projects
None yet
Development

No branches or pull requests

5 participants