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: Incorrect TBSCertificateList.Issuer field when using non-pkix.Name-encodable Issuer #53754

Open
cipherboy opened this issue Jul 8, 2022 · 4 comments · May be fixed by #53985
Open
Labels
NeedsInvestigation
Milestone

Comments

@cipherboy
Copy link

@cipherboy cipherboy commented Jul 8, 2022

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

$ go version
go version go1.18.2 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

Not relevant.

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cipherboy/.cache/go-build"
GOENV="/home/cipherboy/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/cipherboy/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cipherboy/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/cipherboy/GitHub/golang/go/src/go.mod"
GOWORK=""
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-build2750385870=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Go has historically had issues round-tripping certificates with its pkix.Name field. This has lead to the introduction of RawSubject/RawIssuer on x509.Certificate (see #40876 and others). This is correctly handled during signing, e.g., here.

In #50674, better support was added to the CRL for the raw Issuer field, but both x509.CreateRevocationList(...) and x509.Certificate.CreateCRL(...) still re-use the (incorrectly) parsed pkix.Name over the RawSubject form.

See also related Hashicorp Vault issues:

For example:

openssl req -x509 -newkey rsa -keyout key.pem -out cert.pem -days 365 -nodes -subj '/C=US/ST=California/L=San Francisco/O=Internet Widgets, Inc./OU=WWW/CN=Root/emailAddress=admin@example.com' -sha256 -addext basicConstraints=CA:TRUE -addext "keyUsage = digitalSignature, keyEncipherment, dataEncipherment, cRLSign, keyCertSign"

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

and

openssl req -x509 -newkey rsa -keyout key.pem -out cert.pem -days 365 -nodes -subj '/C=US/ST=California/L=San Francisco/O=Internet Widgets, Inc./OU=WWW/CN=Root/emailAddress=admin@example.com' -sha256 -addext basicConstraints=CA:TRUE -addext "keyUsage = digitalSignature, keyEncipherment, dataEncipherment, cRLSign, keyCertSign" -utf8

https://go.dev/play/p/0nMCWyMrXs6

What did you expect to see?

Subject of the issuer should be correctly preserved in the CRL's Issuer field.

What did you see instead?

The Email address was incorrectly elided from the CRL's Issuer field and the types were converted from UTF8 to PrintableString in the second example.


I was originally thinking it would be sufficient to round-trip from issuer.RawSubject into a pkix.RDNSequence, but this doesn't solve the UTF8 issue:

    x509_test.go:2705: Expected issuer.RawSubject, parsedCRL.RawIssuer to be the same; wanted: 30819a310b30090603550406130255533113301106035504080c0a43616c69666f726e69613116301406035504070c0d53616e204672616e636973636f311f301d060355040a0c16496e7465726e657420576964676574732c20496e632e310c300a060355040b0c03575757310d300b06035504030c04526f6f743120301e06092a864886f70d010901161161646d696e406578616d706c652e636f6d, got: 30819a310b3009060355040613025553311330110603550408130a43616c69666f726e6961311630140603550407130d53616e204672616e636973636f311f301d060355040a1316496e7465726e657420576964676574732c20496e632e310c300a060355040b1303575757310d300b06035504031304526f6f743120301e06092a864886f70d0109010c1161646d696e406578616d706c652e636f6d

Notably, some implementations like OpenSSL will treat the two as unequal still due to different type encodings.

Additionally, other implementations like OpenJDK will fail to consider this CRL for checking revocation status. I believe .NET and Microsoft's AIA-based verification behaves similarly, but fails closed rather than open.

I think this means we'd have to rework away from using pkix.TBSCertificateList to another type similar to tbsCertificate in order to use asn1.RawValue? Let me know if that's the right approach and I can file a PR for that.

@heschi
Copy link
Contributor

@heschi heschi commented Jul 8, 2022

cc @golang/security

@heschi heschi added the NeedsInvestigation label Jul 8, 2022
@heschi heschi added this to the Go1.20 milestone Jul 8, 2022
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Jul 8, 2022

Yup, because pkix.TBSCertificateList cannot be changed we'll need to transition to using an internal (private) type that just uses an asn1.RawValue and do the same trick that CreateCertificate does to pass through a raw issuer (and probably deprecate pkix.TBSCertificateList.) x509.Certificate.CreateCRL shouldn't be updated, as it is deprecated.

Feel free to send a CL if you'd like, but since we are currently in the release freeze we won't be able to land it until the tree re-opens in August.

cipherboy added a commit to cipherboy/go that referenced this issue Jul 21, 2022
Per discussion with Roland Shoemaker, this updates
x509.CreateRevocationList to mirror the behavior of
x509.CreateCertificate, creating an internal struct for the ASN.1
encoding of the CRL. This allows us to switch the Issuer field type to
asn1.RawValue, bypassing the round-tripping issues of pkix.Name in most
scenarios.

Per linked ticket, this resolves issues where a non-Go created
certificate can be used to create CRLs which aren't correctly attested
to that certificate. In rare cases where the CRL issuer is validated
against the certificate's issuer (such as the linked JDK example), this
can result in failing to check this CRL for the candidate certificate.

Resolves: golang#53754

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy added a commit to cipherboy/go that referenced this issue Jul 21, 2022
Per discussion with Roland Shoemaker, this updates
x509.CreateRevocationList to mirror the behavior of
x509.CreateCertificate, creating an internal struct for the ASN.1
encoding of the CRL. This allows us to switch the Issuer field type to
asn1.RawValue, bypassing the round-tripping issues of pkix.Name in most
scenarios.

Per linked ticket, this resolves issues where a non-Go created
certificate can be used to create CRLs which aren't correctly attested
to that certificate. In rare cases where the CRL issuer is validated
against the certificate's issuer (such as the linked JDK example), this
can result in failing to check this CRL for the candidate certificate.

Resolves: golang#53754

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy added a commit to cipherboy/go that referenced this issue Jul 21, 2022
Per discussion with Roland Shoemaker, this updates
x509.CreateRevocationList to mirror the behavior of
x509.CreateCertificate, creating an internal struct for the ASN.1
encoding of the CRL. This allows us to switch the Issuer field type to
asn1.RawValue, bypassing the round-tripping issues of pkix.Name in most
scenarios.

Per linked ticket, this resolves issues where a non-Go created
certificate can be used to create CRLs which aren't correctly attested
to that certificate. In rare cases where the CRL issuer is validated
against the certificate's issuer (such as the linked JDK example), this
can result in failing to check this CRL for the candidate certificate.

Fixes golang#53754

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy added a commit to cipherboy/go that referenced this issue Jul 21, 2022
Per discussion with Roland Shoemaker, this updates
x509.CreateRevocationList to mirror the behavior of
x509.CreateCertificate, creating an internal struct for the ASN.1
encoding of the CRL. This allows us to switch the Issuer field type to
asn1.RawValue, bypassing the round-tripping issues of pkix.Name in most
scenarios.

Per linked ticket, this resolves issues where a non-Go created
certificate can be used to create CRLs which aren't correctly attested
to that certificate. In rare cases where the CRL issuer is validated
against the certificate's issuer (such as the linked JDK example), this
can result in failing to check this CRL for the candidate certificate.

Fixes golang#53754

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy added a commit to cipherboy/go that referenced this issue Jul 21, 2022
Per discussion with Roland Shoemaker, this updates
x509.CreateRevocationList to mirror the behavior of
x509.CreateCertificate, creating an internal struct for the ASN.1
encoding of the CRL. This allows us to switch the Issuer field type to
asn1.RawValue, bypassing the round-tripping issues of pkix.Name in most
scenarios.

Per linked ticket, this resolves issues where a non-Go created
certificate can be used to create CRLs which aren't correctly attested
to that certificate. In rare cases where the CRL issuer is validated
against the certificate's issuer (such as the linked JDK example), this
can result in failing to check this CRL for the candidate certificate.

Fixes golang#53754
@cipherboy cipherboy linked a pull request Jul 21, 2022 that will close this issue
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 21, 2022

Change https://go.dev/cl/418834 mentions this issue: crypto/x509: create CRLs with Issuer.RawSubject

@cipherboy
Copy link
Author

@cipherboy cipherboy commented Aug 5, 2022

Hey @rolandshoemaker! Congrats on the 1.19 GA :-) Any chance I could get some eyes on the patch set? Thanks!

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

Successfully merging a pull request may close this issue.

4 participants