diff --git a/CHANGELOG.md b/CHANGELOG.md --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,23 @@ # CHANGELOG for crypton +## 2.1.7 + +One fix: RSA-PSS verification was accepting a signature RFC 8017 says to +refuse. It is a conformance fault rather than a forgery -- producing such a +signature takes the private key, since it is the signer who chooses the +encoding, and a third party holding a valid signature cannot turn it into +one of these. + +* fix(pss): RSA-PSS verification refuses an encoding with a bit set outside + `emBits`, as RFC 8017 9.1.2 step 6 requires. Step 9 clears those bits in + DB and crypton did that; clearing is not checking, so an encoding the + standard calls inconsistent verified as though it were sound -- the bit + that made it wrong was thrown away before anything looked at it. Only the + signer can produce such a signature, since it takes the private key to + sign a chosen encoding, so this is conformance rather than forgery. Found + with tlsfuzzer, in the `xor 0x80 at 0` case of + `test-tls13-certificate-verify.py`, while testing hs-tls + ## 2.1.6 Two things a caller could walk into, the licence field saying what the tree diff --git a/Crypto/PubKey/ECC/P256.hs b/Crypto/PubKey/ECC/P256.hs --- a/Crypto/PubKey/ECC/P256.hs +++ b/Crypto/PubKey/ECC/P256.hs @@ -413,8 +413,6 @@ foreign import ccall "crypton_p256e_scalar_invert" ccrypton_p256e_scalar_invert :: Ptr P256Scalar -> Ptr P256Scalar -> IO () --- foreign import ccall "crypton_p256_modinv" --- ccrypton_p256_modinv :: Ptr P256Scalar -> Ptr P256Scalar -> Ptr P256Scalar -> IO () foreign import ccall "crypton_p256_modinv_vartime" ccrypton_p256_modinv_vartime :: Ptr P256Scalar -> Ptr P256Scalar -> Ptr P256Scalar -> IO () diff --git a/Crypto/PubKey/RSA/PSS.hs b/Crypto/PubKey/RSA/PSS.hs --- a/Crypto/PubKey/RSA/PSS.hs +++ b/Crypto/PubKey/RSA/PSS.hs @@ -28,7 +28,7 @@ import Crypto.PubKey.RSA.Prim import Crypto.PubKey.RSA.Types import Crypto.Random.Types -import Data.Bits (shiftR, xor, (.&.)) +import Data.Bits (complement, shiftR, xor, (.&.)) import Data.Word import Crypto.Internal.ByteArray (ByteArray, ByteArrayAccess) @@ -233,6 +233,7 @@ | B.length s /= k = False | os2ip s >= public_n pk = False | B.any (/= 0) pre = False + | B.any (\x -> x .&. topBits /= 0) (B.take 1 maskedDB) = False | B.last em /= pssTrailerField params = False | B.any (/= 0) ps0 = False | b1 /= B.singleton 1 = False @@ -246,6 +247,13 @@ emLen = if emTruncate pubBits then k - 1 else k dbLen = emLen - hashLen - 1 pubBits = numBits (public_n pk) + -- RFC 8017 9.1.2 step 6: the leftmost 8*emLen - emBits bits of the + -- leftmost octet of maskedDB have to be zero already. Step 9 clears + -- them in DB, which is what normalizeToKeySize does below, and clearing + -- is not checking: without this an encoding with the top bit set -- one + -- the standard calls inconsistent -- verifies as though it were sound, + -- because the bit that made it wrong is thrown away before it is read. + topBits = complement (normalizeMask pubBits) -- unmarshall fields (pre, em) = B.splitAt (k - emLen) (ep pk s) -- drop 0..1 byte maskedDB = B.take dbLen em @@ -263,7 +271,12 @@ normalizeToKeySize :: Int -> [Word8] -> [Word8] normalizeToKeySize _ [] = [] -- very unlikely -normalizeToKeySize bits (x : xs) = x .&. mask : xs +normalizeToKeySize bits (x : xs) = x .&. normalizeMask bits : xs + +-- | The bits of the leftmost octet that belong to the encoding: the low +-- @emBits `mod` 8@ of them, or all eight when that is zero. Its complement +-- is the bits RFC 8017 requires to be zero. +normalizeMask :: Int -> Word8 +normalizeMask bits = if sh > 0 then 0xff `shiftR` (8 - sh) else 0xff where - mask = if sh > 0 then 0xff `shiftR` (8 - sh) else 0xff sh = (bits - 1) .&. 0x7 diff --git a/cbits/include32/p256/p256.h b/cbits/include32/p256/p256.h --- a/cbits/include32/p256/p256.h +++ b/cbits/include32/p256/p256.h @@ -83,16 +83,9 @@ const crypton_p256_int* b, crypton_p256_int* c); -// b := 1 / a % MOD -// MOD best be SECP256r1_n -void crypton_p256_modinv( - const crypton_p256_int* MOD, - const crypton_p256_int* a, - crypton_p256_int* b); - -// b := 1 / a % MOD +// b := 1 / a % MOD, in time that depends on a // MOD best be SECP256r1_n -// Faster than crypton_p256_modinv() +// Answers zero for an a that has no inverse, which is zero and MOD void crypton_p256_modinv_vartime( const crypton_p256_int* MOD, const crypton_p256_int* a, @@ -130,13 +123,6 @@ void crypton_p256_base_point_mul(const crypton_p256_int *n, crypton_p256_int *out_x, crypton_p256_int *out_y); - -// {out_x,out_y} := n{in_x,in_y} -void crypton_p256_point_mul(const crypton_p256_int *n, - const crypton_p256_int *in_x, - const crypton_p256_int *in_y, - crypton_p256_int *out_x, - crypton_p256_int *out_y); // {out_x,out_y} := n1G + n2{in_x,in_y} void crypton_p256_points_mul_vartime( diff --git a/cbits/include64/p256/p256.h b/cbits/include64/p256/p256.h --- a/cbits/include64/p256/p256.h +++ b/cbits/include64/p256/p256.h @@ -83,16 +83,9 @@ const crypton_p256_int* b, crypton_p256_int* c); -// b := 1 / a % MOD -// MOD best be SECP256r1_n -void crypton_p256_modinv( - const crypton_p256_int* MOD, - const crypton_p256_int* a, - crypton_p256_int* b); - -// b := 1 / a % MOD +// b := 1 / a % MOD, in time that depends on a // MOD best be SECP256r1_n -// Faster than crypton_p256_modinv() +// Answers zero for an a that has no inverse, which is zero and MOD void crypton_p256_modinv_vartime( const crypton_p256_int* MOD, const crypton_p256_int* a, @@ -130,13 +123,6 @@ void crypton_p256_base_point_mul(const crypton_p256_int *n, crypton_p256_int *out_x, crypton_p256_int *out_y); - -// {out_x,out_y} := n{in_x,in_y} -void crypton_p256_point_mul(const crypton_p256_int *n, - const crypton_p256_int *in_x, - const crypton_p256_int *in_y, - crypton_p256_int *out_x, - crypton_p256_int *out_y); // {out_x,out_y} := n1G + n2{in_x,in_y} void crypton_p256_points_mul_vartime( diff --git a/cbits/p256/p256.c b/cbits/p256/p256.c --- a/cbits/p256/p256.c +++ b/cbits/p256/p256.c @@ -334,8 +334,8 @@ branch both U and V have to be odd to reach. The other input without an inverse is MOD itself -- 2*MOD does not fit in 256 bits, so there is no third -- and that one already leaves here as zero, which is also what - crypton_p256_modinv's constant-time counterpart returns. Answer the same - for zero rather than not answering. + Crypto.PubKey.ECC.P256's scalarInvSafe answers for both. Answer the + same for zero rather than not answering. Reachable: Crypto.PubKey.ECC.P256 exports scalarInv, and scalarFromBinary accepts any 256 bits. A hang inside a foreign call cannot be interrupted diff --git a/cbits/tests/width/p256_width.c b/cbits/tests/width/p256_width.c --- a/cbits/tests/width/p256_width.c +++ b/cbits/tests/width/p256_width.c @@ -7,9 +7,8 @@ #include #include "p256/p256.h" -/* The header declares crypton_p256_point_mul and crypton_p256_modinv, and - nothing defines them. What exists is this family, which has no header at - all -- the Haskell side declares it through the FFI. */ +/* This family has no header at all -- the Haskell side declares it through + the FFI -- so it is declared here. */ void crypton_p256e_point_mul(const crypton_p256_int *n, const crypton_p256_int *in_x, const crypton_p256_int *in_y, crypton_p256_int *out_x, crypton_p256_int *out_y); diff --git a/crypton.cabal b/crypton.cabal --- a/crypton.cabal +++ b/crypton.cabal @@ -1,6 +1,6 @@ cabal-version: 3.0 name: crypton -version: 2.1.6 +version: 2.1.7 -- crypton's own code is BSD-3-Clause. The parts of -- cbits/aes/gcm_fused_x86.c that follow picotls's fusion are MIT, and the -- vendored s2n-bignum assembly in cbits/s2n is taken under ISC; each has diff --git a/tests/PubKey/PSSSpec.hs b/tests/PubKey/PSSSpec.hs --- a/tests/PubKey/PSSSpec.hs +++ b/tests/PubKey/PSSSpec.hs @@ -2,9 +2,14 @@ module PubKey.PSSSpec (spec) where +import Crypto.Number.Basic (numBits) import Crypto.Number.Serialize (i2ospOf_, os2ip) import Crypto.PubKey.RSA +import Crypto.PubKey.RSA.Prim (dp, ep) import qualified Crypto.PubKey.RSA.PSS as PSS +import qualified Data.ByteString as B +import qualified Data.Bits as Bits +import Data.Word (Word8) import Imports @@ -494,10 +499,61 @@ , os2ip sg + modulus < 2 ^ (8 * k) ] +-- | RFC 8017 9.1.2 step 6: the leftmost @8*emLen - emBits@ bits of the +-- leftmost octet of maskedDB have to be zero. Step 9 clears them in DB, +-- and clearing is not checking -- an encoding with one of them set used to +-- verify as though it were sound, because the bit that made it wrong was +-- thrown away before anything looked at it. +-- +-- Only the signer can produce such a thing, since it takes the private key +-- to sign a chosen encoding, so this is conformance rather than forgery. +-- The vectors are walked for one whose altered encoding stays below the +-- modulus, as the signature range tests above do, because an encoding at or +-- past it says nothing. +step6Tests :: Spec +step6Tests = describe "an encoding with a bit outside emBits set" $ do + it "the honest signature verifies, key 1024" $ + verifies rsaKey1 (fst (altered rsaKey1 vectorsKey1)) `shouldBe` True + it "and the altered one does not, key 1024" $ + verifies rsaKey1 (snd (altered rsaKey1 vectorsKey1)) `shouldBe` False + it "the honest signature verifies, key 1026" $ + verifies rsaKey3 (fst (altered rsaKey3 vectorsKey3)) `shouldBe` True + it "and the altered one does not, key 1026" $ + verifies rsaKey3 (snd (altered rsaKey3 vectorsKey3)) `shouldBe` False + it "key 1025 has no bits outside emBits to set" $ + forbidden (numBits (public_n (private_pub rsaKey2))) `shouldBe` 0 + where + verifies key (v, sg) = + PSS.verify PSS.defaultPSSParamsSHA1 (private_pub key) (message v) sg + + -- the bits of the leftmost octet the standard requires to be zero + forbidden bits = Bits.complement mask + where + mask = if sh > 0 then 0xff `Bits.shiftR` (8 - sh) else 0xff :: Word8 + sh = (bits - 1) Bits..&. 0x7 + + -- the first vector whose encoding, with a forbidden bit set, is still + -- below the modulus, paired as (honest, altered) + altered key vs = firstVector + [ ((v, signature v), (v, dp Nothing key em')) + | v <- vs + , let pub = private_pub key + em = ep pub (signature v) + bit = lowestSet (forbidden (numBits (public_n pub))) + em' = B.cons (B.head em Bits..|. bit) (B.tail em) + , B.head em Bits..&. forbidden (numBits (public_n pub)) == 0 + , os2ip em' < public_n pub + ] + + -- the forbidden bit worth setting is the lowest of them: it is the one + -- that adds least, and an encoding at or past the modulus proves nothing + lowestSet w = minimum ([2 ^ i | i <- [0 .. 7 :: Int], Bits.testBit w i]) + spec :: Spec spec = describe "RSA-PSS" $ do signatureRangeTests + step6Tests describe "signature internal" $ do doSignTest rsaKeyInt katZero vectorInt describe "verify internal" $ do