Fix RSAPSS salt length conversion #37
Conversation
openssl/rsa.go
Outdated
| func SignRSAPSS(priv *PrivateKeyRSA, h crypto.Hash, hashed []byte, saltLen int) ([]byte, error) { | ||
| if saltLen == 0 { | ||
| saltLen = -1 // RSA_PSS_SALTLEN_DIGEST | ||
| slen, err := saltLength(saltLen, true) |
There was a problem hiding this comment.
This seems like a lot of things that only vary by how many letters of saltLength they chop out. 😄 This is what comes to mind:
| slen, err := saltLength(saltLen, true) | |
| opensslSaltLen, err := convertGoSaltLength(saltLen, true) |
But... these functions are short, and the types are different, so there's not much chance to actually mix them up and I think it's fine as-is if you prefer.
There was a problem hiding this comment.
Too mach chopping 🥢. I've simplified saltLength and changed a couple of names to make it clearer.
| if err != nil { | ||
| t.Fatal(err) | ||
| for i, test := range saltLengthCombinations { | ||
| signed, err := openssl.SignRSAPSS(priv, crypto.SHA256, hashed, test.signSaltLength) |
There was a problem hiding this comment.
When I saw this, I thought it was odd that it wasn't set up as a normal table test with named cases, but I see it's based on https://go-review.googlesource.com/c/go/+/426659/4/src/crypto/rsa/pss_test.go. Might be worth linking to it, https://github.com/golang/go/blob/54182ff54a687272dd7632c3a963e036ce03cb7c/src/crypto/rsa/pss_test.go#L200 ?
Co-authored-by: Davis Goodin <[email protected]>
4 participants
Fix the conversion between our sentinel salt length variables and the OpenSSL versions in SignRSAPSS and VerifyRSAPSS . We previously set -1 (hash length equals salt length) when 0 was passed when we should've been setting -2 or -3.
This buggy behavior was inherited from BoringSSL bindings since the very beginning, but it hasn't triggered any bug yet because Go
rsa.SignPSSandrsa.VerifyPSSresolved the salt length special case before passing it to boring. There is an upstream CL that will remove this handling from the Go side and fix the Boring side.We should fix the OpenSSL bindings before that CL lands, else the sync process will fail in the test phase.