Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support X25519Kyber768Draft00 hybrid post-quantum KEM #43

Merged
merged 16 commits into from
Jul 18, 2023

Conversation

bwesterb
Copy link

@bwesterb bwesterb commented Apr 14, 2023

See https://datatracker.ietf.org/doc/draft-westerbaan-cfrg-hpke-xyber768d00/

I'm not that experienced in Rust, so I believe various things can be done a bit more ergonomically.

As a Kyber implementation, I use the one by Argyle Software. I carefully reviewed the basic implementation (not avx2!) and found some issues. I fixed those in the fork that's actually used. That fork also adds deterministic key generation and an API to convert private->public key.

I considered using liboqs-rust's implementation of Rust instead, but it unfortunately doesn't support the deterministic key generation required.

Test vectors were generated with our Go implementation in Circl.

To do:

  • Check test vectors

@bwesterb bwesterb marked this pull request as draft April 14, 2023 11:19
@bwesterb bwesterb marked this pull request as ready for review April 14, 2023 14:02
@bwesterb bwesterb changed the title [WIP] Support X25519Kyber768Draft00 hybrid post-quantum KEM Support X25519Kyber768Draft00 hybrid post-quantum KEM Apr 14, 2023
@bwesterb
Copy link
Author

Hi @rozbb, when would you have time to have a look at this?

@rozbb
Copy link
Owner

rozbb commented May 1, 2023

Hi Bas. Thanks for this! Sorry, I've been extremely swamped at work. I intend to take a look at this more carefully this week, but on first glance, I'm not sure this is something that should be in the upstream repo. Lots of parts of this, including the primitives and the spec itself, are quite new. I think it might be best to keep this as a separate repo for now, both to deter people from using it prematurely, and because I don't think I have the bandwidth to maintain and update it with the libs and specs.

@bwesterb
Copy link
Author

bwesterb commented May 1, 2023

Thanks Michael, looking forward to your review.

Lots of parts of this, including the primitives and the spec itself, are quite new.

True. The final version of Kyber, and its integration with HPKE, will be different. Hence the "draft00" in the name. However, it's likely that this version is going to see wide (preliminary) deployment in, among other things, post-quantum ECH. (Just like this version of Kyber already landed in BoringSSL.)

I don't think I have the bandwidth to maintain and update it with the libs and specs.

I will create PRs to keep it up to date with the libs/spec. I understand if you'd only want to add it once it's completely settled.

@rozbb
Copy link
Owner

rozbb commented May 6, 2023

(sorry, ran checks while looking at this)

First off, these are great edits, and it fits in really well with the existing library! I'm glad the structure was such that a non-DH KEM was more or less pluggable into the framework.

That said, I realized as I was going through the code that I don't think I'll be able to meaningfully look at the PRs and iterations as they come in. It might be best for your own productivity to iterate the feature in your fork, and send a huge lump of a PR when nearing standardization. Does that make sense? Sorry, I wish I had more bandwidth lately to keep up to date with the cool PQ stuff, but I don't :(

@bwesterb
Copy link
Author

bwesterb commented May 8, 2023

Thanks for having a look!

and send a huge lump of a PR when nearing standardization.

I think the final PR will be almost identical to the current one, barring the following changes:

  1. codepoint/name for the final hybrid;
  2. bump of Kyber crate for the final version;
  3. updated test vectors;
  4. use ExtractAndExpand(x25519_ss || kyber_ss, kyber_ct) instead of x25519_ss || kyber_ss as shared secret for IND-CCA2 robustness, as the final version of Kyber will not hash the ciphertext in anymore.

@bwesterb
Copy link
Author

IANA assigned a codepoint for this version of X25519+Kyber768Draft00.

@rozbb
Copy link
Owner

rozbb commented Jul 11, 2023

Sorry for the long delay on this. I think merging into a branch would be appropriate. I'm gonna take a closer look this weekend and hopefully merge. Thank you again for the contribution. It would be very nice to be the first HPKE instance with PQ support once it drops!

