Skip to content

crypto/x509: can't verify signature on RSA-PSS certificate requests it created #45990

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

Closed
ycongal-smile opened this issue May 6, 2021 · 19 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ycongal-smile
Copy link
Contributor

ycongal-smile commented May 6, 2021

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

 $ $GODIR/bin/go version
go version devel go1.17-1108cbe60b Thu May 6 02:21:55 2021 +0000 linux/amd64

(Freshly compiled from master)

Does this issue reproduce with the latest release?

Yes

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

go env Output
 $ $GODIR/bin/go env |sed "s,$HOME,\$HOME,"
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="$HOME/.cache/go-build"
GOENV="$HOME/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="$HOME/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="$HOME/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="$HOME/Documents/projets/misc/bug_go_pss/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="$HOME/Documents/projets/misc/bug_go_pss/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.17-1108cbe60b Thu May 6 02:21:55 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="$HOME/Documents/projets/misc/bug_go_pss/go/src/go.mod"
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-build3149370505=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I created a RSA-PSS CertificateRequest and tried to check its signature.

Here is a simple test program : https://play.golang.org/p/TGNgUYvNH5o

It can also be reproduced with the tests from crypto/x509/x509_test.go :

From 6d9c39291cf2d3b6de10b0889d7d1baa72c81d93 Mon Sep 17 00:00:00 2001
From: Yoann Congal <[email protected]>
Date: Thu, 6 May 2021 11:39:29 +0200
Subject: [PATCH] crypto/x509: add test for RSA-PSS CertificateRequest

