Update to round 4 specification

[pufferffish]

The Classic McEliece team has announced the round 4 specification (See here). This PR updates the implementation and the KAT tests according to the specification.

[Colfenor]

Hello @octeep :) !

First, thank you for your PR ! Please first run cargo fmt and verify the unit tests. It seems that test_encrypt, test_crypto_kem_dec and test_crypto_kem_enc are failing specifically for the variant mceliece8192128f we would need to fix that prior to merging.

You can run the unit test for a specific variant using e.g.: cargo test --features mceliece8192128f

[prokls]

Sorry, I am really behind with reviewing the source code. I will still go through it and dedicate some time tomorrow.

[prokls]

FTR the following changes have been applied in the reference C implementation:

• CRYPTO_CIPHERTEXTBYTES resized from 128 to 96
• protocol changes in crypto_kem_enc and crypto_kem_dec of operations.c
• controlbits.c now uses crypto_declassify.h
• controlbits.h uses int32_min instead of a conditional assignment (const-time op)
• controlbits.c added in controlbitsfrompermutation(…):

    diff = crypto_int16_nonzero_mask(diff);
    crypto_declassify(&diff,sizeof diff);

• introduce uint16_is_smaller_declassify and uint32_is_equal_declassify in encrypt.c and pk_gen.c (const-time op)
• introduce gf_is_zero_declassify in sk_gen.c
• introduce subroutines/crypto_declassify.h with an empty function body

I wonder whether we should make a new major release for it

[prokls]

• Hm, so I guess one could claim that ciphertext.as_array().len() exposes the length at runtime. But this is not a compile-time value and I don't see any protocol-level breaking changes. Thus, I opt for a minor release with these changes.
• The changes described above can be found in the codebase except for the design of declassify and the min function. I personally cannot identify the purpose of declassify.
• The code changes by @octeep are wonderful and I fully support merging.
• Tomorrow I will publish a new minor version unless opposing opinions emerge.

[prokls]

Great work!

[faern]

This definitely is a protocol public API breaking change. Since the API of Ciphertext::as_array changes from having [u8; 128] as the return type into [u8; 96]

In our client implementation, we expect to receive CRYPTO_CIPHERTEXTBYTES from the other party. And when that constant changes from 128 to 96, but the server still sends 128 bytes (due to not being upgraded) our deserialization/parsing of the response will fail: https://github.com/mullvad/mullvadvpn-app/blob/master/talpid-tunnel-config-client/src/lib.rs#L93

Please release this as a major version.

EDIT: Please also remember to push a git tag. I can't see one for the 2.0.1 release. Would appreciate one 🙏

[Colfenor]

• Hm, so I guess one could claim that ciphertext.as_array().len() exposes the length at runtime. But this is not a compile-time value and I don't see any protocol-level breaking changes. Thus, I opt for a minor release with these changes.

@prokls thanks also for taking time to review :) can you elaborate on what you mean with ciphertext.as_array().len() exposes the length at runtime, was this not a problem before already ?

@faern I've added the git tag v2.0.1 on commit 68fdbe6

concerning major release, since this change does seem to break an implementation depending on the crate, I'd opt for a major release

[faern]

concerning major release, since this change does seem to break an implementation depending on the crate, I'd opt for a major release

Thanks! But it's not just that, the release is breaking the public API and should be a major bump to be semver compliant anyway. The return type of a public method has changed. It's not so much about the .len() really, but just the fact that [u8; 96] is a different type than [u8; 128].

@faern I've added the git tag v2.0.1 on commit 68fdbe6

Thank you 🙏

[faern]

The impl From<[u8; 128]> for Ciphertext is also a point of public API breakage. After this PR, Ciphertext will not implement this any longer. It will implement impl From<[u8; 96]> which is a different thing.

[dkales]

I concur this should be a breaking change.

[dkales]

Can we rebase this onto the current main and then we should merge this and release as 3.0.
I would also propose to yank 2.0.1, since it violates SemVer.
NVM I misunderstood the above conversation about 2.0.1, thought this was already merged there.

[prokls]

This definitely is a protocol public API breaking change. Since the API of Ciphertext::as_array changes from having [u8; 128] as the return type into [u8; 96]

