search

mcginty / snow

A Rust implementation of the Noise Protocol Framework
Apache License 2.0
884 stars 120 forks source link

Fix nonce incrementing in stateful transport to match the specification #152

Closed complexspaces closed 1 year ago

complexspaces commented 1 year ago

This PR fixes https://github.com/mcginty/snow/issues/150 by correcting the stateful encrypt/decrypt functions to never use the reserved value of u64::MAX, which additionally guarantees a lack of wraparound issues.

In addition, a unit test was added to ensure this behavior does not regress by mistake.

Fixes https://github.com/mcginty/snow/issues/150

mcginty commented 1 year ago

This looks good, and given the wording of that nonce being reserved, I'm wondering if the same restriction should be placed on StatelessCipherState as it would also be using a "special" nonce, regardless of it being exhausted or not. https://github.com/mcginty/snow/blob/b55a713b2833c95bf8a6b40dac1af1905426d028/src/cipherstate.rs#L123-L153

complexspaces commented 1 year ago

... I'm wondering if the same restriction should be placed on StatelessCipherState as it would also be using a "special" nonce

That's a valid question, and I almost had implemented that too. The specification is unclear if "reserved" means "reserved for Noise" or "reserved for a protocol to use for certain things." If its the first interpretation, we should, but in the second we might prevent an otherwise valid use case.

mcginty commented 1 year ago

Given the wording for 11.4. Out-of-order transport messages, I think we should cautiously treat it as abiding by the same rules as the in-order transports, if you don't mind adding that to this PR.

I asked for clarification and if I hear anything to the contrary we can update.

mcginty commented 1 year ago

Ah yes, MAXNONCE is used in the REKEY(k) function, so we should consider that reserved for all cases where the nonce is used.

complexspaces commented 1 year ago

Oh great catch! I agree we need the check everywhere then. I'll update this PR shortly.

mcginty commented 1 year ago

Thanks to Trevor for pointing it out :).

complexspaces commented 1 year ago

I have pushed the changes that now include validations for the stateless cipher function as well.

mcginty commented 1 year ago

Thanks, this looks great.

The only thought I had is that Exhausted is probably not a great name any more (maybe InvalidNonce with rustdocs to explain the scenario), but that's actually better for me to change separately, as I'd like to get this merged into the 0.9 branch for a correctness patch release.