Ctr128 switch counter u64 to u128

[koivunej]

Start fixing issue #12. Used the static data from issue to create a test case. Have done some randomized testing with original poc without any issues. At one point included wrap around tests to make sure aes-ctr matched output of openssl, see comment.

[koivunej]

Whoops, the initial PR uses u128::from_be_bytes which was stabilized in 1.32. There was a comment to use from_be_bytes when it stabilizes but apparently there's the requirement of supporting an older stable 1.27? If so I'll need to restore the unsafe and be_conv parts.

[tarcieri]

This might be acceptable, but we'd need to increase MSRV. Thanks for the PR!

Will wait to hear from @newpavlov on increasing MSRV.

[koivunej]

Well, might as well make this non-draft until MSRV decision has been made.

[koivunej]

I understand that the MSRV has been raised already, am I wrong?

[koivunej]

Force pushed a rebase on top of the latest master, zero conflicts, changed the first commit as conventional as the second.

[tarcieri]

I'd say bump it to 1.34.

[koivunej]

Sorry, I haven't made much progress with the test cases. I've been thinking about this more though, and my conclusion is that this is a nasty silent/sometimes breaking change for anyone trying to integrate a pre-PR and post-PR code together. From that I deduce that the previous counter behaviour must be chooseable (previous or "openssl compat" behaviour).

I don't have a proposal for such yet, but probably this it's ok to keep this open and to catch discussion on the topic.

[tarcieri]

@koivunej well, the good news is I've bumped MSRV all the way to 1.41. The bad news is I've done a 2018 edition upgrade and you'll need to rebase your PR.

Regarding this:

From that I deduce that the previous counter behaviour must be chooseable (previous or "openssl compat" behaviour).

It seems like the ctr crate needs to support several different flavors of CTR mode. This includes things like counter size and endianness as well, e.g. AES-GCM uses a 32-bit big endian counter, whereas AES-GCM-SIV uses a 32-bit little endian counter.

I think we need to split Ctr up into several different types, e.g. Ctr128BE, Ctr32BE, Ctr32LE.

[koivunej]

well, the good news is I've bumped MSRV all the way to 1.41. The bad news is I've done a 2018 edition upgrade and you'll need to rebase your PR.

Thanks for the ping, I think the rebasing is the easier part :)

Regarding this:

From that I deduce that the previous counter behaviour must be chooseable (previous or "openssl compat" behaviour).

It seems like the ctr crate needs to support several different flavors of CTR mode. This includes things like counter size and endianness as well, e.g. AES-GCM uses a 32-bit big endian counter, whereas AES-GCM-SIV uses a 32-bit little endian counter.

I don't know the specifics of those different flavors but something like this was my thinking as well. I think you mentioned this in a another issue already.

I think we need to split Ctr up into several different types, e.g. Ctr128BE, Ctr32BE, Ctr32LE.

My first idea for this was a generic "counter" parameter which would be a newtype (over u128, u32 or [u8; 16]) and selecting the endianess. Still, the test cases would need to be found.

Of course if someone else wants to take a crack at this and/or has the code already done, please don't take this PR as a lock of me insisting to be the one to complete this.

[tarcieri]

I think this will break crates like AES-SIV which (as an algorithm) rely on the existing implementation.

I think we need to factor apart some different Ctr types. We need the current one, but we can add additional ones.

[tarcieri]

@koivunej I tried rebasing this but unfortunately it seems the implementation has diverged quite a bit.

I think these changes ultimately make sense and our failure to match OpenSSL behavior represents a defect.

If you're interested in rebasing this, that'd be great.

[koivunej]

Thanks for pinging me :) Yeah, this got lost on my radar as I couldn't find the test vectors. With libp2p getting first class noise support this isn't anymore a factor in rust-ipfs. However I can try the rebase this week-ish and see for myself, having anything working will make the generic counter refactor easier as the next step.

I have been trying to think about the tests. Originally I used quickcheck'd tests comparing output from openssl to aes-ctr, but didn't include them here because of license concerns. Have you considered including such "interoperability" tests in the codebase, would that create an issue with licensing?

[tarcieri]

@koivunej awesome, thanks!

I have been trying to think about the tests. Originally I used quickcheck'd tests comparing output from openssl to aes-ctr, but didn't include them here because of license concerns. Have you considered including such "interoperability" tests in the codebase, would that create an issue with licensing?

There are various places we've used proptest to check against other implementations. I'm not terribly concerned about licensing issues for dev-dependencies since that code is inactive in release builds (IANAL, but in such a case I don't think the licensing requirements of unused dev-dependencies that aren't even linked to are relevant).

[koivunej]

@tarcieri earlier:

I tried rebasing this but unfortunately it seems the implementation has diverged quite a bit.

Can confirm, a lot of time can be wasted this route :) The u64 -> u128 replacement was quite easy to just do, I've reset the PR branch to this work now.. I'll see next about getting that random test case going, and why does that build step fail.

EDIT: Wanted to add, intentionally leaving the question open on what to do with the dependent crates like AES-SIV. I'd kind of want to see the counter working as an another generic type but didn't again do anything related to this yet, at least.

[tarcieri]

Wanted to add, intentionally leaving the question open on what to do with the dependent crates like AES-SIV.

@koivunej I'll need to evaluate it on a case-by-case basis. AES-SIV is in a weird position where it always clears some counter bits to make it possible to use with 32-bit, 64-bit, or 128-bit counters internally, so I don't think it will be a problem.

I'd kind of want to see the counter working as an another generic type but didn't again do anything related to this yet, at least.

That seems somewhat tricky. It was already somewhat tricky dealing with endianness in the Ctr32* types.

[koivunej]

... the counter working as an another generic type ...

That seems somewhat tricky. It was already somewhat tricky dealing with endianness in the Ctr32* types.

Yeah I can kinda see how that'd happen.. I had to write it since it's my only idea so far :)

RE: openssl in the dev-dependencies, that cannot work as we cannot have optional dev dependencies, also I got some ugly linker errors which went away with a compiling without lld-9 and never came back.

Now that the test case is back up to date (in an std subcrate under aes-ctr/aes-ctr-compat), I am again faced with what to test? First instict was to do a multiblock encryption, check against openssl ciphertext, decryption on nonces around maximum values of u128, u64, u32, u16? Probably only two of those are valid. I guess there could be a number of random single block encryption and openssl ciphertext comparisons?

Also, I think you reran the tests. I wonder why am I not seeing that failure over here, perhaps I'll look into it next.

[tarcieri]

I am again faced with what to test? First instict was to do a multiblock encryption, check against openssl ciphertext, decryption on nonces around maximum values of u128, u64, u32, u16?

Yeah, AES-GCM-SIV has some "counter wrap" tests like this (albeit for its 32-bit little endian counter), for example:

https://tools.ietf.org/html/rfc8452#appendix-C.3

Also, I think you reran the tests.

Nope!

[koivunej]

Also, I think you reran the tests.

Nope!

I must have seen this wrong, or there was some github ui loading issue. Nevermind :) It seems that the test failure is with aesni, which seems to produce the same ciphertext as before this PR. The similar change to aesni might be a bit more involved.

https://tools.ietf.org/html/rfc8452#appendix-C.3

Lets see if I can make anything out of this... Not much, I dont know the construction at all. I'll push what I have at the moment next.

[tarcieri]

Not much, I dont know the construction at all. I'll push what I have at the moment next.

I was just pointing it out as an example of a test for counter wrapping behavior. It sounds like you get the basic idea.

[koivunej]

Pushed the latest.

In 4641aa8 I've used the aes-ctr/aes-ctr-compat (basically a simplification of the poc) from 22912cc to generate the data to be used in aes-ctr/src/lib.rs as test cases. I dont think the aes-ctr/aes-ctr-compat should really be included but perhaps I can publish it as a separate repository if someone wants to tinker around later on, perhaps to grow a more complete comparison suite? Dropping it would be just dropping a single commit.

The openssl could not had been added in aes-ctr because of the previously linked "optional dev-dependencies are not allowed", which would mean stdification of the crate by default and only being no_std without dev-dependencies. I'd like to keep it out of this repository because as a workspace member it'll add unnecessary items to Cargo.lock.

The CI running the tests, the last step with aesni still fail as expected (the more involved change).

I did try extracting the trait Counter, while certainly doable I cannot think how to efficiently implement anything other than the current u128 or "u128 and a mask" (to imitiate the previous behaviour) counters with it.

So what would be the next steps? File a bug against block-ciphers for the aesni::Aes{128,192,256}Ctr impls (the more involved change)?

[tarcieri]

@koivunej if you rebase, the aes-ctr crate has been removed from this repository and the tests should pass.

I think we should change the AES-CTR implementation in the aes crate to match, but it's a bit in flux right now.

[koivunej]

@tarcieri rebased, dropped other than the actual changes. The previous version changes can be found at: https://github.com/koivunej/stream-ciphers/tree/ctr128-64to128_nth_revision

Updating the PR desc next.

[tarcieri]

@newpavlov what do you think about these changes?

We'd need to make similar ones in the aes crate.

[newpavlov]

Looks good, but I think we should postpone release with this change until cipher v0.3 release and ideally have all 32, 64 and 128 bit variants, including BE and LE flavors.

[tarcieri]

Yeah I agree, we can rename the current Ctr128 to Ctr64, make it generic over block sizes ala Ctr32BE and Ctr32LE, and then this new implementation can become Ctr128. This is more or less what I proposed in #172.