In our client implementation, we expect to receive CRYPTO_CIPHERTEXTBYTES from the other party. And when that constant changes from 128 to 96, but the server still sends 128 bytes (due to not being upgraded) our deserialization/parsing of the response will fail: https://github.com/mullvad/mullvadvpn-app/blob/master/talpid-tunnel-config-client/src/lib.rs#L93

Please release this as a major version.

EDIT: Please also remember to push a git tag. I can't see one for the 2.0.1 release. Would appreciate one pray

Thank you very much for the comments here. Apparently I did not pay enough attention to this. I ran "cargo doc" now and can clearly see arguments to declare this as breaking change. As @faern mentioned CRYPTO_CIPHERTEXTBYTES is exposed and its change implies a breaking change. Let's proceed with a new major version release.

thanks also for taking time to review :) can you elaborate on what you mean with ciphertext.as_array().len() exposes the length at runtime, was this not a problem before already ?

It was not a “problem” per se. It just exposes the length of an array and thus the value changes with the Round 4 implementation. This can have various implications on software depending on our crate. As a result, we should look for changing values in the public API and decide on them whether a new major release is required. The changes of the round 4 implementation indeed (unlike my previous comments) require a new major version.

Since I did not find the time to thoroughly review the tests: what do you think is necessary to accomplish to make it a major release? Is kem fine now?

[dkales]

I guess there should also be some tests that test the kem interface against KATs, since this would have detected the breakage introduced before.

[faern]

Changing values is not by definition an API breakage. The fact that the constant CRYPTO_CIPHERTEXTBYTES changes value is not what makes this a breaking change. The thing that makes it a breaking change is that types change. [u8; 128] is not the same type as [u8; 96]. For the same reason as if you are changing some hypothetical pub fn foo() -> String into pub fn foo() -> Vec<bool>, the signature of the function changes and the API is now different.

Changing values can be classified as breaking. A bit depending on how strict the library author want to be, and how important the value is for users of the library. But that's more of a grey zone. Changing function signatures however is 100% clear API breakage :)

[d-z-m]

Hello 👋, just wanted to say this change is desired, and that I hope code review proceeds in the near future! 🚀

[faern]

I agree. Having access to round 4 CME in Rust is desirable for us as well.

[Colfenor]

Hello from my side, yes I also want to see this merged, however before that I'm currently in the progress of creating a PR which implements the tests for the kem interface against KATs. This is needed as a proof that this PR correctly handles the kem feature

@faern maybe my reply was lost on this issue #34

I have a few questions to you regarding the kem feature written as the last reply in this issue #34

Moreover I created this isse here dealing with the unit tests of kem against kats #37 please feel free to give some feedback

[faern]

I so wish we never went ahead and implemented RustCryptos kem interface. It has terrible ergonomics, does not lend itself very well to zeroing out key material and is a pain to work with 🙈

[faern]

@Colfenor My only objection was that there should be tests catching if CryptoCiphertextBytesTypenum is not defined to the same value as CRYPTO_CIPHERTEXTBYTES. And I have now added that in #39. So if you merge that and consider the move to round 4 a breaking change, then I'm all happy.

[Colfenor]

Alright, yup I agree, the unit test for the kem feature against KATs should not concern this PR and this will be then a breaking change. Therefore we would have 3.0.0 as the new version

[Colfenor]

alright, we should be good to go, (fingers crossed that nothing breaks now in the final steps) thanks everyone for the discussion & thoughts and review

and happy encrypting/decrypting stuff with classic-mceliece-rust round-4 :)
as always, if you spot bugs / inconsistencies please report them here

[Colfenor]

@faern do you want to be added to the MIT license, since you provided a couple of contributions ? I'm not sure about the etiquette, therefore I want to ask

[faern]

@Colfenor Please fix this before making a release! #41

[Colfenor]

thx, that was some timing ^^ the version is not released yet on crates.io

[faern]

@faern do you want to be added to the MIT license, since you provided a couple of contributions ? I'm not sure about the etiquette, therefore I want to ask

I'd be honored to be mentioned as a contributor/copyright holder, but I do not demand it. Do as you see fit, you are allowed to include my name (Please use "Linus Färnstrand").