haskell-crypto / cryptonite

lowlevel set of cryptographic primitives for haskell
Other
226 stars 139 forks source link

ChaChaPoly1305 should allow HMAC-then-decrypt #327

Closed voidus closed 1 year ago

voidus commented 4 years ago

Currently, the only way to decrypt ChaChaPoly1305-encrypted data is:

  1. ChachaPoly1305.initialize
  2. ChaChaPoly1305.appendAAD
  3. ChaChaPoly1305.finalizeAAD
  4. ChaChaPoly1305.decrypt
  5. ChaChaPoly1305.finalize, which will yield the Auth tag that can be compared.

This isn't ideal, we want to compare the tag before operating on the data. This is very inconvenient with the current implementation:

  let auth = undefined
    ciphertext = undefined
    key = undefined
    nonce = undefined

  calculatedAuth <-
    let rootState = ChaCha.initialize 20 key nonce
        (polyKey, _) = ChaCha.generate rootState 64
        pad16 :: Word64 -> Bytes
        pad16 n
          | modLen == 0 = ByteArray.empty
          | otherwise = ByteArray.replicate (16 - modLen) 0
          where
            modLen = fromIntegral (n `mod` 16)
     in throwCryptoErrorIO $
          Poly1305.initialize (ByteArray.take 32 polyKey :: ScrubbedBytes)
            <&> flip Poly1305.update ciphertext
            <&> flip
              Poly1305.updates
              [ pad16 (fromIntegral $ ByteArray.length ciphertext),
                either (error "meh") id $
                  ByteArrayPack.fill
                    16
                    ( ByteArrayPack.putStorable (toLE @Word64 0)
                        >> ByteArrayPack.putStorable (toLE @Word64 $ fromIntegral $ ByteArray.length ciphertext)
                    )
              ]
            <&> Poly1305.finalize

  when (auth /= calculatedAuth) $
    fail "Invalid HMAC"

And this doesn't even use AAD, not to mention that it leaks quite a bit of the implementation.

  1. Given that the state is already handled explicitly, could we add a function like authenticate :: ByteArray ba => ba -> State -> State, which updates the Poly1305-State but doesn't decrypt the data?
  2. I understand that this library tries to be low-level, but how do you feel about an additional function
decryptAtOnce
  :: (ByteArrayAcces key, ByteArray nonce, ByteArrayAccess aad, ByteArray ciphertext, ByteArray plaintext)
  => key
  -> nonce
  -> aad
  -> ciphertext
  -> (CryptoError | MACInvalid | plaintext)

This with a similar function for encryption would make things more usable, and it would encapsulate the MAC verification in the case where two passes over the data are acceptable, i.e. we can MAC-then-decrypt.

If you like the idea, I could give it a try.

ocheron commented 4 years ago

For block ciphers we have helper functions aeadSimpleEncrypt and aeadSimpleDecrypt. No problem adding similar functions for ChaChaPoly1305.

When not incremental, a decryption function which is lazy enough should give the short-circuit evaluation you want.