[WIP] 64-bit counters for legacy and XChaCha variants, fix looping counter regressions#399
[WIP] 64-bit counters for legacy and XChaCha variants, fix looping counter regressions#399jpdoyle wants to merge 6 commits intoRustCrypto:masterfrom
Conversation
|
This PR could remove the warning in |
dhardy
left a comment
dhardy
left a comment
There was a problem hiding this comment.
I didn't review the SSE2 code (arguably the most complex part of this PR) but can if it's helpful.
I'd like to see 64-bit counter support merged for usage in rand.
| self.0.state[12] = self.0.state[12].wrapping_add(1); | ||
| self.0.counter = self.0.counter.saturating_add(1); | ||
| if let Some(ctr) = self.0.state[12].checked_add(1) { | ||
| self.0.state[12] = ctr; | ||
| } else if V::COUNTER_SIZE > 1 { | ||
| if let Some(ctr) = self.0.state[13].checked_add(1) { | ||
| self.0.state[12] = 0; | ||
| self.0.state[13] = ctr; | ||
| } | ||
| } |
There was a problem hiding this comment.
Old behaviour when COUNTER_SIZE=1 was to wrap. New behaviour is to saturate, but I don't think this is justified?
This also saturates when COUNTER_SIZE=2. Arguably it should wrap just in case someone set the block position to something just below the end.
Hint: it may be easier to convert to u64, increment and convert back, or what you did a few lines below.
| /// Current counter position | ||
| counter: u64, |
There was a problem hiding this comment.
We don't need to add additional state to ChaChaCore (at least not for these variants).
This is just for testing?
| type Counter = u32; | ||
| type Counter = u64; |
There was a problem hiding this comment.
This should depend on the variant.
| let le_pos = self.counter.to_le_bytes(); | ||
| let max_val = V::MAX_USABLE_COUNTER; | ||
| if self.counter <= max_val { | ||
| for i in 0..V::COUNTER_SIZE { | ||
| debug_assert_eq!( | ||
| self.state[12 + i], | ||
| u32::from_le_bytes(<_>::try_from(&le_pos[4 * i..(4 * (i + 1))]).unwrap()) | ||
| ); | ||
| } | ||
| } | ||
| self.counter |
There was a problem hiding this comment.
Assuming COUNTER_SIZE <= 2, it's much simpler to do this:
let mut counter = self.state[12];
if V::COUNTER_SIZE > 1 {
counter |= self.state[13] << 32;
}
| } else { | ||
| let ctr = _mm_cvtsi128_si64(backend.v[3]) as u64; | ||
|
|
||
| state[12] = (ctr&(u32::MAX as u64)) as u32; |
There was a problem hiding this comment.
I think the and operation here is unnecessary, given that the next line works
|
If you want to see some code that works for all backends, I have a working branch with code for all of the backends. Just search for Edit: my current PR contains all of the necessary 64-bit counter code. You could just look at the diff and see what all needs to be changed. |
|
Closing in favor of #439 |
4 participants
This is still WIP. So far it is basically a PoC of what is needed to perform these changes.
The overall checklist before merging is:
IetfChaCha20traitsso that there is an explicit representation of a looped/saturated counter. Currently the implementation of StreamCipherCoreWrapper does not allow me to directly implement a cipher that stops once it would loop; in the current implementation I get around this by effectively using a 64-bit counter for the 32-bit counter mode, which allows me to distinguish between a just-initialized cipher (block_pos = 0, pos = 64) and a looped cipher (block_pos = 2**32, pos = 64). Note that this does not currently allow correct behavior at the end of a 64-bit-counter keystream.Ietfvariant to use a 32-bit counter again, fixing thebad_overflow_check{6,7}seek-focused testsSeekNum::from_block_byteintraitsto match the looped counter representation, thereby fixing cipher: Seeking near the end of the keystream causestry_current_posto return an error traits#1808. (NOTE: the test in that issue is present in this PR, and succeeds; that is because I use a 64-bit overall counter even in 32-bitIetfmode)This would close #334, #391.