Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial bindings to hashing algorithms, and add code style #1

Closed
wants to merge 2 commits into from

Conversation

Kleidukos
Copy link
Member

@Kleidukos Kleidukos commented Jan 10, 2022

This PR brings:

  • Code guidelines for C FFI
  • Proper license for IOG / Cardano code
  • Bindings to hashing algorithms
    • Blake2b
    • SHA-256
    • SHA-512

Dependencies

@Kleidukos Kleidukos force-pushed the add-initial-bindings branch 5 times, most recently from 84c1d33 to ca4f755 Compare January 10, 2022 19:08
-- | @int sodium_compare(const void * const b1_, const void * const b2_, size_t len);@
--
-- <https://libsodium.gitbook.io/doc/helpers#comparing-large-numbers>
foreign import capi unsafe "sodium.h sodium_compare" c_sodium_compare :: Ptr a -> Ptr a -> CSize -> IO CInt
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put that in its own module

Comment on lines 181 to 195
-- From https://libsodium.gitbook.io/doc/advanced/sha-2_hash_function
-- and https://libsodium.gitbook.io/doc/hashing/generic_hashing

type CRYPTO_SHA256_BYTES = #{const crypto_hash_sha256_BYTES}
type CRYPTO_SHA512_BYTES = #{const crypto_hash_sha512_BYTES}
type CRYPTO_BLAKE2B_256_BYTES = #{const crypto_generichash_blake2b_BYTES}

type CRYPTO_SHA256_STATE_SIZE = #{size crypto_hash_sha256_state}
type CRYPTO_SHA512_STATE_SIZE = #{size crypto_hash_sha512_state}
type CRYPTO_BLAKE2B_256_STATE_SIZE = #{size crypto_generichash_blake2b_state}

type CRYPTO_SIGN_ED25519_BYTES = #{const crypto_sign_ed25519_BYTES}
type CRYPTO_SIGN_ED25519_SEEDBYTES = #{const crypto_sign_ed25519_SEEDBYTES}
type CRYPTO_SIGN_ED25519_PUBLICKEYBYTES = #{const crypto_sign_ed25519_PUBLICKEYBYTES}
type CRYPTO_SIGN_ED25519_SECRETKEYBYTES = #{const crypto_sign_ed25519_SECRETKEYBYTES}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything goes

Comment on lines 29 to 31

-- A pointer which knows the size of underlying memory block
newtype SizedPtr (n :: Nat) = SizedPtr (Ptr Void)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Irrelevant

Comment on lines 32 to 53

-- | Like 'allocaBytes'.
allocaSized :: forall n b. KnownNat n => (SizedPtr n -> IO b) -> IO b
allocaSized k = allocaBytes (fromCInt size) (k . SizedPtr)
where
size :: CInt
size = fromInteger (natVal (Proxy @n))

memcpySized :: forall n. KnownNat n => SizedPtr n -> SizedPtr n -> IO ()
memcpySized (SizedPtr dest) (SizedPtr src) = void (c_memcpy dest src size)
where
size :: CSize
size = fromInteger (natVal (Proxy @n))

memsetSized :: forall n. KnownNat n => SizedPtr n -> Word8 -> IO ()
memsetSized (SizedPtr s) c = void (c_memset s (fromIntegral c) size)
where
size :: CSize
size = fromInteger (natVal (Proxy @n))

fromCInt :: CInt -> Int
fromCInt = fromIntegral
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can go away

Comment on lines 54 to 69

-------------------------------------------------------------------------------
-- Some C functions
-------------------------------------------------------------------------------

-- | @void *memcpy(void *dest, const void *src, size_t n);@
--
-- Note: this is safe foreign import
foreign import ccall "memcpy"
c_memcpy :: Ptr a -> Ptr a -> CSize -> IO (Ptr ())

-- | @void *memset(void *s, int c, size_t n);@
--
-- Note: for sure zeroing memory use @c_sodium_memzero@.
foreign import ccall "memset"
c_memset :: Ptr a -> CInt -> CSize -> IO (Ptr ())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That goes away

Comment on lines 98 to 107
foreign import capi unsafe "sodium.h crypto_hash_sha256" c_crypto_hash_sha256 :: SizedPtr CRYPTO_SHA256_BYTES -> Ptr CUChar -> CULLong -> IO CInt

-- | @int crypto_hash_sha256_init(crypto_hash_sha256_state *state);@
foreign import capi unsafe "sodium.h crypto_hash_sha256_init" c_crypto_hash_sha256_init :: SizedPtr CRYPTO_SHA256_STATE_SIZE -> IO CInt

-- | @int crypto_hash_sha256_update(crypto_hash_sha256_state *state, const unsigned char *in, unsigned long long inlen);@
foreign import capi unsafe "sodium.h crypto_hash_sha256_update" c_crypto_hash_sha256_update :: SizedPtr CRYPTO_SHA256_STATE_SIZE -> Ptr CUChar -> CULLong -> IO CInt

