kazu-yamamoto / crypton Goto Github PK

When using haskell-language-server to check a Haskell module in my project which transitively depends on both crypton and cryptonite, I received the following error. (This detail is shown when -d (debug) flag is turned on in haskell-language-server). Howerver, the project works fine with both cabal build and cabal repl.

By looking at the source code of crypton, I noticed that in cbits/blake2/sse/blake2s.c file, there is a non-static function named blake2s. This is a possible cause of symbol conflicts.

GHC runtime linker: fatal error: I found a duplicate definition for symbol
   blake2s
whilst processing object file
   /nix/store/pjy8nwfnn2cpzfbwlzc5ib7vj05w92pl-crypton-lib-crypton-0.32/lib/x86_64-linux-ghc-9.4.5/crypton-0.32-EGpwvAbNtQg1OZ3fW00GIA/HScrypton-0.32-EGpwvAbNtQg1OZ3fW00GIA.o
The symbol was previously defined in
   /nix/store/jn06hq2n07xwd2zalfjmnc8cwri3xxxf-cryptonite-lib-cryptonite-0.30/lib/x86_64-linux-ghc-9.4.5/cryptonite-0.30-5urQnFo6cSD563Pl2BMv1A/HScryptonite-0.30-5urQnFo6cSD563Pl2BMv1A.o
