Misuse issue in (X)ChaCha20 and (X)ChaCha20-Poly1305 APIs

[brycx]

During a review of parts of the codebase, I've come across a rather unfortunate misuse possibility in ChaCha20, which also affects ChaCha20-Poly1305, but much more limited therein.

(X)ChaCha20

In orion::hazardous::stream::chacha20::encrypt() we allow the user to specify the initial_counter, that the ChaCha20 state should be initialized with (also known as the block counter). If this initial_counter is too large, meaning would overflow when adding 1, then we return an error.

orion/src/hazardous/stream/chacha20.rs

Line 303 in 1ee15eb

match initial_counter.checked_add(ctr as u32) {

The problem is, that before this error is returned, we have already copied the plaintext to the output destination dst_out (in order to later XOR it with the keystream).

orion/src/hazardous/stream/chacha20.rs

Line 347 in 1ee15eb

dst_out[..plaintext.len()].copy_from_slice(plaintext);

If the initial_counter.checked_add(1) returns Err() the plaintext bytes that were supposed to be XORed with the next keystream bytes, remain in plaintext in the output buffer.

    let k = SecretKey::generate();
    let n = Nonce::from_slice(&[0u8; 12]).unwrap();
    let ic: u32 = u32::MAX - 1;

    let text = [b'x'; 128 + 4];
    let mut dst_out = [0u8; 128 + 4];

    let err = encrypt(&k, &n, ic, &text, &mut dst_out).unwrap_err();

    assert_eq!(&[b'x'; 4], &dst_out[dst_out.len() - 4..]);

While we do return an error, and a user should not use the output of encrypt() if it returns an error, they might reasonably expect that dst_out not contain a copy of some plaintext bytes.

This issue also applies to XChaCha20, as it uses chacha20::encrypt().

(X)ChaCha20-Poly1305

The impact on (X)ChaCha20-Poly1305 is more limited, as the user cannot specify the initial_counter. However, the plaintext is encrypted with an initial block counter of 1, because the first is used for the Poly1305 key generation. Thus, plaintext bytes may be left in the output buffer if the length of the plaintext is one block less than the maximum (max is 2^32-1 * 64 bytes), which is very unlikely to happen in real-world use.

For the streaming implementation, one block less than the required amount for (X)ChaCha20-Poly1305, is required to hit this, as the initial counter there used internally starts with 2, for plaintext bytes.

Solution

The obvious solution here seems to be, to only copy anything to the output buffer that is ciphertext. Thus, we change the underlying XOR macro usage in ChaCha20 and can take the same approach when processing plaintext bytes in AEAD. In AEAD, we could maybe even get a bit of speedup as we could then encrypt and authenticate the ciphertext block individually, instead of making two passes over the plaintext and ciphertext bytes (first encrypt, then authenticate).

We could also discuss removing the option to set initial_counter for (X)ChaCha20, in a future major release, as there seems to be no real use for it.

cc @vlmutolo

[vlmutolo]

A third option would be to check all the error conditions before any output is written. But I think you're right that we shouldn't ever write plaintext to the output buffer, even if we immediately overwrite it.

One particularly bad situation is if the user sets up the output buffer to be memory-mapped to a file. In that case, there's a chance the plaintext is written to disk and is then far more observable.

[brycx]

Just a note that the removal of initial_counter in the public API, wouldn't fix the issue AEADs.

One particularly bad situation is if the user sets up the output buffer to be memory-mapped to a file. In that case, there's a chance the plaintext is written to disk and is then far more observable.

Very good point! We'll use the proposed approach instead then.