cipher: Seeking near the end of the keystream causes `try_current_pos` to return an error

[jpdoyle]

The following test fails on the last line:

    #[test]
    fn test_current_pos() {
        let offset: u64 = 256u64 << 30;

        let mut cipher = chacha20::ChaCha20::new(&Default::default(), &Default::default());
        cipher
            .try_seek(offset - 64)
            .expect("Couldn't seek to nearly 256GB");
        assert_eq!(cipher.try_current_pos::<u64>().unwrap(),
            offset - ((1<<6))
        );
        cipher
            .try_seek(offset - 63)
            .expect("Couldn't seek to nearly 256GB");
        cipher.try_current_pos::<u64>().unwrap();
    }

The root cause appears to be a combination of the incorrect looping counter behavior highlighted in https://github.com/RustCrypto/stream-ciphers/issues/391 and from the behavior of try_seek in StreamCipherCoreWrapper. try_seek sets the block position to (((256u64 << 30)-63) / 64) as u32, which is u32::MAX, and then calls write_keystream_block, which increments the counter, causing it to loop back to 0. Then the try_current_pos calls get_block_pos(), which returns 0 -- see https://github.com/RustCrypto/traits/blob/67131afeab8f235bb1698f94585bb84c8afd9716/cipher/src/stream/wrapper.rs#L176-L202

SeekNum::get_block_byte is then called with a value of 0 for the block parameter, and the checked subtraction on this line returns None.

[newpavlov]

As explained here, it's an artifact of how we generically detect wrapping of keystream. We could change the code to properly detect wrapping around 0, but it has to be done in the cipher crate.

[newpavlov]

The reason behind this issue is the following.

For efficiency sake we want to be lazy with keystream generation. It means that during stream cipher initialization we do not generate any keystream blocks and the buffer gets initialized into the "exhausted" state (we do it by setting cursor position to block size, which results in number of unread bytes equal to zero). This also allows us to store cursor position in the first byte of the buffer instead of introducing a separate field, since it's guaranteed that after block generation we will consume at least one byte from it (otherwise we would leave the buffer in the "exhausted" state).

Now it leads to a problem when we have generated the last keystream block and haven't consumed all its bytes. The cipher core gets wrapped into its initial state, while we have some unread keystream bytes in the buffer. With a naive check of the remaining keystream bytes it would appear as if we have a plethora of remaining keystream blocks (the cipher core has wrapped around after all). To work around this we do not allow the buffered stream cipher to get to the last block (i.e. we raise an error when we pass the start boundary of the last block, not its end boundary). Unfortunately, the seek implementation did not account for this and allowed user to seek to the last block.

To properly handle this problem I think the best solution would be to add a special handling for the first keystream block. In other words, if the cipher core is on the first block and the read buffer is not empty, we calculate number of remaining keystream bytes as the number of bytes remaining in the read buffer. This would require to modify the StreamCipherCore::remaining_blocks method to return whether we are on the first block or not, but it should be a fairly simple change for the implementation crates.

[baloo]

This is one of (if not the last) big issue we want fixed for the next release. I've tried to fix with your description but I can't wrap my head around it.

[newpavlov]

I decided to keep the old behavior (i.e. forbid generation of the last keystream block) and to just fix the wrapper seeking implementation. Forbidding generation of the last keystream byte instead of the block results in a quite convoluted and annoying code.

In practice, users should extremely rarely (if ever) depend on generation of the last keystream block, so I think it's a reasonable trade off, especially considering that we have the unchecked methods as a potential escape hatch.