`chacha20` regression: Looping counters and 32-bit counter for xchacha

[jpdoyle]

It appears that all the tests added by my previous PRs #217 and #216 were removed. The behavior checked by those tests appears to have regressed. In particular, xchacha20 can no longer use the full 64-bit counter space and instead sets the upper 32 bits to 0, and the overflow handling allows for looping counters -- e.g.,

stream-ciphers/chacha20/src/backends/soft.rs

Line 31 in 44beace

self.0.state[12] = self.0.state[12].wrapping_add(1);

The commit which removed these tests, and which also appears to have introduced a regression on all this behavior, is the following commit by @newpavlov :

610e7685495c997df4232606439e1b12e59f4147 is the first bad commit
commit 610e7685495c997df4232606439e1b12e59f4147 (tag: salsa20-v0.10.0, tag: rabbit-v0.4.0, tag: hc-256-v0.5.0)
Author: Artyom Pavlov <newpavlov@gmail.com>
Date:   Thu Feb 10 08:27:06 2022 +0000

    Update crates to cipher v0.4 (#276)

[tarcieri]

64-bit counter support is tracked as #334.

Counter looping and the removal of tests is definitely quite unfortunate. Sorry to hear that happened. It seems like we should track it as a security vulnerability as it results in the reuse of the keystream.

[newpavlov]

The check is performed in the try_apply_keystream_partial while docs for apply_keystream_blocks and other methods explicitly warn:

WARNING: this method does not check number of remaining blocks!

The same distinction is present in the StreamCipher methods.

I understand that it's a somewhat brittle and misusable API, so it may be worth to tweak it in cipher v0.5.

[jpdoyle]

I'm not sure how it's getting skipped but if that overflow check was sufficient it should make this test pass:
https://github.com/jpdoyle/stream-ciphers/blob/e5d04a685a496b8fdc7dbcbeb691e24e1b4cd220/chacha20/tests/mod.rs#L293-L308
however on my machine I'm seeing:

---- overflow::bad_overflow_check4 stdout ----
thread 'overflow::bad_overflow_check4' panicked at chacha20/tests/mod.rs:303:9:
assertion `left == right` failed
  left: 0
 right: 274877906944
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

which indicates that encryption at just before the 256GB mark succeeds and increments the counter back to 0.

[jpdoyle]

technically I think that test may still fail based on that assertion if the counter doesn't return a special overflow value, but removing that assertion causes the subsequent expect_err(..) to fail

[newpavlov]

You hit an unfortunate corner-case of the implemented check. I don't remember the exact details, but it was more efficient to check whether user tries to generate the last keystream block instead of whether we consume the last byte in it. This test passes as expected:

#[test]
fn bad_overflow_check4() {
    let mut cipher = chacha20::ChaCha20::new(&Default::default(), &Default:>
    cipher
        .try_seek(OFFSET_256GB - 65)
        .expect("Couldn't seek to nearly 256GB");
    let mut data = [0u8; 1];
    cipher
        .try_apply_keystream(&mut data)
        .expect("Couldn't encrypt the last byte of 256GB");

    let mut data = [0u8; 1];
    cipher
        .try_apply_keystream(&mut data)
        .expect_err("Could encrypt past the last byte of 256GB");
}

It may be worth to return an error from try_seek if we jump to the last block.

[newpavlov]

I think we can close this issue? If you have proposals on how we can improve the wrapping detection, then it's better to discuss them in the traits repo.

[newpavlov]

Closing in favor of RustCrypto/traits#1808.