96 bit nonces for chacha plus some cleanup

[calvinmetcalf]

this adds 2 commits

1. pulls some of the functions for chacha out into macros and some local variables out into constants
2. allows 96 bit nonces and limits itself to only use the first counter byte, boringssl also does this for 8 byte nonces.

[calvinmetcalf]

was basing it off statements like this on line 160 https://boringssl.googlesource.com/boringssl/+/master/crypto/cipher/e_chacha20poly1305.c basically while it does overflow it takes a counter which won't be more then 32 bits, so I interpreted it wrong. (I swore I saw a comment in the code where it didn't allow that, but at this point suspecting it was in a dream).

I can maybe put a flag into the change state to specify whether it should use the 2nd counter?

[ebfe]

That's one option. Another would be (and maybe this is preferable) to just mirror what boringssl does. Apparently they implemented chacha20poly1305 without explicit support for 12-byte nonces in Chacha20.

[calvinmetcalf]

@ebfe yes they do but that is because they are implementing an out of date version of the spec the idea is to add the ability to use longer nonces.

[DaGenix]

I do wonder if anyone is actually using ChaCha20 with an 8 byte nonce to encrypt 256GB chunks of data, but, it looks to me like the overhead of continuing to support that use case isn't too great. It would be nice to be fully conformant with both flavors of ChaCha20 - even more so than boringssl.

What is the motiation for converting the code to use additional macros? Does it help performance?

[ebfe]

crypto::chacha20poly1305 is currently implemented as in boringssl (8-byte
nonces but with the upper 4 byte of the counter always zero). If the plan is to
now follow the current and future rfc drafts it might make sense to mark
crypto::chacha20poly1305 as experimental (as the changes are incompatible
unless you fix the first 4-byte of your 12-byte nonces to zero).

[ebfe]

Another option of implementing this might be to provide a fn set_counter(lo: u32, hi: u32) which has the additional benefit of allowing random access to the key stream.

[calvinmetcalf]

@ebfe current chacha20poly1305 asserts that the nonce is 8 so accidentally using a 12 byte nonce with it won't be an issue

@DaGenix the macros are intended to promote readability by pulling the verbose stuff out of the body propper.

[ebfe]

On Fri, Jan 30, 2015 at 02:24:14AM -0800, Calvin Metcalf wrote:

@ebfe current chacha20poly1305 asserts that the nonce is 8 so
accidentally using a 12 byte nonce with it won't be an issue

That's the point. It's currently using 8-byte nonces. If the plan is to
update it to the current rfc draft that will be incompatible with what
we have now.

[calvinmetcalf]

One idea was to use nonce length as a switch for the 2 modes.

On Fri, Jan 30, 2015, 5:55 AM Michael Gehring [email protected]
wrote:

On Fri, Jan 30, 2015 at 02:24:14AM -0800, Calvin Metcalf wrote:

@ebfe current chacha20poly1305 asserts that the nonce is 8 so
accidentally using a 12 byte nonce with it won't be an issue

That's the point. It's currently using 8-byte nonces. If the plan is to
update it to the current rfc draft that will be incompatible with what
we have now.

—
Reply to this email directly or view it on GitHub
#230 (comment).

[DaGenix]

Frustratingly, I don't see any official position of the boringssl developers on their plans regarding their chacha20 implementation or chacha20poly1035. However, one of the authors of boringssl, Adam Langley, is also one of the authors of the chacha20poly1305 RFC. So, I will guess that they will at least update boring SSL to support that RFC. Whether they drop support for the current implementation is something that I can't guess at. Right now, it appears that OpenSSL doesn't support chacha20poly1305. I don't know about the other major crypto libraries, but I suspect they don't support it either. I'll make a guess that they won't support it until the IETF RFC is approved, so, I'm guessing that what they implement will look more like the current draft than it will the boringssl implementation. So, that leaves us with an algorithm without a clear spec or direction. This is the type of thing I've resisted taking in the past, but, we already have a few expirimental algorithms in the code base. Since these changes add a new feature to chacha20 that will be necessary to implement the current draft chacha20poly1305 spec without breaking any existing functionality and since rust-crypto is very much in a pre-1.0 state, I don't see any reason not to take these changes, so, I'll merge them.

@ebfe - I agree with you completely: I think it would be very nice to seeking in both chacha20 and salsa20. #228 is open to track cleaning up all of the APIs - I just added a comment to say that we need to figure out how to expose the ability to seek in salsa20 / chacha20 (or alternatively determine that we don't want to support it for some reason). Its also somewhat inconsistent - this PR adds support for 96 bit nonces which are necessary to implement a draft RFC but implementing that RFC will fundementally change the behavior of our algorithm, breaking compatibility with the only other existing implementation of that algorithm. I think what we need is a mechanism to mark algorithms as expirimental and require users to intentionally turn on that flag in order to use those features. Or, maybe do something like the Linux kernel and put algorithms without clear specs into a staging directory. Or, maybe we need to come up with a policy not to merge algorithms that don't have a clear spec. I'm not really sure - I'll open up an issue to discuss that point. Also, if I'm misinterpretting anything you've said, please let me know.

Thanks @ebfe & @calvinmetcalf!

[DaGenix]

ug - this had conflicts. @calvinmetcalf / @Yawning: I think I merged everything correctly, but if you two could take a peek and make sure I didn't mess anything up it would be greatly appreciated. Thanks!

[Yawning]

The merge looks ok from casual inspection however test_chacha20_256_tls_vectors_96_nonce() is missing a test annotation, so there's dead code warnings when the test suites are built. Opening a new pull request just to add #[test] is somewhat silly, but I will if that's prefered procedure (The tests themselves pass without issue once the annotation is added).

[Yawning]

https://gist.github.com/Yawning/95f1782e6846b856280a

The HChaCha20 routine should probably also use swizzle() for consistency, though that's more a long term maintainability thing, than correctness.