---
 src/crypto/x509/x509_test.go | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 51dda16815..5314a99cf7 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -1390,6 +1390,7 @@ func TestCreateCertificateRequest(t *testing.T) {
 		sigAlgo SignatureAlgorithm
 	}{
 		{"RSA", testPrivateKey, SHA1WithRSA},
+		{"RSA-256-PSS", testPrivateKey, SHA256WithRSAPSS},
 		{"ECDSA-256", ecdsa256Priv, ECDSAWithSHA1},
 		{"ECDSA-384", ecdsa384Priv, ECDSAWithSHA1},
 		{"ECDSA-521", ecdsa521Priv, ECDSAWithSHA1},
-- 
2.20.1

What did you expect to see?

Program should display "OK" and the test should be OK.

What did you see instead?

Program panicked and test failed : csr.CheckSignature() returned an error instead of nil which would mean a verified signature.

@seankhliao seankhliao changed the title crypto/x509 can't verify signature on RSA-PSS certificate requests it created crypto/x509: can't verify signature on RSA-PSS certificate requests it created May 6, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 6, 2021
@ycongal-smile
Copy link
Contributor Author

ycongal-smile commented May 6, 2021

I've tracked down the check that fail the verification :

// 4. If the rightmost octet of EM does not have hexadecimal value

106 func emsaPSSVerify(mHash, em []byte, emBits, sLen int, hash hash.Hash) error {
.../...
132     // 4.  If the rightmost octet of EM does not have hexadecimal value
133     //     0xbc, output "inconsistent" and stop.
134     if em[emLen-1] != 0xbc {
135         return ErrVerification
136     }

@ycongal-smile
Copy link
Contributor Author

ycongal-smile commented May 6, 2021

I've checked the output of x509.CreateCertificateRequest against OpenSSL : It fails. So I bet that the bug is on the creation side.

 $ openssl req -verify -inform der < go_test.csr.der 
verify failure
139692742198400:error:0407E086:rsa routines:RSA_verify_PKCS1_PSS_mgf1:last octet invalid:../crypto/rsa/rsa_pss.c:88:
139692742198400:error:0D0C5006:asn1 encoding routines:ASN1_item_verify:EVP lib:../crypto/asn1/a_verify.c:170:

EDIT : I've checked that a RSA-PSS CSR created by OpenSSL was correctly verified by csr.CheckSignature().

@ycongal-smile
Copy link
Contributor Author

It seems that rsa.SignPKCS1v15() is called instead of rsa.SignPSS().

Here is the backtrace got by step through crypto/x509.CreateCertificateRequest :

0  0x00000000004f2a72 in crypto/rsa.SignPKCS1v15
   at ./go/src/crypto/rsa/pkcs1v15.go:231
1  0x00000000004f5b46 in crypto/rsa.(*PrivateKey).Sign
   at ./go/src/crypto/rsa/rsa.go:149
2  0x00000000005561d4 in crypto/x509.CreateCertificateRequest
   at ./go/src/crypto/x509/x509.go:2646

ycongal-smile added a commit to ycongal-smile/go that referenced this issue May 6, 2021
ycongal-smile added a commit to ycongal-smile/go that referenced this issue May 6, 2021
In case of a RSA-PSS algorithm, the hashFunc of CreateCertificateRequest
is embedded in a rsa.PSSOptions struct. Given to key.Sign(), this will
generate a proper RSA-PSS signature.

Pasted from the RSA-PSS handling code in CreateCertificate()

Fixes golang#45990
@ycongal-smile
Copy link
Contributor Author

ycongal-smile commented May 6, 2021

I found a fix (#46029). I just need to get the CLA signed (hence the draft status of the PR)

(EDIT Jul 9 2021 Oct 26 2021 Nov 23 2021 Mar 14 2022 Jun 27 2022 : the corporate CLA is still in the process of being signed. This is frustratingly long but we'll do it Jul 07 2022: Finally signed! 🎉)

@networkimprov
Copy link

cc @FiloSottile @katiehockman

@FiloSottile FiloSottile self-assigned this Mar 2, 2022
@FiloSottile FiloSottile added this to the Go1.19 milestone Mar 2, 2022
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Mar 14, 2022
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Mar 14, 2022
In case of a RSA-PSS algorithm, the hashFunc of CreateCertificateRequest
is embedded in a rsa.PSSOptions struct. Given to key.Sign(), this will
generate a proper RSA-PSS signature.

Pasted from the RSA-PSS handling code in CreateCertificate()

Fixes golang#45990
@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
@ycongal-smile
Copy link
Contributor Author

Hi! The CLA is now signed for the linked PR (#46029) but I don't know how to convince the bot to rescan the PR. Maybe someone on this issue can help?

ycongal-smile added a commit to ycongal-smile/go that referenced this issue Jul 7, 2022
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Jul 7, 2022
In case of a RSA-PSS algorithm, the hashFunc of CreateCertificateRequest
is embedded in a rsa.PSSOptions struct. Given to key.Sign(), this will
generate a proper RSA-PSS signature.

Pasted from the RSA-PSS handling code in CreateCertificate()

Fixes golang#45990
@ycongal-smile
Copy link
Contributor Author

... I just had to push new commits. All is ready to be reviewed/merged now.

cipherboy pushed a commit to cipherboy/go that referenced this issue Sep 16, 2022
cipherboy pushed a commit to cipherboy/go that referenced this issue Sep 16, 2022
In case of a RSA-PSS algorithm, the hashFunc of CreateCertificateRequest
is embedded in a rsa.PSSOptions struct. Given to key.Sign(), this will
generate a proper RSA-PSS signature.

Pasted from the RSA-PSS handling code in CreateCertificate()

Fixes golang#45990
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Sep 19, 2022
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Sep 19, 2022
In case of a RSA-PSS algorithm, the hashFunc of CreateCertificateRequest
is embedded in a rsa.PSSOptions struct. Given to key.Sign(), this will
generate a proper RSA-PSS signature.

Pasted from the RSA-PSS handling code in CreateCertificate()

Fixes golang#45990
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/431875 mentions this issue: crypto/x509: fix certificate request creation with RSA-PSS

stevendpclark added a commit to hashicorp/vault that referenced this issue Oct 3, 2022
…e api

 - Mainly to work properly with GCP backed managed keys, we need to
   issue signatures that would match the GCP key algorithm.
 - At this time due to golang/go#45990 we
   can't issue PSS signed CSRs, as the libraries in Go always request
   a PKCS1v15.
 - Add an extra check in intermediate/generate that validates the CSR's
   signature before providing it back to the client in case we generated
   a bad signature such as if an end-user used a GCP backed managed key
   with a RSA PSS algorithm.
   - GCP ignores the requested signature type and always signs with the
     key's algorithm which can lead to a CSR that says it is signed with
     a PKCS1v15 algorithm but is actually a RSA PSS signature
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Nov 6, 2023
In case of a RSA-PSS algorithm, the hashFunc of CreateCertificateRequest
is embedded in a rsa.PSSOptions struct. Given to key.Sign(), this will
generate a proper RSA-PSS signature.

Pasted from the RSA-PSS handling code in CreateCertificate()

Fixes golang#45990
@phlipse
Copy link

phlipse commented Nov 20, 2023

Any news on that topic? https://go-review.googlesource.com/c/go/+/431916 fixes the issue but seems to be forgotten.

@ycongal-smile
Copy link
Contributor Author

ycongal-smile commented Nov 20, 2023

Any news on that topic? https://go-review.googlesource.com/c/go/+/431916 fixes the issue but seems to be forgotten.

@phlipse I'm sorry but I don't have any :(

I've kept things updated here, at https://go-review.googlesource.com/c/go/+/431916 (as you found) and #55153. I've tried to ping but without success. I'm now stuck because I don't know who to ping and how... (If anyone reading this, knows, please reply!)

@phlipse
Copy link

phlipse commented Nov 20, 2023

@FiloSottile are you still the correct / latest assignee for this issue and can you please suggest how to proceed? Or is this information outdated?

ycongal-smile added a commit to ycongal-smile/go that referenced this issue Dec 4, 2023
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Dec 4, 2023
In case of a RSA-PSS algorithm, the hashFunc of CreateCertificateRequest
is embedded in a rsa.PSSOptions struct. Given to key.Sign(), this will
generate a proper RSA-PSS signature.

Pasted from the RSA-PSS handling code in CreateCertificate()

Fixes golang#45990
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Jan 2, 2024
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Jan 2, 2024
In case of a RSA-PSS algorithm, the hashFunc of CreateCertificateRequest
is embedded in a rsa.PSSOptions struct. Given to key.Sign(), this will
generate a proper RSA-PSS signature.

Pasted from the RSA-PSS handling code in CreateCertificate()

Fixes golang#45990
@ycongal-smile
Copy link
Contributor Author

@FiloSottile are you still the correct / latest assignee for this issue and can you please suggest how to proceed? Or is this information outdated?

gentle ping

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/555595 mentions this issue: crypto/x509: provide crypto.SignerOpts to Sign in case of PSS signature

@ycongal-smile
Copy link
Contributor Author

Issue duplicated by #65074

ycongal-smile added a commit to ycongal-smile/go that referenced this issue Feb 5, 2024
ycongal-smile added a commit to ycongal-smile/go that referenced this issue Feb 5, 2024
In case of a RSA-PSS algorithm, the hashFunc of CreateCertificateRequest
is embedded in a rsa.PSSOptions struct. Given to key.Sign(), this will
generate a proper RSA-PSS signature.

Pasted from the RSA-PSS handling code in CreateCertificate()

Fixes golang#45990
@phlipse
Copy link

phlipse commented Apr 18, 2024

The new issue seemed to have contracted more attention then the old one.

@ycongal-smile
Copy link
Contributor Author

The new issue seemed to have contracted more attention then the old one.

I've updated my PR description to tell its fixes both issues.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/431916 mentions this issue: crypto/x509: fix certificate request creation with RSA-PSS

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/586015 mentions this issue: crypto/x509: cleanup signature generation

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 16, 2024
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 May 16, 2024
@dmitshur dmitshur moved this to In Progress in Release Blockers May 16, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Release Blockers May 16, 2024
gopherbot pushed a commit that referenced this issue May 22, 2024
Centralizing some repetitive code, which would have prevented #45990.

This also fixes the deprecated Certificate.CreateCRL for RSA-PSS, not
that anyone cared, probably.

This has two other minor observable behavior changes: MD2 is now treated
as a completely unknown algorithm (why did we even have that!? removing
lets us treat hash == 0 as always meaning no prehash); and we now do the
signature verification self-check for all signing operations.

Change-Id: I3b34fe0c3b6eb6181d2145b0704834225cd45a27
Reviewed-on: https://go-review.googlesource.com/c/go/+/586015
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment