]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/rsa,crypto/internal/boring: fix PSS salt handling
authorRoland Shoemaker <roland@golang.org>
Thu, 1 Sep 2022 00:15:08 +0000 (17:15 -0700)
committerRoland Shoemaker <roland@golang.org>
Tue, 27 Sep 2022 23:19:20 +0000 (23:19 +0000)
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
src/crypto/rsa/pss.go
src/crypto/rsa/pss_test.go

index f4c4193c008aa3b04a2a40fdcc5581adc6337168..a1f85591a77d265358fe9422bac89d5bf4d099f2 100644 (file)
@@ -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")
        }
+
+       // 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 = -1
+               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")
        }
+
+       // 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 // auto-recover
+               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)))
index 29e79bd34279939e726a478f5231bcb163650820..a3e9bfc83ba29fe41f7d1c1af8c6637a01d2a3fa 100644 (file)
@@ -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
index 51f97601878e2cf3986986e44f6098dca1ad46ae..ecc02e47d6c837ddc8ac5e8df034c13926b6a476 100644 (file)
@@ -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")
+       }
+}