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: wrong result checking signature of CSR with unordered multi-value RDN #49519

Open
defacto64 opened this issue Nov 11, 2021 · 2 comments
Assignees
Labels
NeedsDecision
Milestone

Comments

@defacto64
Copy link

@defacto64 defacto64 commented Nov 11, 2021

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

$ go version
go version go1.17.1 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/adriano/.cache/go-build"
GOENV="/home/adriano/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/adriano/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/adriano/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.17"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.17/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.1"
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-build274131833=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I checked the signature of a CSR (Certificate Signing Request) via the CertificateRequest.CheckSignature() method.

Here is the CSR:
problem-csr-20211109.txt

Link to a runnable program: https://play.golang.org/p/R7hiD6-HmXm

What did you expect to see?

I expected to get a negative result, as the signature on the attached CSR (also embedded in the example program) was not computed according to RFC2986. In particular, the attached CSR contains an unordered multi-value RDN, that is an unordered SET OF, in its Subject field, and this was apparently ignored when the CSR was generated (RFC2986 requires DER-encoding the certificationRequestInfo component prior to signing it). So the signature, although it's cryptographically correct, was computed over the wrong data, and I'd expect a CSR validator to detect that.

A number of other tool(kit)s report the signature as invalid, which IMO is the correct result, including GnuTLS and BouncyCastle.

What did you see instead?

I got a positive result.

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Nov 18, 2021

@dr2chase dr2chase added the NeedsInvestigation label Nov 18, 2021
@FiloSottile FiloSottile added this to the Go1.19 milestone Mar 2, 2022
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Mar 2, 2022

We do not recompute the DER encoding of the CSR (or certificate) when checking the signature, since that would rely on our encoder in order to verify the cryptographic correctness of the signature (which has, in the past, caused issues for other implementations). Instead we simply check the signature against the raw TBS DER encoding that was extracted during parsing. I believe our current behavior here, in regards to the signature, is correct.

That said there is an open question about parsing, if we consider improperly ordered RDNs as malformed (which the RFC suggests is correct), then we should reject the CSR during parsing. With the current CSR parser this isn't possible, but with #50674 this may be viable.

@rolandshoemaker rolandshoemaker added NeedsDecision and removed NeedsInvestigation labels Mar 2, 2022
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

4 participants