-- | @int crypto_hash_sha256_final(crypto_hash_sha256_state *state, unsigned char *out);@
foreign import capi unsafe "sodium.h crypto_hash_sha256_final" c_crypto_hash_sha256_final :: SizedPtr CRYPTO_SHA256_STATE_SIZE -> SizedPtr CRYPTO_SHA256_BYTES -> IO CInt
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the real C types instead of SizedPtr

Comment on lines 116 to 120
foreign import capi unsafe "sodium.h crypto_generichash_blake2b" c_crypto_generichash_blake2b
:: Ptr out -> CSize
-> Ptr CUChar -> CULLong
-> Ptr key -> CSize
-> IO CInt
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document each parameter and their relation.

-- | @int crypto_generichash_blake2b(unsigned char *out, size_t outlen, const unsigned char *in, unsigned long long inlen, const unsigned char *key, size_t keylen);@
--
-- <https://libsodium.gitbook.io/doc/hashing/generic_hashing>
foreign import capi unsafe "sodium.h crypto_generichash_blake2b" c_crypto_generichash_blake2b
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Out parameter, this is where the result of the hash will go
  2. Length of the result
  3. In parameter ,what you want to hash
  4. Length of what you want to hash
  5. Key
  6. Length of the key

@Kleidukos Kleidukos force-pushed the add-initial-bindings branch 2 times, most recently from 3e2d8e5 to 174d2d0 Compare January 11, 2022 19:31
@Kleidukos
Copy link
Member Author

So, I'm a little worried here because:

  • The only signing algorithm here is Ed25519ph
  • The RFC says about *ph variants:

The Ed25519ph and Ed448ph variants are prehashed. […] These variants SHOULD NOT be used.

  • The crypto state is based on crypto_hash_sha512_state
typedef struct crypto_sign_ed25519ph_state {
    crypto_hash_sha512_state hs;
} crypto_sign_ed25519ph_state;

/*
 * WARNING: Unless you absolutely need to use SHA512 for interoperability,
 * purposes, you might want to consider crypto_generichash() instead.
 * Unlike SHA512, crypto_generichash() is not vulnerable to length
 * extension attacks.
 */

@jmcardon Am I missing anything? Does that mean we should provide signing from another backend?

@Kleidukos Kleidukos changed the title Add initial bindings and code style Add initial bindings to hashing algorithms and code style Jan 11, 2022
@Kleidukos Kleidukos changed the title Add initial bindings to hashing algorithms and code style Add initial bindings to hashing algorithms, and add code style Jan 13, 2022
Comment on lines +7 to +10
#define SODIUM_VERSION_STRING "1.0.18"

#define SODIUM_LIBRARY_VERSION_MAJOR 10
#define SODIUM_LIBRARY_VERSION_MINOR 3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these correspond to each other?

-- <https://libsodium.gitbook.io/doc/usage>
--
-- @since 0.0.1.0
foreign import capi "sodium.h sodium_init" c_sodium_init :: IO CInt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we ensure that users of the library call this? it would be nice to make sure it's run when the program starts without having to remember to do anything.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmcardon and @kozross suggested a secureMain function just like other defaultMain functions that exist, where the sodium_init function is called. Something like

main :: IO ()
main = secureMain $ do
  rest of the function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that's the easiest way. You can (and indeed, should) also document that c_sodium_init must be called before any other functionality is used for those who don't want to use secureMain.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

-> IO CInt

-- | @int crypto_generichash_blake2b_update(crypto_generichash_blake2b_state *state, const unsigned char *in, unsigned long long inlen)@
foreign import capi unsafe "sodium.h crypto_generichash_blake2b_update" c_crypto_generichash_blake2b_update
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to tell if these should be safe or unsafe, it feels like for small inputs unsafe would be fine, but if someone mmaps a 4TB file and hashes it, this is going to be problematic. I'd thought about exposing both and then on the Haskell side choosing based on the data size. Whether the runtime of hashing algorithms has security concerns I have no idea...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking

Copy link
Contributor

@kozross kozross Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say this is a case of 'offer both options, explain why they're different' for sure. Automagical deciding could be done at a higher level, but even then, I'm not sure if it's a good idea.

Comment on lines +47 to +48
go outPtr arrPtr = traverse_
(\i -> peek @CUChar (plusPtr arrPtr i) >>= poke (plusPtr outPtr i)) [0..383]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'd rather generate those with c2hs, these are not here to stay. :)

instance Ix CSize where
range (CSize m, CSize n) = CSize <$> range (m, n)
unsafeIndex (CSize m, CSize n) (CSize i) = unsafeIndex (m, n) i
inRange (CSize m, CSize n) (CSize i) = inRange (m, n) i
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a useful thing to get added to base

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly surprised these don't exist already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants