crypto/rsa,crypto/internal/boring: fix PSS salt handling

Fix the coversion between our sentinel salt length variables and the
BoringSSL versions in SignRSAPSS. We previously set -1 (hash length
equals salt length) when 0 was passed when we should've been setting
-2. This now matches the conversion that happens in VerifyRSAPSS. Also
adds a note documenting why we do this.

Additionally in non-Boring mode, properly handle passing of salt lengths
with a negative value which aren't one of the magic constants, returning
an error instead of panicking.

See https://commondatastorage.googleapis.com/chromium-boringssl-docs/rsa.h.html#RSA_sign_pss_mgf1
for the BoringSSL docs.

Fixes #54803

Change-Id: Id1bd14dcf0ef4733867367257830ed43e25ef882
Reviewed-on: https://go-review.googlesource.com/c/go/+/426659
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>

src/crypto/internal/boring/rsa.go

@@ -245,14 +245,28 @@ func encrypt(ctx *C.GO_EVP_PKEY_CTX, out *C.uint8_t, outLen *C.size_t, in *C.uin
 	return C._goboringcrypto_EVP_PKEY_encrypt(ctx, out, outLen, in, inLen)
 }
 
+var invalidSaltLenErr = errors.New("crypto/rsa: PSSOptions.SaltLength cannot be negative")
+
 func SignRSAPSS(priv *PrivateKeyRSA, h crypto.Hash, hashed []byte, saltLen int) ([]byte, error) {
 	md := cryptoHashToMD(h)
 	if md == nil {
 		return nil, errors.New("crypto/rsa: unsupported hash function")
 	}
-	if saltLen == 0 {
-		saltLen = -1
+
+	// A salt length of -2 is valid in BoringSSL, but not in crypto/rsa, so reject
+	// it, and lengths < -2, before we convert to the BoringSSL sentinel values.
+	if saltLen <= -2 {
+		return nil, invalidSaltLenErr
 	}
+
+	// BoringSSL uses sentinel salt length values like we do, but the values don't
+	// fully match what we use. We both use -1 for salt length equal to hash length,
+	// but BoringSSL uses -2 to mean maximal size where we use 0. In the latter
+	// case convert to the BoringSSL version.
+	if saltLen == 0 {
+		saltLen = -2
+	}
+
 	var out []byte
 	var outLen C.size_t
 	if priv.withKey(func(key *C.GO_RSA) C.int {
@@ -271,9 +285,21 @@ func VerifyRSAPSS(pub *PublicKeyRSA, h crypto.Hash, hashed, sig []byte, saltLen
 	if md == nil {
 		return errors.New("crypto/rsa: unsupported hash function")
 	}
-	if saltLen == 0 {
-		saltLen = -2 // auto-recover
+
+	// A salt length of -2 is valid in BoringSSL, but not in crypto/rsa, so reject
+	// it, and lengths < -2, before we convert to the BoringSSL sentinel values.
+	if saltLen <= -2 {
+		return invalidSaltLenErr
 	}
+
+	// BoringSSL uses sentinel salt length values like we do, but the values don't
+	// fully match what we use. We both use -1 for salt length equal to hash length,
+	// but BoringSSL uses -2 to mean maximal size where we use 0. In the latter
+	// case convert to the BoringSSL version.
+	if saltLen == 0 {
+		saltLen = -2
+	}
+
 	if pub.withKey(func(key *C.GO_RSA) C.int {
 		return C._goboringcrypto_RSA_verify_pss_mgf1(key, base(hashed), C.size_t(len(hashed)),
 			md, nil, C.int(saltLen), base(sig), C.size_t(len(sig)))

src/crypto/rsa/pss.go

@@ -249,8 +249,8 @@ const (
 
 // PSSOptions contains options for creating and verifying PSS signatures.
 type PSSOptions struct {
-	// SaltLength controls the length of the salt used in the PSS
-	// signature. It can either be a number of bytes, or one of the special
+	// SaltLength controls the length of the salt used in the PSS signature. It
+	// can either be a positive number of bytes, or one of the special
 	// PSSSaltLength constants.
 	SaltLength int
 
@@ -272,12 +272,23 @@ func (opts *PSSOptions) saltLength() int {
 	return opts.SaltLength
 }
 
+var invalidSaltLenErr = errors.New("crypto/rsa: PSSOptions.SaltLength cannot be negative")
+
 // SignPSS calculates the signature of digest using PSS.
 //
 // digest must be the result of hashing the input message using the given hash
 // function. The opts argument may be nil, in which case sensible defaults are
 // used. If opts.Hash is set, it overrides hash.
 func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, digest []byte, opts *PSSOptions) ([]byte, error) {
+	if boring.Enabled && rand == boring.RandReader {
+		bkey, err := boringPrivateKey(priv)
+		if err != nil {
+			return nil, err
+		}
+		return boring.SignRSAPSS(bkey, hash, digest, opts.saltLength())
+	}
+	boring.UnreachableExceptTests()
+
 	if opts != nil && opts.Hash != 0 {
 		hash = opts.Hash
 	}
@@ -288,17 +299,13 @@ func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, digest []byte,
 		saltLength = (priv.N.BitLen()-1+7)/8 - 2 - hash.Size()
 	case PSSSaltLengthEqualsHash:
 		saltLength = hash.Size()
-	}
-
-	if boring.Enabled && rand == boring.RandReader {
-		bkey, err := boringPrivateKey(priv)
-		if err != nil {
-			return nil, err
+	default:
+		// If we get here saltLength is either > 0 or < -1, in the
+		// latter case we fail out.
+		if saltLength <= 0 {
+			return nil, invalidSaltLenErr
 		}
-		return boring.SignRSAPSS(bkey, hash, digest, saltLength)
 	}
-	boring.UnreachableExceptTests()
-
 	salt := make([]byte, saltLength)
 	if _, err := io.ReadFull(rand, salt); err != nil {
 		return nil, err
@@ -326,6 +333,12 @@ func VerifyPSS(pub *PublicKey, hash crypto.Hash, digest []byte, sig []byte, opts
 	if len(sig) != pub.Size() {
 		return ErrVerification
 	}
+	// Salt length must be either one of the special constants (-1 or 0)
+	// or otherwise positive. If it is < PSSSaltLengthEqualsHash (-1)
+	// we return an error.
+	if opts.saltLength() < PSSSaltLengthEqualsHash {
+		return invalidSaltLenErr
+	}
 	s := new(big.Int).SetBytes(sig)
 	m := encrypt(new(big.Int), pub, s)
 	emBits := pub.N.BitLen() - 1

src/crypto/rsa/pss_test.go

@@ -208,6 +208,9 @@ func TestPSSSigning(t *testing.T) {
 		{PSSSaltLengthEqualsHash, 8, false},
 		{PSSSaltLengthAuto, PSSSaltLengthEqualsHash, false},
 		{8, 8, true},
+		{PSSSaltLengthAuto, 42, true},
+		{PSSSaltLengthAuto, 20, false},
+		{PSSSaltLengthAuto, -2, false},
 	}
 
 	hash := crypto.SHA1
@@ -273,3 +276,28 @@ func fromHex(hexStr string) []byte {
 	}
 	return s
 }
+
+func TestInvalidPSSSaltLength(t *testing.T) {
+	key, err := GenerateKey(rand.Reader, 245)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	digest := sha256.Sum256([]byte("message"))
+	// We don't check the exact error matches, because crypto/rsa and crypto/internal/boring
+	// return two different error variables, which have the same content but are not equal.
+	if _, err := SignPSS(rand.Reader, key, crypto.SHA256, digest[:], &PSSOptions{
+		SaltLength: -2,
+		Hash:       crypto.SHA256,
+	}); err.Error() != invalidSaltLenErr.Error() {
+		t.Fatalf("SignPSS unexpected error: got %v, want %v", err, invalidSaltLenErr)
+	}
+
+	// We don't check the specific error here, because crypto/rsa and crypto/internal/boring
+	// return different errors, so we just check that _an error_ was returned.
+	if err := VerifyPSS(&key.PublicKey, crypto.SHA256, []byte{1, 2, 3}, make([]byte, 31), &PSSOptions{
+		SaltLength: -2,
+	}); err == nil {
+		t.Fatal("VerifyPSS unexpected success")
+	}
+}