Copy link
Owner

@rozbb rozbb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some notes in there, and pushed some changes. Lmk if you agree with them.

One nagging thing I can't solve. In cargo doc --all-features I can't make the xyber KEM get exported to be alongside the other KEMs in the kem module. Do you know how to fix this by any chance?

   Compiling hpke v0.10.0 (/Users/micro/projects/rust-hpke)
warning: ambiguous glob re-exports
  --> src/kem.rs:19:9
   |
14 | pub use dhkem::*;
   |         -------- but the name `EncappedKey` in the type namespace is also re-exported here
...
19 | pub use xyber768d00::*;
   |         ^^^^^^^^^^^^^^ the name `EncappedKey` in the type namespace is first re-exported here
   |
   = note: `#[warn(ambiguous_glob_reexports)]` on by default

Thank you again for this! I agree with the spec as far as I understand it. It's extremely simple, which I think is a feature.

# https://github.com/Argyle-Software/kyber/issues/73
# https://github.com/Argyle-Software/kyber/issues/75
# https://github.com/Argyle-Software/kyber/issues/77
git = "https://github.com/bwesterb/argyle-kyber"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for future: we should probably not rely on this code or forks of it. I agree with your commits @bwesterb but I think that library needs quite a bit of work to get to a good state.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some good people working on a better Rust implementation of Kyber. Let's promptly switch when it's out.

I did very carefully check every bit of the code in that fork, and I feel confident in deploying it. (Not so much upstream though.)


const KEM_ID: u16 = 0x30;