This could be caused by:
   * Loading two different object files which export the same symbol
   * Specifying the same object file twice on the GHCi command line
   * An incorrect `package.conf' entry, causing some object to be
     loaded twice.

Suggested fix: rename all non-static C symbols in crypton to be prefixed with crypton_.

    blowfish64
      KAT
        ECB
          E0:                                OK
          D0:                                OK
          E1:                                OK
          D1:                                OK
          E2:                                OK
          D2:                                OK
          E3:                                Test suite test-crypton: FAIL
Test suite logged to: dist/test/crypton-0.32-test-crypton.log
0 of 1 test suites (0 of 1 test cases) passed.

Full build log: crypton-0.32.log crypton-0.33.log

Greetings! I'm wondering if there's anything I can do about this error:

[138 of 138] Compiling Crypto.Tutorial  ( Crypto/Tutorial.hs, dist/build/Crypto/Tutorial.o, dist/build/Crypto/Tutorial.dyn_o )

In file included from cbits/aes/x86ni.h:38:0: error:
    0,
                     from cbits/aes/gf.c:35:

/usr/lib/gcc/x86_64-redhat-linux/4.8.5/include/wmmintrin.h:34:3: error:
     error: #error "AES/PCLMUL instructions not enabled"
     # error "AES/PCLMUL instructions not enabled"
       ^
   |
34 | # error "AES/PCLMUL instructions not enabled"
   |   ^

In file included from cbits/aes/x86ni.h:39:0: error:
    0,
                     from cbits/aes/gf.c:35:

/usr/lib/gcc/x86_64-redhat-linux/4.8.5/include/tmmintrin.h:31:3: error:
     error: #error "SSSE3 instruction set not enabled"
     # error "SSSE3 instruction set not enabled"
       ^
   |
31 | # error "SSSE3 instruction set not enabled"
   |   ^
`gcc' failed in phase `C Compiler'. (Exit code: 1)
cabal: Failed to build crypton-0.34 (which is required by test:test_messages

The only reason it can fail is if it receives key not equal to 32 bytes.

Given that this is a very common size, we could add some newtype that cannot accidentally be constructed and pass it here (e.g. with additional function Crypto.Cipher.XSalsa.generate32, which is commonly used for NaCl crypto_box construction).

newtype Bytes32 a = Bytes32 a -- where a would be an instance of ByteArrayAccess

It would eliminate a lot of error handling code in all consumers for errors that can never happen (that currently can only be removed with partial pattern matches on CryptoFailable / using errors)

Happy to make a PR

I wonder whether the README needs update. E.g. it claims

There might be other stuff that isn't up to date anymore for the fork...

Would it be possible to add totp (and maybe hotp) algorithms for time-based one-time password approach?

https://www.rfc-editor.org/rfc/rfc6238

or are these too specific to be in a general crypto sink package?

My question at yesodweb/wai#931 (comment) was likely posted in the wrong place and is better raised here.

If the repository for package cryptonite-conduit is forked here, I'll raise a pull request to help complete its transition from cryptonite to crypton.

The motivation for my request is that the Pantry project depends on cryptonite-conduit, and the Stack project depends on pantry and on http-download (which, in turn, depends on cryptonite-conduit). My end objective is to move Stack/Pantry from cryptonite onto crypton.

Please correct me if I'm wrong or this is not a real issue 🙇

According to this section, we know that

• The Haskell 2010 Report specifies
  • (passing unpinned ByteArray#s directly to native code) can not be done safely for safe calls since the array might be moved by the garbage collector in the middle of the call
  • cannot pass heap-allocated references to unsafe FFI

which implies that we only pass pinned object to safe FFI currently. And about safe

• The Haskell 2010 Report specifies
  • safe FFI calls must allow foreign calls to safely call into Haskell code

which is not the use cases in Crypton, since we only use FFI to run CPU instructions and never pass callback to or run Haskell calls in cbits (at least for Cipher & BlockCipher as far as I know).

Does that mean we're safe to mark most of FFI as unsafe and hope it'll gain more performance?

One potential downside and discussion was documented here:

when you call an unsafe function, and a global garbage collection is triggered in another thread (...), the garbage collector cannot proceed

a foreign call to a function that is guaranteed to take a short time, and does not call back into Haskell can be marked unsafe. This works both for the single-threaded and the multi-threaded runtime.

It would be great to hear your opinion or suggestions :)

Sometimes you want to save or load a key. If your key is an AES type from Crypton.Cipher.AES then there's no clear way to get the bytes out for serialization.

This could be addressed by giving the AES types a Serialize instead of a ByteArrayAccess instance.

The motivation is to mitigate any compromise of system entropy source (that can be not even an entropy but some substitute, in case DoNotUseEntropy flag is used).

This line: https://github.com/kazu-yamamoto/crypton/blob/master/Crypto/Random.hs#L52

Replace this:

seedNew = Seed `fmap` getRandomBytes seedLength

with something like:

seedNew = Seed . take seedLength . BA.convert . (hash :: ByteString -> Digest SHA512) `fmap` getRandomBytes 64

It assumes that seedLength is less than 64 bytes, it's 40 in this file, but we could of course use some more "clever" approach and define something like getHashedRandomBytes that would get "random" bytes in 32/64 byte chunks, hash (256/512) and append them, and throw away what was unused. Given that more than 64 bytes almost never needed, it seems overkill.

cc @kazu-yamamoto - that would certainly be a good incentive to switch from cryptonite :)))

We will be adopting it for both remote access protocol and for double ratchet, it's here for now:

https://github.com/simplex-chat/simplexmq/tree/xrcp/cbits
https://github.com/simplex-chat/simplexmq/blob/xrcp/src/Simplex/Messaging/Crypto/SNTRUP761/Bindings.hs

