From 32535011664d6f1634d78e2783d829db130751df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Ch=C3=A9ron?= Date: Tue, 21 Nov 2017 19:25:41 +0100 Subject: [PATCH 1/7] Time-constant P256.scalarAdd and P256.scalarSub --- Crypto/PubKey/ECC/P256.hs | 29 ++++++++--------------------- cbits/p256/p256.c | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/Crypto/PubKey/ECC/P256.hs b/Crypto/PubKey/ECC/P256.hs index 9259f8e..0a3d704 100644 --- a/Crypto/PubKey/ECC/P256.hs +++ b/Crypto/PubKey/ECC/P256.hs @@ -222,34 +222,21 @@ scalarIsZero s = unsafeDoIO $ withScalar s $ \d -> do result <- ccryptonite_p256_is_zero d return $ result /= 0 -scalarNeedReducing :: Ptr P256Scalar -> IO Bool -scalarNeedReducing d = do - c <- ccryptonite_p256_cmp d ccryptonite_SECP256r1_n - return (c >= 0) - -- | Perform addition between two scalars -- -- > a + b scalarAdd :: Scalar -> Scalar -> Scalar scalarAdd a b = - withNewScalarFreeze $ \d -> withScalar a $ \pa -> withScalar b $ \pb -> do - carry <- ccryptonite_p256_add pa pb d - when (carry /= 0) $ void $ ccryptonite_p256_sub d ccryptonite_SECP256r1_n d - needReducing <- scalarNeedReducing d - when needReducing $ do - ccryptonite_p256_mod ccryptonite_SECP256r1_n d d + withNewScalarFreeze $ \d -> withScalar a $ \pa -> withScalar b $ \pb -> + ccryptonite_p256e_modadd ccryptonite_SECP256r1_n pa pb d -- | Perform subtraction between two scalars -- -- > a - b scalarSub :: Scalar -> Scalar -> Scalar scalarSub a b = - withNewScalarFreeze $ \d -> withScalar a $ \pa -> withScalar b $ \pb -> do - borrow <- ccryptonite_p256_sub pa pb d - when (borrow /= 0) $ void $ ccryptonite_p256_add d ccryptonite_SECP256r1_n d - --needReducing <- scalarNeedReducing d - --when needReducing $ do - -- ccryptonite_p256_mod ccryptonite_SECP256r1_n d d + withNewScalarFreeze $ \d -> withScalar a $ \pa -> withScalar b $ \pb -> + ccryptonite_p256e_modsub ccryptonite_SECP256r1_n pa pb d -- | Give the inverse of the scalar -- @@ -352,12 +339,12 @@ foreign import ccall "cryptonite_p256_is_zero" ccryptonite_p256_is_zero :: Ptr P256Scalar -> IO CInt foreign import ccall "cryptonite_p256_clear" ccryptonite_p256_clear :: Ptr P256Scalar -> IO () -foreign import ccall "cryptonite_p256_add" - ccryptonite_p256_add :: Ptr P256Scalar -> Ptr P256Scalar -> Ptr P256Scalar -> IO CInt +foreign import ccall "cryptonite_p256e_modadd" + ccryptonite_p256e_modadd :: Ptr P256Scalar -> Ptr P256Scalar -> Ptr P256Scalar -> Ptr P256Scalar -> IO () foreign import ccall "cryptonite_p256_add_d" ccryptonite_p256_add_d :: Ptr P256Scalar -> P256Digit -> Ptr P256Scalar -> IO CInt -foreign import ccall "cryptonite_p256_sub" - ccryptonite_p256_sub :: Ptr P256Scalar -> Ptr P256Scalar -> Ptr P256Scalar -> IO CInt +foreign import ccall "cryptonite_p256e_modsub" + ccryptonite_p256e_modsub :: Ptr P256Scalar -> Ptr P256Scalar -> Ptr P256Scalar -> Ptr P256Scalar -> IO () foreign import ccall "cryptonite_p256_cmp" ccryptonite_p256_cmp :: Ptr P256Scalar -> Ptr P256Scalar -> IO CInt foreign import ccall "cryptonite_p256_mod" diff --git a/cbits/p256/p256.c b/cbits/p256/p256.c index 4f6a573..ec69f64 100644 --- a/cbits/p256/p256.c +++ b/cbits/p256/p256.c @@ -386,3 +386,23 @@ void cryptonite_p256_to_bin(const cryptonite_p256_int* src, uint8_t dst[P256_NBY p += 4; } } + +/* + "p256e" functions are not part of the original source +*/ + +// c = a + b mod MOD +void cryptonite_p256e_modadd(const cryptonite_p256_int* MOD, const cryptonite_p256_int* a, const cryptonite_p256_int* b, cryptonite_p256_int* c) { + int carry = cryptonite_p256_add(a, b, c); + + // same as cryptonite_p256_mod, but with top = carry + addM(MOD, 0, P256_DIGITS(c), subM(MOD, carry, P256_DIGITS(c), -1)); +} + +// c = a - b mod MOD +void cryptonite_p256e_modsub(const cryptonite_p256_int* MOD, const cryptonite_p256_int* a, const cryptonite_p256_int* b, cryptonite_p256_int* c) { + int borrow = cryptonite_p256_sub(a, b, c); + + // use borrow as mask in order to make difference positive when necessary + addM(MOD, 0, P256_DIGITS(c), borrow); +} From e3edc100c32db586955e87c86be8f0e6d53972ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Ch=C3=A9ron?= Date: Sat, 23 Mar 2019 07:54:23 +0100 Subject: [PATCH 2/7] Remove unnecessary import --- Crypto/PubKey/ECC/P256.hs | 1 - 1 file changed, 1 deletion(-) diff --git a/Crypto/PubKey/ECC/P256.hs b/Crypto/PubKey/ECC/P256.hs index 0a3d704..3c350cd 100644 --- a/Crypto/PubKey/ECC/P256.hs +++ b/Crypto/PubKey/ECC/P256.hs @@ -45,7 +45,6 @@ module Crypto.PubKey.ECC.P256 import Data.Word import Foreign.Ptr import Foreign.C.Types -import Control.Monad import Crypto.Internal.Compat import Crypto.Internal.Imports From 47123ed97a5103d7240f0d78825d3a9010b8ca7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Ch=C3=A9ron?= Date: Sun, 24 Mar 2019 08:02:42 +0100 Subject: [PATCH 3/7] Better P256 scalar primitives Allows scalars in full range [ 0 .. 2^256-1 ]. Modular reduction is added a few more operations with conditional selection. --- cbits/p256/p256.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/cbits/p256/p256.c b/cbits/p256/p256.c index ec69f64..bd94f6a 100644 --- a/cbits/p256/p256.c +++ b/cbits/p256/p256.c @@ -391,18 +391,20 @@ void cryptonite_p256_to_bin(const cryptonite_p256_int* src, uint8_t dst[P256_NBY "p256e" functions are not part of the original source */ +#define MSB_COMPLEMENT(x) (((x) >> (P256_BITSPERDIGIT - 1)) - 1) + // c = a + b mod MOD void cryptonite_p256e_modadd(const cryptonite_p256_int* MOD, const cryptonite_p256_int* a, const cryptonite_p256_int* b, cryptonite_p256_int* c) { - int carry = cryptonite_p256_add(a, b, c); - - // same as cryptonite_p256_mod, but with top = carry - addM(MOD, 0, P256_DIGITS(c), subM(MOD, carry, P256_DIGITS(c), -1)); + cryptonite_p256_digit top = cryptonite_p256_add(a, b, c); + top = subM(MOD, top, P256_DIGITS(c), -1); + top = subM(MOD, top, P256_DIGITS(c), MSB_COMPLEMENT(top)); + addM(MOD, 0, P256_DIGITS(c), top); } // c = a - b mod MOD void cryptonite_p256e_modsub(const cryptonite_p256_int* MOD, const cryptonite_p256_int* a, const cryptonite_p256_int* b, cryptonite_p256_int* c) { - int borrow = cryptonite_p256_sub(a, b, c); - - // use borrow as mask in order to make difference positive when necessary - addM(MOD, 0, P256_DIGITS(c), borrow); + cryptonite_p256_digit top = cryptonite_p256_sub(a, b, c); + top = addM(MOD, top, P256_DIGITS(c), ~MSB_COMPLEMENT(top)); + top = subM(MOD, top, P256_DIGITS(c), MSB_COMPLEMENT(top)); + addM(MOD, 0, P256_DIGITS(c), top); } From 399fc891daf2bdaff1ecac7ac1dcf8bb08d101d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Ch=C3=A9ron?= Date: Sun, 24 Mar 2019 08:05:49 +0100 Subject: [PATCH 4/7] Test P256 primitives will full scalar range --- tests/KAT_PubKey/P256.hs | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/tests/KAT_PubKey/P256.hs b/tests/KAT_PubKey/P256.hs index 2d6bb2b..9e43ecd 100644 --- a/tests/KAT_PubKey/P256.hs +++ b/tests/KAT_PubKey/P256.hs @@ -17,7 +17,19 @@ newtype P256Scalar = P256Scalar Integer deriving (Show,Eq,Ord) instance Arbitrary P256Scalar where - arbitrary = P256Scalar . getQAInteger <$> arbitrary + -- Cover the full range up to 2^256-1 except 0 and curveN. To test edge + -- cases with arithmetic functions, some values close to 0, curveN and + -- 2^256 are given higher frequency. + arbitrary = P256Scalar <$> oneof + [ choose (1, w) + , choose (w + 1, curveN - w - 1) + , choose (curveN - w, curveN - 1) + , choose (curveN + 1, curveN + w) + , choose (curveN + w + 1, high - w - 1) + , choose (high - w, high - 1) + ] + where high = 2^(256 :: Int) + w = 100 curve = ECC.getCurveByName ECC.SEC_p256r1 curveN = ECC.ecc_n . ECC.common_curve $ curve @@ -27,9 +39,8 @@ pointP256ToECC :: P256.Point -> ECC.Point pointP256ToECC = uncurry ECC.Point . P256.pointToIntegers unP256Scalar :: P256Scalar -> P256.Scalar -unP256Scalar (P256Scalar r') = - let r = if r' == 0 then 0x2901 else (r' `mod` curveN) - rBytes = i2ospScalar r +unP256Scalar (P256Scalar r) = + let rBytes = i2ospScalar r in case P256.scalarFromBinary rBytes of CryptoFailed err -> error ("cannot convert scalar: " ++ show err) CryptoPassed scalar -> scalar @@ -41,7 +52,7 @@ unP256Scalar (P256Scalar r') = Just b -> b unP256 :: P256Scalar -> Integer -unP256 (P256Scalar r') = if r' == 0 then 0x2901 else (r' `mod` curveN) +unP256 (P256Scalar r) = r p256ScalarToInteger :: P256.Scalar -> Integer p256ScalarToInteger s = os2ip (P256.scalarToBinary s :: Bytes) @@ -55,9 +66,8 @@ yR = 0x8d585cbb2e1327d75241a8a122d7620dc33b13315aa5c9d46d013011744ac264 tests = testGroup "P256" [ testGroup "scalar" - [ testProperty "marshalling" $ \(QAInteger r') -> - let r = r' `mod` curveN - rBytes = i2ospScalar r + [ testProperty "marshalling" $ \(QAInteger r) -> + let rBytes = i2ospScalar r in case P256.scalarFromBinary rBytes of CryptoFailed err -> error (show err) CryptoPassed scalar -> rBytes `propertyEq` P256.scalarToBinary scalar @@ -66,12 +76,12 @@ tests = testGroup "P256" r' = P256.scalarAdd (unP256Scalar r1) (unP256Scalar r2) in r `propertyEq` p256ScalarToInteger r' , testProperty "add0" $ \r -> - let v = unP256 r + let v = unP256 r `mod` curveN v' = P256.scalarAdd (unP256Scalar r) P256.scalarZero in v `propertyEq` p256ScalarToInteger v' , testProperty "add-n-1" $ \r -> let nm1 = throwCryptoError $ P256.scalarFromInteger (curveN - 1) - v = unP256 r + v = unP256 r `mod` curveN v' = P256.scalarAdd (unP256Scalar r) nm1 in (((curveN - 1) + v) `mod` curveN) `propertyEq` p256ScalarToInteger v' , testProperty "sub" $ \r1 r2 -> @@ -133,7 +143,8 @@ tests = testGroup "P256" pe2 = ECC.pointMul curve (unP256 r2) curveGen pR = P256.toPoint (P256.scalarAdd (unP256Scalar r1) (unP256Scalar r2)) peR = ECC.pointAdd curve pe1 pe2 - in propertyHold [ eqTest "p256" pR (P256.pointAdd p1 p2) + in (unP256 r1 + unP256 r2) `mod` curveN /= 0 ==> + propertyHold [ eqTest "p256" pR (P256.pointAdd p1 p2) , eqTest "ecc" peR (pointP256ToECC pR) ] From 15f117d9c3793a0bf8ea2b6a298bafe394b55a2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Ch=C3=A9ron?= Date: Mon, 25 Mar 2019 06:47:21 +0100 Subject: [PATCH 5/7] Remove tests add-n-1 and sub-n-1 Operation with value close to the curve order is now tested in other tests. This tests substraction with 0 instead. --- tests/KAT_PubKey/P256.hs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/tests/KAT_PubKey/P256.hs b/tests/KAT_PubKey/P256.hs index 9e43ecd..7d5e95f 100644 --- a/tests/KAT_PubKey/P256.hs +++ b/tests/KAT_PubKey/P256.hs @@ -79,11 +79,6 @@ tests = testGroup "P256" let v = unP256 r `mod` curveN v' = P256.scalarAdd (unP256Scalar r) P256.scalarZero in v `propertyEq` p256ScalarToInteger v' - , testProperty "add-n-1" $ \r -> - let nm1 = throwCryptoError $ P256.scalarFromInteger (curveN - 1) - v = unP256 r `mod` curveN - v' = P256.scalarAdd (unP256Scalar r) nm1 - in (((curveN - 1) + v) `mod` curveN) `propertyEq` p256ScalarToInteger v' , testProperty "sub" $ \r1 r2 -> let r = (unP256 r1 - unP256 r2) `mod` curveN r' = P256.scalarSub (unP256Scalar r1) (unP256Scalar r2) @@ -93,11 +88,10 @@ tests = testGroup "P256" [ eqTest "r1-r2" r (p256ScalarToInteger r') , eqTest "r2-r1" v (p256ScalarToInteger v') ] - , testProperty "sub-n-1" $ \r -> - let nm1 = throwCryptoError $ P256.scalarFromInteger (curveN - 1) - v = unP256 r - v' = P256.scalarSub (unP256Scalar r) nm1 - in ((v - (curveN - 1)) `mod` curveN) `propertyEq` p256ScalarToInteger v' + , testProperty "sub0" $ \r -> + let v = unP256 r `mod` curveN + v' = P256.scalarSub (unP256Scalar r) P256.scalarZero + in v `propertyEq` p256ScalarToInteger v' , testProperty "inv" $ \r' -> let inv = inverseCoprimes (unP256 r') curveN inv' = P256.scalarInv (unP256Scalar r') From 6f67cefa3dca6cc8c2de12339cb8d297754b183f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Ch=C3=A9ron?= Date: Tue, 26 Mar 2019 06:24:00 +0100 Subject: [PATCH 6/7] Remove code duplication --- tests/KAT_PubKey/P256.hs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tests/KAT_PubKey/P256.hs b/tests/KAT_PubKey/P256.hs index 7d5e95f..c570548 100644 --- a/tests/KAT_PubKey/P256.hs +++ b/tests/KAT_PubKey/P256.hs @@ -38,18 +38,18 @@ curveGen = ECC.ecc_g . ECC.common_curve $ curve pointP256ToECC :: P256.Point -> ECC.Point pointP256ToECC = uncurry ECC.Point . P256.pointToIntegers +i2ospScalar :: Integer -> Bytes +i2ospScalar i = + case i2ospOf 32 i of + Nothing -> error "invalid size of P256 scalar" + Just b -> b + unP256Scalar :: P256Scalar -> P256.Scalar unP256Scalar (P256Scalar r) = let rBytes = i2ospScalar r in case P256.scalarFromBinary rBytes of CryptoFailed err -> error ("cannot convert scalar: " ++ show err) CryptoPassed scalar -> scalar - where - i2ospScalar :: Integer -> Bytes - i2ospScalar i = - case i2ospOf 32 i of - Nothing -> error "invalid size of P256 scalar" - Just b -> b unP256 :: P256Scalar -> Integer unP256 (P256Scalar r) = r @@ -147,9 +147,3 @@ tests = testGroup "P256" pe = ECC.pointMul curve (unP256 r) curveGen pR = P256.pointNegate p in ECC.pointNegate curve pe `propertyEq` (pointP256ToECC pR) - - i2ospScalar :: Integer -> Bytes - i2ospScalar i = - case i2ospOf 32 i of - Nothing -> error "invalid size of P256 scalar" - Just b -> b From 7e5dbeb146655261546cb599417237e31d470f3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Ch=C3=A9ron?= Date: Tue, 26 Mar 2019 06:25:45 +0100 Subject: [PATCH 7/7] Use vector/vectorOf from QuickCheck and simplify --- tests/Utils.hs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/Utils.hs b/tests/Utils.hs index 11622c4..03ba521 100644 --- a/tests/Utils.hs +++ b/tests/Utils.hs @@ -2,7 +2,6 @@ module Utils where import Control.Applicative -import Control.Monad (replicateM) import Data.Char import Data.Word import Data.List @@ -28,13 +27,13 @@ newtype ChunkingLen = ChunkingLen [Int] deriving (Show,Eq) instance Arbitrary ChunkingLen where - arbitrary = ChunkingLen `fmap` replicateM 16 (choose (0,14)) + arbitrary = ChunkingLen `fmap` vectorOf 16 (choose (0,14)) newtype ChunkingLen0_127 = ChunkingLen0_127 [Int] deriving (Show,Eq) instance Arbitrary ChunkingLen0_127 where - arbitrary = ChunkingLen0_127 `fmap` replicateM 16 (choose (0,127)) + arbitrary = ChunkingLen0_127 `fmap` vectorOf 16 (choose (0,127)) newtype ArbitraryBS0_2901 = ArbitraryBS0_2901 ByteString @@ -63,7 +62,7 @@ instance Arbitrary QAInteger where arbitrary = oneof [ QAInteger . fromIntegral <$> (choose (0, 65536) :: Gen Int) -- small integer , larger <$> choose (0,4096) <*> choose (0, 65536) -- medium integer - , QAInteger . os2ip . B.pack <$> (choose (0,32) >>= \n -> replicateM n arbitrary) -- [ 0 .. 2^32 ] sized integer + , QAInteger . os2ip <$> arbitraryBSof 0 32 -- [ 0 .. 2^32 ] sized integer ] where larger :: Int -> Int -> QAInteger @@ -73,10 +72,10 @@ instance Arbitrary QAInteger where somePrime = 18446744073709551557 arbitraryBS :: Int -> Gen ByteString -arbitraryBS n = B.pack `fmap` replicateM n arbitrary +arbitraryBS = fmap B.pack . vector arbitraryBSof :: Int -> Int -> Gen ByteString -arbitraryBSof minSize maxSize = choose (minSize, maxSize) >>= \n -> (B.pack `fmap` replicateM n arbitrary) +arbitraryBSof minSize maxSize = choose (minSize, maxSize) >>= arbitraryBS chunkS :: ChunkingLen -> ByteString -> [ByteString] chunkS (ChunkingLen originalChunks) = loop originalChunks