`chacha20`: Can't use full keystream of `ChaCha20` IETF variant

[nstilt1]

Off by one error: remaining_blocks() restricts the maximum number of blocks to u32::MAX when 0 is a valid block, raising the maximum number of blocks to u32::MAX + 1 or 1 << 32. This test passed, proving that there are 64 unusable bytes in the keystream:

    macro_rules! impl_chacha20_potential_counter_issue {
        ($name:ident, $num_blocks:literal) => {
            #[test]
            fn $name() {
                let mut cipher = ChaCha20::new(&KEY.into(), &IV.into());
                let mut first_4_blocks = [0u8; 256];
                assert_eq!(cipher.current_pos::<u64>(), 0);
                cipher.apply_keystream(&mut first_4_blocks);

                let mut buf_1 = [0u8; $num_blocks * 64];
                let mut buf_2 = [0u8; $num_blocks * 64 + 1];

                // seek to end of keystream
                let pos = (1 << 32) * 64 - $num_blocks * 64 - 64;
                cipher.try_seek(pos).unwrap();
                assert_eq!(cipher.current_pos::<u64>(), pos);

                // overshoot keystream length
                let applied_keystream = cipher.try_apply_keystream(&mut buf_2);
                assert_eq!(applied_keystream.is_err(), true);

                // exhaust keystream
                cipher.apply_keystream(&mut buf_1);

                // seek to beginning and check if the first block is the same as before
                cipher.seek(0);
                assert_eq!(cipher.current_pos::<u64>(), 0);
                cipher.apply_keystream(&mut first_4_blocks);

                // if this assert fails, exhausting the keystream increments
                // state[13], resulting in a different keystream when it
                // should be the same
                assert_eq!(first_4_blocks, [0u8; 256]);
            }
        };
    }

Because this test passes (specifically assert_eq!(applied_keystream.is_err()) and the pos value and size of the buffers), it proves that the seek_pos has to be 64 bytes less than expected to reach the end of the supposed keystream.

I ran into this error when working the counter, but it is easily fixable and has already been fixed in the PR

[newpavlov]

It think it's a duplicate of RustCrypto/traits#1808, no?

We have either to expand the wrapped cipher state with a special flag to distinguish between the initial cipher state and the wrapped one, or do not allow users to generate the last byte or block of the cipher keystream (unless they explicitly use unchecked methods). Personally, I prefer the latter option.

[nstilt1]

Hmm... so, I can confirm with a modified version of this test and remaining_blocks() that I can apply to a [0u8; 64] but not a [0u8; 65] when seeking to the true last block. If what you say is true, then that means that I would not need to seek to 0 to continue using the cipher once the keystream is supposedly exhausted.

Let me write a test to confirm that I don't need to seek to 0, and if it works, then I will revert my code: remaining_blocks() and all of the inner() methods and soft.rs

[nstilt1]

Closing as "can't fix without breaking crypto laws". That passing test says it all. Sure, it could be fixed with a is_exhausted or has_wrapped part of the state, but that would add one more thing to keep track of

[nstilt1]

@newpavlov could this be fixed by storing pos | (1u8 << 7) or pos + 1 as the first block's pos? I think the first option only works if BlockSize::U8 < 128, and the second option only works if BlockSize::U8 < 255, but I asked copilot for a list of stream ciphers with high block sizes, and ChaCha20 had the highest block size of 64 on the list. To accomplish this, we'd probably need to extract the pos as pos % BlockSize::U8, and there's no need to worry about it being less efficient than a bitwise and operation because the compiler will optimize it to a bitwise and operation if BlockSize::U8 is a power of 2.

[newpavlov]

We could do it, but it would complicate both the buffer and the wrapper implementations. I think it should be sufficient for the checked versions to disallow consumption of the last keystream byte and for rare cases when you do not need the wrapping check we can introduce unchecked methods.