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

Jump to bottom

crypto/x509: wrong result checking signature of CSR with unordered multi-value RDN #49519

Open
defacto64 opened this issue Nov 11, 2021 · 3 comments
Open

crypto/x509: wrong result checking signature of CSR with unordered multi-value RDN #49519

defacto64 opened this issue Nov 11, 2021 · 3 comments
Assignees
@rolandshoemaker
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@defacto64
Copy link

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

@FiloSottile PTAL

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 18, 2021
@FiloSottile FiloSottile added this to the Go1.19 milestone Mar 2, 2022
@rolandshoemaker
Copy link
Member

rolandshoemaker commented Mar 2, 2022 •
edited

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 Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 2, 2022
@ianlancetaylor
Copy link
Contributor

CC @golang/security

Looks like this didn't make 1.19. Moving to backlog. Please recategorize as appropriate.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Backlog Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rolandshoemaker rolandshoemaker

Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Release Status & Release Blockers
Status: No status
Milestone
Backlog
Development

No branches or pull requests
5 participants
@FiloSottile @defacto64 @dr2chase @ianlancetaylor @rolandshoemaker