kizzycode / crypto_api_chachapoly Goto Github PK

The AeadCipher::seal API mutates the provided plaintext buffer to encrypt in-place, after performing various error checks. If the caller ignores the return value (and thus any errors), the plaintext will end up being used unencrypted. This is inherent in the design of the API, so it's a risk that the caller is clearly signing up to.

The AeadCipher::seal_to API provides an interface for interacting with an AEAD that ostensibly encrypts from one buffer into another, according to its documentation:

AEAD-seals plaintext into buf together with ad using key and nonce and returns the ciphertext length

Thus a caller could reasonably assume that if they ignored the return value, the output buffer would be unmodified, and the plaintext would not leak. However, the implementation of AeadCipher::seal_to for ChachaPolyIetf copies the plaintext into the output buffer and then calls AeadCipher::seal internally, breaking this assumption.

This could be addressed by copying the logic from seal into seal_to, so that all error checks are performed first, and the encryption operation is guaranteed to succeed at the time the plaintext is copied into the output buffer.

Originally raised by @daira here.

I can imagine someone misinterpreting new_sec_key's thinking that it fills the entire buffer with random bytes, but it actually only fills the first 32-bytes. To avoid accidental mistakes, I'd recommend changing this line to buf.len() == 32 instead of buf.len() < 32.

(BTW I compared the ChaCha20 and Poly1305 implementations to RFC 8439. I didn't look at the tests. This issue and #2 are the only problems I found).

In the AEAD construction, block counter = 0 is used to generate the poly1305 key, and encryption of data blocks starts with block counter = 1, so at most 2^32 - 1 blocks can be encrypted. However, the value of CHACHAPOLY_MAX is 2^32 blocks. I believe this means that the code will let you encrypt 2^32*64 bytes, and the last block will be XORed with the poly1305 key, because of n overflowing (at least in release builds).

In addition to fixing the constant I'd recommend adding a check (that works even in release builds) for n overflowing in the xor function.