The source code for sntrup761 KEM and for hybrid shared secret construction are from referenced Internet drafts, so we could drop them into crypton if you want. All we did was Haskell bindings and newtypes. That would also help addressing the hack with DRG state, as crypton doesn't make it available, so we have to create closure with atomic access to DRG. If it's in crypton it could use some more efficient locking mechanism (although it probably won't make a huge difference).

Currently there are only sign / verify functions that compute sha512 hash of the passed data internally. The problem with that is that it doesn't allow signing/verifying lazy bytestrings.

We can split implementation of crypton_ed25519_sign / crypton_ed25519_sign_open (and also crypton_decaf_ed448_sign / crypton_ed448_sign_open) to two parts to allow passing digests as well, and expose them as singDigest / verifyDigest functions in both modules that accept sha 512 hashes (like in other modules).

@kazu-yamamoto We will do the PR if it's ok.

It would be great if this library is expanded to support NIST PQC KEM and SIgnature algorithms: Kyber, Dilithium, Falcon, SPHINCS+.

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:

• haskell-foundation/foundation#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.

This was previously filed on cryptonite, but rejected. See haskell-crypto#361 for details.

Is crypton open to adding hardware acceleration support?

I'm not sure how to sort all of this out, so I'll just report from the build log (this is picking up tls 1.7 -- I'll put in a version bound for now to prevent it):
ghc 9.2:

Failed to build connection-0.3.1.
cabal: Failed to build connection-0.3.1 (which is required by exe:typst-hs
from typst-0.1.0.0). See the build log above for details.

Build log (
/home/runner/.cabal/logs/ghc-9.2.8/connection-0.3.1-09428a83ace547de7d30ec2ee51f934daa14d24b4b27780ca67145a388293775.log
):
Warning: connection.cabal:26:36: version operators used. To use version
operators the package needs to specify at least 'cabal-version: >= 1.8'.
Configuring connection-0.3.1...
Preprocessing library for connection-0.3.1..
Building library for connection-0.3.1..
[1 of 2] Compiling Network.Connection.Types ( Network/Connection/Types.hs, dist/build/Network/Connection/Types.o, dist/build/Network/Connection/Types.dyn_o )
[2 of 2] Compiling Network.Connection ( Network/Connection.hs, dist/build/Network/Connection.o, dist/build/Network/Connection.dyn_o )

Network/Connection.hs:122:43: error:
    • Couldn't match expected type ‘crypton-x509-store-1.6.9:Data.X509.CertificateStore.CertificateStore’
                  with actual type ‘Data.X509.CertificateStore.CertificateStore’
      NB: ‘crypton-x509-store-1.6.9:Data.X509.CertificateStore.CertificateStore’
            is defined in ‘Data.X509.CertificateStore’
                in package ‘crypton-x509-store-1.6.9’
          ‘Data.X509.CertificateStore.CertificateStore’
            is defined in ‘Data.X509.CertificateStore’
                in package ‘x509-store-1.6.9’
    • In the ‘sharedCAStore’ field of a record
      In the ‘clientShared’ field of a record
      In the expression:
        (TLS.defaultParamsClient (fst cid) portString)
          {TLS.clientSupported = def
                                   {TLS.supportedCiphers = TLS.ciphersuite_default},
           TLS.clientShared = def
                                {TLS.sharedCAStore = globalCertificateStore cg,
                                 TLS.sharedValidationCache = validationCache}}
    |
122 |             { TLS.sharedCAStore         = globalCertificateStore cg
    |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Process completed with exit code 1.

Here's the definition of IV c (no NFData instance for it)

and we expose only the type without constructor

These facts cause problem in my usage. I'll generate multiple IV cs by applying ivAdd multiple times (like scan initial IV with ivAdd, creates data in shape iv0 'ivAdd' n0 'ivAdd' n1 'ivAdd' n2 ... if no seq in each step) but only use them when certain condition is met, which is very likely to NOT use them at all most of the time.

Then I got visible thunk usage in heap even though I do run seq on IV c. Currently I use workaround, let !_ = iv == iv, to seq them.

Here are some ideas in my mind but it's hard to tell which one works best for this module:

• add strict annotation to IV constructor (behaviour changed on seq)
• create instance NFData for IV c (could evaluate too deep)
• add function to seq bytearray in IV c (which is rare solution in my experience)

Any new idea or thoughts are welcome :)

If you have a package that transitively depends on both crypton and cryptonite (which we just saw due to an http-client-tls update), you get link errors (e.g. multiple definition of 'cryptonite_sha512t_update').

Botan binding work: haskellfoundation/tech-proposals#57

If this really happens, I will try to migrate tls from crypton to botan and stop maintaining crypton.

Currently we can freely coerce between the Digests of different hash algorithms. This can lead to safety issues, and to prevent it, we should make Digest have a type role Digest nominal

Example issue:

> import Data.Coerce
> x :: Digest SHA256; x = undefined
> y = coerce @(Digest SHA256) @(Digest Blake2b_256) x
> :t y
y :: Digest Blake2b_256