GHC 9.6 compatibility by forking more packages

ysangkok commented 1 year ago

Since crypton is a new package, I was wondering whether it doesn't make sense to make big changes now, which is early in its development cycle. That would include stuff like e.g. changes in central dependencies, like basement and foundation.

It seems inconsistent to me to simply fork crypton off, while leaving the dependency on basement intact. Foundation is also suffering from lack of maintenance, and these issues could also affect crypton and warp.

For example, there is a bug in rotateL for Word256 with no reply from the maintainer:

https://github.com/haskell-foundation/foundation/issues/569

And there is this incompatibility with GHC 9.6, which seems fairly foundational (excuse my pun). Since Vincent disagrees with the direction of GHC, it seems he refuses to make it compatible with newer GHC versions. Since crypton is a new fork, it would make sense to offer compatiblity with newer GHC. That way, another fork could be avoided. Would be nice if we could minimize the amount of forking.

EDIT: Note that the above incompatibility has been addressed, and that foundation-0.0.30 is in fact compatible with GHC 9.6 even though the linked issue was not marked as fixed.

kazu-yamamoto commented 1 year ago

Long plan: preparing good bindings to crypto functions in a good crypto library (e.g. OpenSSL) and throw awary crypton OR keep APIs but change the internal drastically to depend on the bindngs
Middle plan: removing dependencies to memory and foundation
Short plan: forking memory and foundation if the middle plain would take time

This is what in my mind.

ysangkok commented 1 year ago

If we remove dependencies on memory and foundation , we need another source for constant-time comparisons and comparisons to an all-zero string. It seems to be available in the libsodium bindings, but I am not sure whether that is the best approach? If you are strictly aiming to reduce the amount of dependencies, it wouldn't be possible.

Here is what I have so far, do you like this approach?

diff --git a/Crypto/Cipher/Types/Utils.hs b/Crypto/Cipher/Types/Utils.hs
index 1185404..06e45d0 100644
--- a/Crypto/Cipher/Types/Utils.hs
+++ b/Crypto/Cipher/Types/Utils.hs
@@ -9,11 +9,10 @@
 --
 module Crypto.Cipher.Types.Utils where

-import           Crypto.Internal.ByteArray (ByteArray)
-import qualified Crypto.Internal.ByteArray as B
+import qualified Data.ByteString as B

 -- | Chunk some input byte array into @sz byte list of byte array.
-chunk :: ByteArray b => Int -> b -> [b]
+chunk :: Int -> B.ByteString -> [B.ByteString]
 chunk sz bs = split bs
   where split b | B.length b <= sz = [b]
                 | otherwise        =
diff --git a/Crypto/Data/Padding.hs b/Crypto/Data/Padding.hs
index 902b132..e2735b8 100644
--- a/Crypto/Data/Padding.hs
+++ b/Crypto/Data/Padding.hs
@@ -14,8 +14,13 @@ module Crypto.Data.Padding
     , unpad
     ) where

-import           Data.ByteArray (ByteArray, Bytes)
-import qualified Data.ByteArray as B
+import           Data.ByteString (ByteString)
+import qualified Data.ByteString as B
+import qualified Data.ByteString.Unsafe as BU
+import           Foreign.Ptr (castPtr)
+import           Foreign.C.Types (CInt)
+import           System.IO.Unsafe (unsafePerformIO)
+import           LibSodium.Bindings.Comparison (sodiumMemcmp)

 -- | Format of padding
 data Format =
@@ -25,7 +30,7 @@ data Format =
     deriving (Show, Eq)

 -- | Apply some pad to a bytearray
-pad :: ByteArray byteArray => Format -> byteArray -> byteArray
+pad :: Format -> ByteString -> ByteString
 pad  PKCS5     bin = pad (PKCS7 8) bin
 pad (PKCS7 sz) bin = bin `B.append` paddingString
   where
@@ -41,21 +46,26 @@ pad (ZERO sz)  bin = bin `B.append` paddingString
     m = len `mod` sz
     len = B.length bin

+constTimingCompare :: ByteString -> ByteString -> CInt
+constTimingCompare p1 p2 = unsafePerformIO $ BU.unsafeUseAsCStringLen p1 $ \(c1, l1) ->
+  BU.unsafeUseAsCStringLen p2 $ \(c2, l2) ->
+    pure $ sodiumMemcmp (castPtr c1) (castPtr c2) (fromIntegral l1)
+
 -- | Try to remove some padding from a bytearray.
-unpad :: ByteArray byteArray => Format -> byteArray -> Maybe byteArray
+unpad :: Format -> ByteString -> Maybe ByteString
 unpad  PKCS5     bin = unpad (PKCS7 8) bin
 unpad (PKCS7 sz) bin
     | len == 0                           = Nothing
     | (len `mod` sz) /= 0                = Nothing
     | paddingSz < 1 || paddingSz > len   = Nothing
-    | paddingWitness `B.constEq` padding = Just content
+    | constTimingCompare paddingWitness padding == 0 = Just content
     | otherwise                          = Nothing
   where
     len         = B.length bin
     paddingByte = B.index bin (len - 1)
     paddingSz   = fromIntegral paddingByte
     (content, padding) = B.splitAt (len - paddingSz) bin
-    paddingWitness     = B.replicate paddingSz paddingByte :: Bytes
+    paddingWitness     = B.replicate paddingSz paddingByte
 unpad (ZERO sz)  bin
     | len == 0                           = Nothing
     | (len `mod` sz) /= 0                = Nothing
diff --git a/Crypto/Internal/ByteArray.hs b/Crypto/Internal/ByteArray.hs
index 57ab57a..592ceb2 100644
--- a/Crypto/Internal/ByteArray.hs
+++ b/Crypto/Internal/ByteArray.hs
@@ -10,30 +10,18 @@
 {-# LANGUAGE BangPatterns #-}
 {-# OPTIONS_HADDOCK hide #-}
 module Crypto.Internal.ByteArray
-    ( module Data.ByteArray
-    , module Data.ByteArray.Mapping
-    , module Data.ByteArray.Encoding
-    , constAllZero
+    ( constAllZero
     ) where

-import Data.ByteArray
-import Data.ByteArray.Mapping
-import Data.ByteArray.Encoding
+import Data.ByteString
+import Data.ByteString.Unsafe

-import Data.Bits ((.|.))
-import Data.Word (Word8)
-import Foreign.Ptr (Ptr)
-import Foreign.Storable (peekByteOff)
+import LibSodium.Bindings.Comparison
+
+import Foreign.Ptr (castPtr)

 import Crypto.Internal.Compat (unsafeDoIO)

-constAllZero :: ByteArrayAccess ba => ba -> Bool
-constAllZero b = unsafeDoIO $ withByteArray b $ \p -> loop p 0 0
-  where
-    loop :: Ptr b -> Int -> Word8 -> IO Bool
-    loop p i !acc
-        | i == len  = return $! acc == 0
-        | otherwise = do
-            e <- peekByteOff p i
-            loop p (i+1) (acc .|. e)
-    len = Data.ByteArray.length b
+constAllZero :: ByteString -> Bool
+constAllZero b = unsafeDoIO $ unsafeUseAsCStringLen b $ \(p1, l1) ->
+  pure $ sodiumIsZero (castPtr p1) (fromIntegral l1) == 1
diff --git a/crypton.cabal b/crypton.cabal
index eec79fe..b0b313d 100644
--- a/crypton.cabal
+++ b/crypton.cabal
@@ -251,8 +251,7 @@ Library
     Build-depends:   base

   Build-depends:     bytestring
-                   , memory >= 0.14.18
-                   , basement >= 0.0.6
+                   , libsodium-bindings
                    , ghc-prim
   ghc-options:       -Wall -fwarn-tabs -optc-O3
   if os(linux)
@@ -462,7 +461,6 @@ Test-Suite test-crypton
                      XSalsa
   Build-Depends:     base >= 0 && < 10
                    , bytestring
-                   , memory
                    , tasty
                    , tasty-quickcheck
                    , tasty-hunit
@@ -479,7 +477,6 @@ Benchmark bench-crypton
   Build-Depends:     base
                    , bytestring
                    , deepseq
-                   , memory
                    , gauge
                    , random
                    , crypton

kazu-yamamoto commented 1 year ago

Here is what I have so far, do you like this approach?

Yes. This is exactly the same as my middle plan.

kazu-yamamoto commented 1 year ago

If we remove dependencies on memory and foundation , we need another source for constant-time comparisons and comparisons to an all-zero string.

I don't know this area well. Doesn't bytestring provide such features?

exarkun commented 1 year ago

If we remove dependencies on memory and foundation , we need another source for constant-time comparisons and comparisons to an all-zero string.

I don't know this area well. Doesn't bytestring provide such features?

It doesn't provide a constant-time comparison function. The Eq instance for strict ByteString comes down to memcmp from the underlying C library. A constant-time comparison function is necessary to avoid leaking timing information to an observer. Timing operations on secrets can result in a side-channel for exfiltrating those secrets.

I assume the same motivation applies for behavior of the constAllZero function.

-chunk :: ByteArray b => Int -> b -> [b]
+chunk :: Int -> B.ByteString -> [B.ByteString]

The ByteArray and ByteArrayAccess classes are quite a useful way to support abstraction over different byte buffer representations. Especially considering these functions are all already generic on the ByteArray / ByteArrayAccess instance, it would be nice if there were a way to keep this. I see that crypton already has internal names for these things (Crypto.Internal.ByteArray) that would make them quite easy to redirect to another library. Perhaps this part of memory can be saved - either as part of crypton or some new library?

kazu-yamamoto commented 1 year ago

FYI: Vincent has completely gone out from the Haskell community. I guess no package updates in the future. https://twitter.com/vincenthz/status/1704395906374889934

joeyh commented 2 months ago

I have been bitten by the dependency on basement. It fails to build on 32 bit architectures with current versions of ghc. Fix is easy but it won't be applied to basement. https://github.com/haskell-foundation/foundation/issues/565

kazu-yamamoto / crypton

GHC 9.6 compatibility by forking more packages #5