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: CreateRevocationList over-enforces KeyUsage check #59669

Open
cipherboy opened this issue Apr 17, 2023 · 1 comment
Open

crypto/x509: CreateRevocationList over-enforces KeyUsage check #59669

cipherboy opened this issue Apr 17, 2023 · 1 comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@cipherboy
Copy link
Contributor

cipherboy commented Apr 17, 2023

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

$ go version
go version go1.20.3 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/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="github.com/hashicorp"
GONOSUMDB="github.com/hashicorp"
GOOS="linux"
GOPATH="/home/cipherboy/go"
GOPRIVATE="github.com/hashicorp"
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.20.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/cipherboy/GitHub/cipherboy/vault/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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build652088920=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Attempt to use x509.CreateRevocationList(...) with an ITU X.509 compliant certificate lacking the KeyUsage extension.

What did you expect to see?

CRL creation succeed.

What did you see instead?

CRL creation failed due to this check:

go/src/crypto/x509/x509.go

Lines 2299 to 2301 in 0853f8c

if (issuer.KeyUsage & KeyUsageCRLSign) == 0 {
return nil, errors.New("x509: issuer must have the crlSign key usage bit set")
}

Additional Information

While RFC 5280 is unclear about the behavior when the attribute is missing, it is clear that the extension MUST be added:

Conforming CAs MUST include this extension in certificates that
contain public keys that are used to validate digital signatures on
other public key certificates or CRLs. When present, conforming CAs
SHOULD mark this extension as critical.

Per @maxb in Vault#20137, this line about requiring the KeyUsage extension was added in RFC 3280's rewrite of RFC 2459.

However, the broader ITU X.509 standard writes (on printed page 73/pdf page 83):

keyUsage matches if all of the bits set in the presented value are also set in the key usage extension in the stored attribute value, or if there is no key usage extension in the stored attribute value.

meaning the behavior without this extension is quite clear (outside of PKIX space).

Additionally, it is unclear why Go would enforce this on CRL creation time: it seems more usefully to be a client-side check for CRL validity, than a CA generation enforcement mechanism. NSS appears to handle this behavior correctly (thanks @stevendpclark for the reference), but we're having a hard time finding a reference for OpenSSL at the moment.

(There's probably an aside here about few people actually doing verification of CRLs -- most people presume they're transmitted over a pseudo-secure and/or simply fail to verify them altogether. And likely string together whatever pipeline they want to fetch it into their validating software (e.g., web server for mTLS cert auth)... But that's another discussion for another time...) :-)

Proposal

I'd propose eliding this check if the issuer certificates lacks KU altogether, but otherwise persist it. This mirrors the behavior of ITU X.509, minimally violates RFC 5280, and aligns with NSS's behavior.

This would be aligned with earlier precedence.

See also: #11087
See also: #49414

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 17, 2023
@seankhliao
Copy link
Member

cc @golang/security

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

2 participants