ChaCha RNG block/word position

[dhardy]

Currently this consists of:

// return 36 bit counter
pub fn get_word_pos(&self) -> u64;

// supports `u64` or `[u8; 5]`
// NOTE: [u8; 5] input uses a weird mixed-endian format
pub fn set_word_pos<W: Into<WordPosInput>>(&mut self, word_offset: W);

// supports `u32` or `[u8; 4]`
pub fn set_block_pos<B: Into<BlockPos>>(&mut self, block_pos: B);

pub fn get_block_pos(&self) -> u32;

First, we need 64-bit counters; thats #334 and #399.

Second, there are some weird Endian-conversions going on here. I would go into them except a more radical reform might be better.

Third, I don't think there's a good reason we need to support byte-arrays. Let users call to_le_bytes if they need that. (We don't technically need get_word_pos below either, but it's very little code and serves as an example.)

Proposal:

/// Get block position and sub-block index
///
/// Returns `(block_position, index)` where `index` is a value in the range `0..=16`.
pub fn get_block_pos(&self) -> (u64, u8) {
    self.core.core.0.state[12] as u64 | (self.core.core.0.state[13] as u64) << 32
}

/// Get the word position
///
/// This is a 68-bit index.
#[inline]
pub fn get_word_pos(&self) -> u128 {
    let (block, index) = self.get_block_pos();
    (block as u128) << 4 | ((index & 0b1111) as u128)
}

/// Set the block position
///
/// The sub-block index is reset to 0.
pub fn set_block_pos(&mut self, block: u64) {
    self.core.core.0.state[12] = block as u32;
    self.core.core.0.state[13] = (block >> 32) as u32;
    self.core.reset();
}

/// Set the word position
///
/// The low 68 bits are used; higher bits are ignored.
pub fn set_word_pos(&mut self, word: u128) {
    self.set_block_pos((word >> 4) as u64);
    let index = (word >> 4) as usize;
    self.core.generate_and_set(index);
}

I won't make a PR because I don't want to interfere with #399 or whatever @nstilt1 is up to.

[nstilt1]

The byte arrays are for anyone who wants to use the output of an RNG or a KDF to set these parameters. I don't personally use byte arrays to set anything in my personal code, but I'm sure that someone who is doing that would appreciate these. RNGs can use next_u32() but KDFs only output byte arrays. But the word arrays are more useful since you can specifically set state[12], state[13], state[14] and state[15]

[tarcieri]

I think [u8; 4] is fine if it matches (legacy) ChaCha20 test vectors

[dhardy]

That's an argument for set_block_pos supporting byte-arrays but not set_word_pos.

[nstilt1]

Good point. While a byte array could be convenient for that, it is still pretty arbitrary the way it works now

[dhardy]

I still see a few issues in the latest set_word_pos: (https://github.com/RustCrypto/stream-ciphers/blob/master/chacha20/src/rng.rs#L395)

• Docs mentions a "36-bit number" (should be 68)
• The index is 6-bit due to BUFFER_SIZE
• The block counter should be incremented (get_word_pos decrements it, which I'm guessing is correct)

[nstilt1]

Is this issue still active? If you need a reason for us to use <Into<X>>, one reason is that users can supply an "intermediate value" or resulting value in the process of converting a u64 into two u32s. This saves a hair of processing time, but more importantly it allows the expansion of 2 inputs into up to 16 different numerical inputs. One use case for this CSPRNG is using output_type (key/salt) for one input, purpose (MAC/KDF/ECDH/ECDSA/RSA) for another input, and an additional input for epoch (u32 or u64). Having this "input size splitting" handled by the RNG library allows other libraries to not need to manually test and confirm input conversions, allowing them to trust the RNG's implementation that

rng.set_block_pos(epoch);
rng.set_stream([output_type, purpose]);

does what they think that it's supposed to do without needing to write tests. I know I only used 3 inputs, but it could easily be expanded to have more parameters if desired. 16x 1B inputs is probably waaaay more than enough for most people, but it gives them options: options to split a u64 into two u32s, or 8 u8s. I think it gives tons of control that I haven't seen any other library provide in any language that I've used.

I will say though, the [u32; 2] input also allows for setting the upper 32 bits of the block pos more easily and more readily given the docs mentioning setting the upper 32 bits. It basically offers an explicit control for the upper 32 bits and can act as a way to extend the stream_id by 32 more bits by treating the block_pos as an extension of the stream_id, or even more if the desired output size was small, like for a key or salt.

[dhardy]

I think the current code is acceptable, so closing.

I'm not sure why set_stream bothers to call self.core.generate_and_set(self.core.index()); instead of just self.core.reset().

[nstilt1]

That's a good point. A similar operation is present in the current rand_chacha:

                if self.rng.index() != 64 {
                    let wp = self.get_word_pos();
                    self.set_word_pos(wp);
                }

The test_chacha_clone_streams test will fail if we change it because the test assumes that the current word_pos carries over, but that raises a new issue. Clone is not implemented for ChaChaXRng. It also raises the question of how this should be implemented. Should it carry over the previous word_pos or should it reset? I feel like that's not my decision to make.

[dhardy]

The most important thing is that this is documented clearly. It would be perfectly reasonable to reset the word_pos to 0 in set_stream since if someone really wants to they may call set_word_pos afterwards to restore a prior value. If we want to make this change from rand_chacha here I think it's reasonable (but would be less reasonable later since it is a silent change in behaviour affecting results).

This would make the test_chacha_clone_streams useless — it could be deleted. Alternatively it could be adapted to show that set_word_pos enables the same behaviour for anyone especially wanting it.

[nstilt1]

If I can get reset() to work in set_stream() I would be more than happy to replace it. But it's failing some tests. I modified the tests again but they are still failing here:

        let mut word_pos = rng.get_word_pos();
        assert_eq!(word_pos, 1); // passes

        rng.set_stream(1234567);
        assert_eq!(rng.get_block_pos(), 0); // passes
        assert_eq!(rng.get_word_pos(), 64); // passes, should not pass

        let _ = rng.next_u32();
        word_pos = rng.get_word_pos();
        assert_eq!(word_pos, 1); // fails, 65 != 1

        let test = rng.next_u32();
        rng.set_word_pos(word_pos);
        let expected = 3110319182;
        assert_eq!(test, expected); // passes
        assert_eq!(rng.next_u32(), expected); // fails

It looks like a classic 64 + 1 possibly resulting from generate() being called, incrementing block_pos by 4. But if we think it will be easier to just document the behavior it originally had, I'm fine with that. Or we can make set_stream() overwrite the block_pos and index to ensure that nothing naughty happens under the hood, but some users might perceive that as naughty behavior.

[dhardy]

@nstilt1 this is likely caused by the block code: internally it never sets the index to 0 but to N (the buffer size); fn index just returns this.

This is a leaky API; for correctness index should return something in the range 0..N (i.e. < N), but I'm not sure if that would have perf. issues. Maybe I'll add raw_index to the buffer for internal use and make index return 0 for anything >= N.

[nstilt1]

I found the issue with that test I had (stream_id_endianness). generate_and_set in the original set_stream was incrementing the block_pos by 4, so any test that assumes that it continues at word_pos = 1 and not word_pos = 1 + 64 would fail. This behavior is not expected based on the source code, so it probably wasn't expected looking at the docs.

Should set_stream() reset block_pos to 0, which also resets the index to 0, or should it just reset the index while keeping whatever block_pos it is already at? Keeping it as it was is probably infeasible, but it might break some existing dependents that don't set block_pos after setting stream_id. I also want to change get_block_pos() to attempt to return the actual current block_pos instead of the block_pos + 4 caused by generate.

[dhardy]

I suggest that set_stream sets the word position (block_pos and index) to 0. Just bear in mind that without rand_core#44 index() returns N after a reset but this can be considered 0.