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

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

crypto/x509: potentially anomalous path building results #65085

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

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 🙂
@woodruffw woodruffw mentioned this issue Jan 12, 2024
Open
4 tasks
@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

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 woodruffw mentioned this issue Feb 5, 2024
Merged
@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

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@woodruffw woodruffw mentioned this issue Mar 12, 2024
Open
@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
@rolandshoemaker
crypto/x509: properly reject invalid DNS names when checking constraints
5856162 
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
@rolandshoemaker
crypto/x509: reject critical AIA extensions
f4bb7b9 
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
@rolandshoemaker
crypto/x509: reject critical SKI extensions
e0aab32 
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
@rolandshoemaker @gopherbot
crypto/x509: reject critical AKI
671696a 
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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rolandshoemaker rolandshoemaker

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Security
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@alex @woodruffw @rolandshoemaker @gopherbot @seankhliao