fn derive_keypair(ikm: &[u8]) -> (Self::PrivateKey, Self::PublicKey) {
Copy link
Owner

@rozbb rozbb Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note for your spec. DeriveKeypair currently says

For a given KEM, the ikm parameter given to DeriveKeyPair() SHOULD have length at least Nsk, and SHOULD have at least Nsk bytes of entropy.

This probably isn't true anymore. In this case, the recommendation should probably be 96 bytes

Copy link
Author

@bwesterb bwesterb Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HPKE spec stipulates that

For a given KEM, the ikm parameter given to DeriveKeyPair() SHOULD have length at least Nsk, and SHOULD have at least Nsk bytes of entropy.

For a crypto system where a secret key is some random blob directly reinterpreted, such as with ECC, this makes sense. In general, it doesn't. Kyber768, for instance, stretches a single 32 byte seed into two secret values e_1 and e_2 that have roughly 3119 bits of entropy. That pulls the maximum security down to 256 bits, but that's completely fine as Kyber768 targets a AES-192 level of security anyway.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the goal a property like: if one of the underlying primitives is broken, the other holds up? So if say X25519 is broken, and you only have 128 bits of entropy in the Kyber setup, then the security level is below the desired 192, no?

This is offtopic, ofc. Just interested in the hybrid model

Copy link
Author

@bwesterb bwesterb Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Officially Kyber768 targets NIST PQC level 3, which is defined as being as hard to attack as AES-192. The security target of this hybrid is just "128 bits" though, which is the security level of X25519. We picked Kyber768 instead of Kyber512, as the Kyber team themselves are more comfortable with Kyber768 as the default in case of cryptanalytic improvements.

Is the goal a property like: if one of the underlying primitives is broken, the other holds up?

Yes, more precisely, it's designed to be IND-CCA2 robust. That is: if one is broken, the combined hybrid (as used in HPKE) is still IND-CCA2 (classically — not necessarily PQ of course if Kyber is broken). See the security considerations section of the I-D.

So if say X25519 is broken, and you only have 128 bits of entropy in the Kyber setup, then the security level is below the desired 192, no?

There is 256 bits of entropy in the Kyber setup.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, I know "entropy math" is famously hand-wavy. But if you have a 32 byte seed that you stretch into 32 + 64 bytes (X25519 and Kyber respectively) and say the entire X25519 is broken, then there should only be 2/3 of the entropy left, ie 170 bits. Similarly if Kyber is fully broken then there's only 1/3 = 85 bits left. Is that wildly wrong?

Copy link
Author

@bwesterb bwesterb Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me simplify. I generated a secret 128-bit key X, and computed Y = SHAKE128(X, 256). The first 128 bits of Y are 64ba9be5763117f981a6a47af2352481. With your reasoning, there is no entropy left in X. In a sense there is some merit to it: it's likely there is only one X matching that Y. But, computationally, it's infeasible to find such X: it's the preimage resistance of SHAKE128. Also, finding the second part of Y (without trying to find X first) is computationally hard. The generic security property is the flat sponge claim.

The same happens here: if Kyber is so broken that we can recover its seed, then we're not done yet. To go from that seed, to the original seed, or the seed used for X25519, you'd need to break SHA2.

Copy link
Author

@bwesterb bwesterb Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, a Kyber public key contains a value called rho (used to generate the matrix A), that's derived from the private seed in a similar way. In fact, the last 32 bytes of the private seed are stretched to rho and sigma. The latter is used to generate the final private values s_1 and s_2.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I think I understand now. Thanks for the link too

Cargo.toml Outdated
@@ -39,7 +41,8 @@ rand_core = { version = "0.6", default-features = false }
p256 = { version = "0.13", default-features = false, features = ["arithmetic", "ecdh"], optional = true}
p384 = { version = "0.13", default-features = false, features = ["arithmetic", "ecdh"], optional = true}
sha2 = { version = "0.10", default-features = false }
serde = { version = "1.0", default-features = false, optional = true }
serde = { version = "1.0", default-features = false, optional = true, features = ["derive"] }
serde-big-array = { version = "0.5", optional = true }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this dep. I don't know who maintains it and it uses a good amount of unsafe. Flagging for later.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, it's from: bwesterb#2 @OtaK

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree with @rozbb. I don't like this dependency either but until serde adopts const generics (tracking issue here: serde-rs/serde#1937) we'll have to rely on a third party implementation of it.

For context, I also saw that serde_with supports const generics and we could totally switch to this implementation -which seems safer- but for the needs of something that is under a feature flag, it is clearly bloated vs serde-big-array so I prioritized code size for this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worked around it entirely. We're already using GenericArray, so I think it makes sense to piggyback off of their serde impls. See newest commit.


/// Deterministically derives a keypair from the given input keying material and ciphersuite
/// ID. The keying material should have as many bits of entropy as it extracts for its seeds,
/// i.e., 768 bits.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "768 bits" doesn't make sense to me. See this comment.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@bwesterb
Copy link
Author

Left some notes in there, and pushed some changes. Lmk if you agree with them.

They're a clear improvement, thanks.

There is an issue with the comments on the seed size: I left a comment.

One nagging thing I can't solve. In cargo doc --all-features I can't make the xyber KEM get exported to be alongside the other KEMs in the kem module. Do you know how to fix this by any chance?

Sorry, I don't.

Thank you again for this! I agree with the spec as far as I understand it. It's extremely simple, which I think is a feature.

Thank you! That was a design goal. :)

@rozbb
Copy link
Owner

rozbb commented Jul 18, 2023

OK I'm pretty happy with it now. Lmk what you think about the switch to GenericArray. Only thing left to do is figure out why the build is broken.

@bwesterb
Copy link
Author

OK I'm pretty happy with it now. Lmk what you think about the switch to GenericArray.

lgtm

@rozbb rozbb changed the base branch from master to unstable-pq-xyber July 18, 2023 14:02
@rozbb rozbb merged commit e519560 into rozbb:unstable-pq-xyber Jul 18, 2023
12 checks passed
OtaK added a commit to wireapp/rust-hpke that referenced this pull request Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants