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/rsa: reject short signatures #21896

Closed
rsc opened this issue Sep 14, 2017 · 6 comments
Closed

crypto/rsa: reject short signatures #21896

rsc opened this issue Sep 14, 2017 · 6 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@rsc
Copy link
Contributor

rsc commented Sep 14, 2017

During dev.boringcrypto work, I discovered that x/crypto/openpgp tests depend on crypto/rsa accepting trimmed RSA signatures, in which leading zeros have been removed, while BoringSSL does not. @agl says that the Go library is in error and that such signatures are not valid. We should fix this (in master, not just dev.boringcrypto) for Go 1.10 and mark it in the release notes. Will need to fix openpgp first.
@rsc rsc added this to the Go1.10 milestone Sep 14, 2017
@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. Security labels Nov 16, 2017
@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@FiloSottile FiloSottile added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jul 16, 2018
@FiloSottile FiloSottile modified the milestones: Go1.11, Go1.12 Jul 16, 2018
@FiloSottile
Copy link
Contributor

This is very likely to break people, and unlikely to be a security issue (unless someone is relying on verifiable signatures not being malleable, but that is often regrettable anyway) so let's do it in early 1.12 instead of late 1.11.

@andybons andybons modified the milestones: Go1.12, Go1.13 Nov 28, 2018
@bradfitz bradfitz modified the milestones: Go1.13, Go1.14 May 16, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
rolandshoemaker pushed a commit to rolandshoemaker/go that referenced this issue Mar 29, 2020
crypto/rsa: reject invalid length PKCS#1v1.5 signatures
dea15e6 
Per RFC 8017, reject signatures which are not the same length as the RSA
modulus. This matches the behavior of SignPKCS1v15 which properly left pads the
signatures it generates to the size of the modulus.

Fixes golang#21896

Change-Id: I78cf5b225975263fe60aa3acdb458bd4d9cd8de0
rolandshoemaker added a commit to rolandshoemaker/go that referenced this issue Mar 29, 2020
@rolandshoemaker
crypto/rsa: reject invalid length PKCS#1v1.5 signatures
6040f79 
Per RFC 8017, reject signatures which are not the same length as the RSA
modulus. This matches the behavior of SignPKCS1v15 which properly left pads the
signatures it generates to the size of the modulus.

Fixes golang#21896

Change-Id: I78cf5b225975263fe60aa3acdb458bd4d9cd8de0
@rolandshoemaker rolandshoemaker mentioned this issue Mar 29, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/226203 mentions this issue: crypto/rsa: reject invalid length PKCS#1v1.5 signatures

@gopherbot
Copy link

Change https://golang.org/cl/227651 mentions this issue: [dev.boringcrypto] crypto/internal/boring: reject short signatures in VerifyRSAPKCS1v15

gopherbot pushed a commit that referenced this issue Apr 9, 2020
@FiloSottile
[dev.boringcrypto] crypto/internal/boring: reject short signatures in…
4936781 
… VerifyRSAPKCS1v15

This matches the new crypto/rsa behavior introduced in CL 226203.

Updates #21896

Change-Id: If04eeff933d7310c2baa0f8fd26907892c2397fd
Reviewed-on: https://go-review.googlesource.com/c/go/+/227651
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
gopherbot pushed a commit that referenced this issue May 7, 2020
@FiloSottile
[dev.boringcrypto] crypto/internal/boring: reject short signatures in…
36c94f8 
… VerifyRSAPKCS1v15

This matches the new crypto/rsa behavior introduced in CL 226203.

Updates #21896

Change-Id: If04eeff933d7310c2baa0f8fd26907892c2397fd
Reviewed-on: https://go-review.googlesource.com/c/go/+/227651
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
@margamanterola
Copy link

margamanterola commented Sep 9, 2020 •
edited

Hey! This change broke Flatcar Container Linux build infrastructure.

The signatures for our files are generated with gpg ( gpg --batch --local-user "${FLAGS_sign}" --output "${sigdir}/${sigfile}.sig" --detach-sign "${sigfile}"). Later on in the build pipeline, we download the files using a go utility that verifies the signatures. When moving to go1.15, we've started to get random failures on some signatures that don't match the expected size (signatures 594 bytes long validate fine, but those 593 bytes long don't).

I'm surprised that there's no way to disable this check and that there's no error message specific for this check. I had to go down a rather long rabbit hole to find out that this change was what causing my code to suddenly say openpgp: invalid signature: RSA verification failure.

I understand that the rest of the Go libraries do the right thing, but I'd expect the library to be able to maintain compatibility with gpg. Maybe through some option or something.

@FiloSottile
Copy link
Contributor

crypto/rsa should not accept invalid signatures just because PGP generates them. We had fixed this in golang.org/x/crypto/openpgp years ago, and all its tests pass. If you have an example fo a PGP signature that doesn't validate with the latest golang.org/x/crypto/openpgp, please open a new issue with a reproduction test case.

@margamanterola
Copy link

Thanks for the reply! After further investigation I found that the issue we were seeing was indeed due to having an old version of golang.org/x/crypto/openpgp in go.mod, which didn't have the fixes you mentioned. I've taken care of this and now the signature verifies correctly, even for the file with 593 bytes. Sorry for the noise.

@golang golang locked and limited conversation to collaborators Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

crypto/rsa: reject invalid length PKCS#1v1.5 signatures rolandshoemaker/go
6 participants
@bradfitz @rsc @andybons @FiloSottile @gopherbot @margamanterola