From 61ed6d5c3341f73af9529b4808dd0997c6c86ed4 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Wed, 31 Aug 2022 17:15:08 -0700 Subject: [PATCH] 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 Reviewed-by: Damien Neil Run-TryBot: Roland Shoemaker --- src/crypto/internal/boring/rsa.go | 30 ++++++++++++++++++++++++-- src/crypto/rsa/pss.go | 35 +++++++++++++++++++++---------- src/crypto/rsa/pss_test.go | 28 +++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 13 deletions(-) diff --git a/src/crypto/internal/boring/rsa.go b/src/crypto/internal/boring/rsa.go index f4c4193c00..a1f85591a7 100644 --- a/src/crypto/internal/boring/rsa.go +++ b/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") } + + // 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))) diff --git a/src/crypto/rsa/pss.go b/src/crypto/rsa/pss.go index 29e79bd342..a3e9bfc83b 100644 --- a/src/crypto/rsa/pss.go +++ b/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 diff --git a/src/crypto/rsa/pss_test.go b/src/crypto/rsa/pss_test.go index 51f9760187..ecc02e47d6 100644 --- a/src/crypto/rsa/pss_test.go +++ b/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") + } +} -- 2.48.1