possibly remove ByteArrayAccess for Context

[cdepillabout]

At work (@xc-jp), we were seeing some strange behavior with code using Context SHA256.

I wrote up an explanation of what we were seeing at the following gist:

https://gist.github.com/cdepillabout/0b98301dd747fa642de74ecca525d5e3

Basically, the size of a Context SHA256 is declared differently in the C code versus the Haskell code:

In C, it is 168 bytes: https://github.com/haskell-crypto/cryptonite/blob/65643a3beae620ec00ae6813af9a1b585bbc038a/cbits/cryptonite_sha256.h#L45

In Haskell, it is 192 bytes: https://github.com/haskell-crypto/cryptonite/blob/65643a3beae620ec00ae6813af9a1b585bbc038a/Crypto/Hash/SHA256.hs#L29

The size declared in the Haskell code was changed from the C code in this commit:

https://github.com/vincenthz/hs-cryptohash/commit/ad14d1cb3afffebe5ebe6438c2e388233651a076

This is okay because of how cryptonite works. The memory is allocated from the Haskell side (so 192 bytes are allocated while only 168 bytes are required).

However, the C-side calls memset 0 on the memory, but only initializes the first 168 bytes to zero.

This causes strange stuff to happen if you call the hashUpdates function and pass it a [Context SHA256] as the second argument (which is okay to do because Context has a ByteArrayAccess instance).

https://github.com/haskell-crypto/cryptonite/blob/65643a3beae620ec00ae6813af9a1b585bbc038a/Crypto/Hash.hs#L75-L86

---

I sent an email to @ocheron and @vincenthz trying to argue that the above is a bug, and the entire 192 bytes allocated for the Context SHA256 should get memset to 0.

@ocheron sent a nice reply saying that he doesn't think it is a bug, but just a bad use of the cryptonite API. He is saying that you don't get any referential transparency when working with the ByteArrayAccess instance for Context SHA256. He says that the exact sequence of bytes that are used will also vary for other reasons like architecture endianness, and that there is simply no referential transparency at this level, so attempts to minimize issues will make them harder to notice and debug.

[cdepillabout]

One potential solution to this problem is to remove the ByteArrayAccess instance for Context in cryptonite.

This would make sure that it is not possible to pass a Context to hashUpdates.

I was worried that some reverse dependencies of cryptonite would be using this ByteArrayAccess instance for Context, so I modified cryptonite to remove the ByteArrayAccess instance for Context and tried compiling all the reverse dependencies of cryptonite that are buildable in nixpkgs:

https://github.com/cdepillabout/nix-reverse-deps-of-haskell-package/blob/master/default.nix

Everything built cleanly (which means that nothing was using the ByteArrayAccess instance for Context), except for cryptohash.

It makes sense that cryptohash would be using this instance. I'm not sure what to do about this. Maybe some sort of unsafeWithByteArray function could be exported to be used in cryptohash.

---

The next best solution would probably be to just add some Haddocks to the ByteArrayAccess instance for Context warning against using it with hashUpdates.

[ocheron]

Thanks for the detailed report. One thing to note is that this kind of instance allowing to read internals is not only on the hash context but other parts of the library too.

[ocheron]

d494081 adds a note in the Haddock documentation for the 4 types having this.