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

x/crypto: error parsing even large ASN.1 identifiers #58821

Open
xinfengliu opened this issue Mar 2, 2023 · 13 comments
Open

x/crypto: error parsing even large ASN.1 identifiers #58821

xinfengliu opened this issue Mar 2, 2023 · 13 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@xinfengliu
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.20.1 darwin/arm64

Does this issue reproduce with the latest release?

Yes on go 1.20.1 .
Similar to #49678 but with an even larger oid: https://oidref.com/1.2.36.20151795998
This caused our program failure to parse a customer's certificate. Error message: x509: invalid certificate policies

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/docker/Library/Caches/go-build"
GOENV="/Users/docker/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/docker/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/docker/go"
GOPRIVATE=""
GOPROXY="https://goproxy.cn,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/docker/work/codes/go/src/lxf/asn.1-oid-test/go.mod"
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 -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/25/fnckz31d0jjfqdhs8yb80lmh0000gn/T/go-build2511654897=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://go.dev/play/p/u4NrlO1xGqr

What did you expect to see?

Object Identifier: 1.2.36.20151795998.3.1.1.1
ASN.1 Encoding:    060b2a24cb8990821e03010101
Decode result: true
Object Identifier: 1.2.36.20151795998.3.1.1.1

What did you see instead?

Object Identifier: 1.2.36.20151795998.3.1.1.1
ASN.1 Encoding:    060b2a24cb8990821e03010101
Decode result: false
Object Identifier: 
@gopherbot gopherbot added this to the Unreleased milestone Mar 2, 2023
@dmitshur
Copy link
Contributor

dmitshur commented Mar 2, 2023

CC @golang/security.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 2, 2023
@AlexanderYastrebov
Copy link
Contributor

From https://go-review.googlesource.com/c/crypto/+/365674 done for #49678 it follows that int element can not be greater than 2^31-1.
Interestingly that decoder was "fixed" but not the encoder.

@rolandshoemaker
Copy link
Member

Dupe of #30757, #36467, and #38737. By design asn1.ObjectIdentifier does not support OID components that are larger than 31 bits. Due to backwards compatibility constraints, there is no clear path forward to fix this.

@AlexanderYastrebov
Copy link
Contributor

It should be possible to support larger than 2^31 values on the platforms where sizeof(int) > 32 (https://pkg.go.dev/encoding/asn1#ObjectIdentifier is defined as []int) similar to Marshal that supports values up to maxInt.
The problem is that behavior would be different depending on the int size of the platform, i.e. large values would not work on 32 bit platforms (also not sure if Marshal errors in that case).

@rolandshoemaker
Copy link
Member

Right, we make a conscious choice to limit to the lower of the two possibilities so that there aren't certificates (or other ASN1 based protocols) which only work on 64 bit platforms.

@srebhan
Copy link

srebhan commented Mar 8, 2023

We also run into the same issue with a user trying to parse a certificate from a Windows CA. Do you think it would be possible to add a asn1.ObjectIdentifier64 (or similar) which can handle those larger Oids? Maybe by switching the target certificate field (with the new field-type) explicitly using an "option" for x509.ParseCertificate...

@rolandshoemaker
Copy link
Member

When you say "Windows CA" do you mean a CA managed by Microsoft? Do you happen to know what the offending OID is?

AlexanderYastrebov added a commit to AlexanderYastrebov/asn1oid64 that referenced this issue Mar 9, 2023
AlexanderYastrebov added a commit to AlexanderYastrebov/asn1oid64 that referenced this issue Mar 9, 2023
@AlexanderYastrebov

This comment was marked as off-topic.

AlexanderYastrebov added a commit to AlexanderYastrebov/crypto that referenced this issue Mar 9, 2023
asn1.ObjectIdentifier is a slice of ints and asn1.Marshal can serialize
int values greater than 2^31-1 on 64 bit platforms.

This change updates ReadASN1ObjectIdentifier to support greater int
values on 64 bit platforms.

For golang/go#58821
@AlexanderYastrebov
Copy link
Contributor

Right, we make a conscious choice to limit to the lower of the two possibilities so that there aren't certificates (or other ASN1 based protocols) which only work on 64 bit platforms.

I've created a fix in case this decision changes
golang/crypto@master...AlexanderYastrebov:crypto:go58821

@xinfengliu
Copy link
Author

Thanks @AlexanderYastrebov for the effort.

I hope to reconsider the decision of // We don't want different behavior based on the architecture. (from the code comment) given that there are multiple issue reports like this. If we still keep current 32-bit golang programs' behavior by supporting larger Oid number on 64-bit platform, we don't break anything, we just make 64-bit programs not break in such cases.

@srebhan
Copy link

srebhan commented Mar 10, 2023

@rolandshoemaker I linked the issue we have open in Telegraf influxdata/telegraf#12684. Citing @skinfrakkiirid:

I can't share the certificate due to where I work, however this certificate differs by being issued from a local Windows CA[...]

he also provided the policy section of the cert

X509v3 extensions:
X509v3 Subject Alternative Name:
DNS:dnsserver1, DNS:dnsserver2 IP Address:address1, IP Address:address2
X509v3 Subject Key Identifier:
98:FF:66:3C:58:26:36:A1:F5:CB:C2:B3:BC:3C:33:90:3E:8A:5E:80
X509v3 Authority Key Identifier:
keyid:13:57:08:E7:5E:7E:92:45:E9:AF:CC:80:B2:D5:D2:A4:E2:30:8

@Tomo59
Copy link

Tomo59 commented Jun 6, 2023

I hope to reconsider the decision of // We don't want different behavior based on the architecture. (from the code comment) given that there are multiple issue reports like this. If we still keep current 32-bit golang programs' behavior by supporting larger Oid number on 64-bit platform, we don't break anything, we just make 64-bit programs not break in such cases.

Unfortunately, this solution won't support the 128 bit long 2.25 OID subtree (as mentioned in #30757) so you would end up with a bit more certificates working on 64 bit architecture but not all certificates.

I think this deserves a real solution where all certificates are supported by Go.

It's really sad that a 4 years old issue has still no clear solution forward. Also I don't understand why #39795 has been closed.

@rolandshoemaker
Copy link
Member

#60665 suggests a path forward on this.

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.
Projects
None yet
Development

No branches or pull requests

7 participants