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: malformed x509 certificate is accepted since 1.17 #51369

Open
bitmingw opened this issue Feb 26, 2022 · 4 comments
Open

crypto/x509: malformed x509 certificate is accepted since 1.17 #51369

bitmingw opened this issue Feb 26, 2022 · 4 comments
Labels
NeedsDecision

Comments

@bitmingw
Copy link

@bitmingw bitmingw commented Feb 26, 2022

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

$ go version
go version go1.17.7 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/dev/.cache/go-build"
GOENV="/home/dev/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/dev/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/dev/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/dev/Downloads/1.17.7/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/dev/Downloads/1.17.7/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.7"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build678374455=/tmp/go-build -gno-record-gcc-switches"

What did you do?

To demonstrate this I included verify.go and three certificates in the file verify_cert.tar.gz

We have a testing pipeline that checks the handling of x509 certificates. The host.pem included in verify_cert.tar.gz is a malformed certificate. Since it is not compliant to spec, parsing the certificate should result in error. With golang version 1.15.15 and 1.16.14, x509.ParseCertificate returns error, which is expected. However, after upgrade golang version to 1.17.7, this malformed certificate is now accepted.

What did you expect to see?

With go 1.15.15

$ go run verify.go root.pem intermediate.pem host.pem 
panic: failed to parse certificate: x509: cannot parse URI "g\x81\x05\x05\x01\x01": parse "g\x81\x05\x05\x01\x01": net/url: invalid control character in URL

goroutine 1 [running]:
main.main()
	/home/dev/shared/verify.go:47 +0x4f7
exit status 2

With go 1.16.14

$ go run verify.go root.pem intermediate.pem host.pem 
panic: failed to parse certificate: x509: SAN uniformResourceIdentifier is malformed

goroutine 1 [running]:
main.main()
	/home/dev/shared/verify.go:47 +0x4f7
exit status 2

What did you see instead?

With go 1.17.7, no error is returned.

$ go run verify.go root.pem intermediate.pem host.pem 
@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Feb 26, 2022

cc @rolandshoemaker @FiloSottile

@seankhliao seankhliao added the NeedsInvestigation label Feb 27, 2022
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Feb 28, 2022

This is somewhat of a bug in pre-1.17 behavior, but poses a question about what we should do in the post-1.17 parser. The pre-1.17 parser ignores the class of the ASN.1 tag for each name, meaning it accepts tags with the correct context-specific tag, but invalid class bits, such as in this case (from your example the full tag is 0x06, so the class bits are 000, and the tag is 0x06). This means we attempt to parse a random OID as a URI, which is obviously wrong, and throws an error.

In the post-1.17 parser we are more strict, validating that the class bits are correct for the tag, ignoring the strange entries in the SubjectAltName. This is, I believe, the correct behavior.

This does pose a question though, we currently accept entries in the SEQUENCE OF which contain both context-specific tags >8, the highest tag specified in RFC 5280, and invalid class bits. It may be prudent, and catch cases like this, to throw an error when we hit these cases, since they are not spec compliant. Since crypto/x509 explicitly targets the web PKI, it seems acceptable to refuse to accept these types of malformed certificates, which while may be acceptable elsewhere, are invalid according to the rules of RFC 5280.

@rolandshoemaker rolandshoemaker added NeedsDecision and removed NeedsInvestigation labels Feb 28, 2022
@DemiMarie
Copy link

@DemiMarie DemiMarie commented Feb 28, 2022

This does pose a question though, we currently accept entries in the SEQUENCE OF which contain both context-specific tags >8, the highest tag specified in RFC 5280, and invalid class bits. It may be prudent, and catch cases like this, to throw an error when we hit these cases, since they are not spec compliant. Since crypto/x509 explicitly targets the web PKI, it seems acceptable to refuse to accept these types of malformed certificates, which while may be acceptable elsewhere, are invalid according to the rules of RFC 5280.

Is this an invalid class bit, or simply a type of SubjectAltName that Go does not support?

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Feb 28, 2022

Both.

5280 provides a method to extend GeneralNames, using the OtherName (context-specific tag 0), so it seems reasonable only accept the 9 defined context-specific tags (just ignoring the ones we don't support) and fail on undefined, non-OtherName, tags, which are not defined in the profile we are targeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision
Projects
None yet
Development

No branches or pull requests

5 participants