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

Add more PQC hybrid key exchange algorithms #7821

Merged

Conversation

Frauschi
Copy link
Contributor

@Frauschi Frauschi commented Aug 1, 2024

Hi,

This PR adds support for all remaining hybrid PQC + ECC hybrid key exchange groups to match OQS. Next to two new combinations with SECP curves, this mainly also adds support for combinations with X25519 and X448.

This also enables compatibility with the PQC key exchange support in Chromium browsers and Mozilla Firefox (hybrid Kyber768 and X25519; when WOLFSSL_ML_KEM is not defined).

In the process of extending support, some code and logic cleanup happened. Furthermore, two memory leaks within the hybrid code path have been fixed.

Looking forward to your feedback.

@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@dgarske dgarske requested a review from anhu August 1, 2024 16:05
@dgarske
Copy link
Contributor

dgarske commented Aug 1, 2024

Okay to test. Contributor agreement on file. @anhu please review. Thanks

@dgarske
Copy link
Contributor

dgarske commented Aug 1, 2024

Seems to be consistently failing with:

   842: test_multiple_crls_same_issuer                      :error = -361, certificate revoked
error = -313, received alert fatal error

ERROR - tests/api.c line 7417 failed with:
    expected: test_ssl_memio_do_handshake(&test_ctx, 10, ((void *)0)) == (1)
    result:   0 != 1

error = -361, certificate revoked
error = -313, received alert fatal error

ERROR - tests/api.c line 7417 failed with:
    expected: test_ssl_memio_do_handshake(&test_ctx, 10, ((void *)0)) == (1)
    result:   0 != 1

...

Server Random : 136CBD0B1E6A36EBA50790532D5963534E14BF12E3SSL_accept error -352, Bad ECC Peer Key
wolfSSL error: SSL_accept failed
C35D79C957A2C04C2DF2C2
Client message: hello wolfssl!
I hear you fa shizzle!
trying server command line[1541]: SuiteTest -v 4 -l TLS13-AES128-GCM-SHA256 -c ./certs/server-ecc.pem -k ./certs/ecc-key.pem -Y -2 -p 0 
trying client command line[1542]: SuiteTest -v 4 -l TLS13-AES128-GCM-SHA256 -A ./certs/ca-ecc-cert.pem -y -2 -p 34205 
FAIL scripts/unit.test (exit status: 1)

Copy link
Member

@anhu anhu left a comment

Choose a reason for hiding this comment

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

Please be sure to test against --enable-kyber

@Frauschi
Copy link
Contributor Author

Frauschi commented Aug 2, 2024

Updated the PR with a fix for the failing tests.

Please be sure to test against --enable-kyber

The tests now properly work with --enable-all --enable-asn=template, --enable-all --enable-asn=template --enable-experimental --enable-kyber and --enable-all --enable-asn=template --enable-experimental --with-liboqs

Furthermore, I updated the unit tests to also test the new changes. The only thing I had to test manually are the new hybrid curves with DTLS (when also enabled with --enable-dtls13 --enable-dtls-frag-ch), as the unit tests break before even reaching those tests (pretty surely not related to this PR, as it fails equally on current master). Using the example client and server with the same arguments as the unit tests, the new curves also work with DTLS. I opened an issue for the failing DTLS tests (see #7825).

@Frauschi Frauschi requested a review from anhu August 2, 2024 14:16
@dgarske
Copy link
Contributor

dgarske commented Aug 5, 2024

Hi @Frauschi we had some issues with our GitHub CI actions. If you rebase this to latest master it should resolve the errors you are seeing. Thank you

@dgarske dgarske assigned anhu and unassigned anhu Aug 5, 2024
@Frauschi
Copy link
Contributor Author

Frauschi commented Aug 5, 2024

Rebased to current master. Thanks for the hint.

FYI, I'm on vacation for the next two weeks, so any upcoming work on this will probably be delayed until afterward.

@Frauschi
Copy link
Contributor Author

Rebased to current master (since #7807 has been merged). I also think that the one NGINX test that has been failing wasn't related to this PR's changes? Can someone retest this please?

@douzzer
Copy link
Contributor

douzzer commented Aug 23, 2024

retest this please.

@JacobBarthelmeh
Copy link
Contributor

This says the branch has conflicts that must be resolved. Could you rebase on top of wolfssl/master please?

@Frauschi
Copy link
Contributor Author

This says the branch has conflicts that must be resolved. Could you rebase on top of wolfssl/master please?

Rebased to current master.

I also updated the PR to reflect the latest Code Point changes in OQS and also incorporated the new Code Points currently set in draft-kwiatkowski-tls-ecdhe-mlkem.

The only thing that is not done yet is the proposed swap of classic and PQC key material within draft-kwiatkowski-tls-ecdhe-mlkem in the key share in case of X25519 hybrids. This is done to always have a FIPS approved algorithms first in case of hybrids (and X25519 is not FIPS approved). See here.

This swap still has to be implemented. However, that will require more thorough code changes. I think I can tackle that around next week. You can decide if this should also go into this PR, or if that should go in a separate one.

@Frauschi
Copy link
Contributor Author

I updated the PR with two new commits to implement the reversed order of X25519/X448 based hybrids, following draft-kwiatkowski-tls-ecdhe-mlkem now.

I tested compatibility with both Firefox and Google Chrome nightly, as both have support for X25519MLKEM768. When compiling with Kyber Round 3 compatibility (WOLFSSL_KYBER_ORIGINALdefined), then we are still compatible to current versions using hybrids of X25519 and Kyber Round 3 (with the old order).

For testing the Round 3 compatibility, I used liboqs as the WolfSSL implementation is not working properly currently (see #8023).

@Frauschi
Copy link
Contributor Author

With the fixes in #8027 applied, we now also have compatibility with current versions of Chrome and Firefox when compiled with the WolfSSL Kyber implementation and with WOLFSSL_KYBER_ORIGINAL defined. (I manually patched the preferredGroup array to make sure the PQC key share is selected during the handshake, as indicated in #8024).

@Frauschi
Copy link
Contributor Author

Frauschi commented Nov 12, 2024

I updated the PR, replacing the three commits with two new ones. The first is a concatenation of the old three ones with further additions. The second one is a proposed fix for #8024.

The first commit adds the new X25519 / X448 hybrids, both for the draft and final versions of ML-KEM (based on the recent addition in #8143). This enables PQC compatibility with all current browsers.

I also updated some of the code points to reflect the latest drafts of draft-connolly-tls-mlkem-key-agreement and draft-kwiatkowski-tls-ecdhe-mlkem.

The proposed fix in the second commit is a simple but pragmatic solution for #8024. If you don't see that as a proper solution or if you prefer to handle that in a separate PR, I can drop the commit.

Tagging @SparkiDev and @ColtonWilley in addition to @anhu to also take a look at the changes.

@dgarske dgarske unassigned anhu and dgarske Feb 14, 2025
@anhu
Copy link
Member

anhu commented Feb 14, 2025

Hi @Frauschi ,
Please note that we will soon be removing the liboqs integration to simplify our code base.

@Frauschi
Copy link
Contributor Author

The two new macros are from liboqs to determine if the final ML-KEM version is available or only the original Kyber version.
In general, all liboqs related changes of this PR are simply present to make sure there is feature parity for the user via both the WolfSSL implementation of ML-KEM and liboqs.

Based on the hint from @anhu, I see three possibilities now:

  • Add the two new macros to .wolfssl_known_macro_extras (where should documentation go then?)
  • Remove them and just assume/require the final ML-KEM version in liboqs (dropping support for the original Kyber version via liboqs)
  • Completely remove the liboqs changes from this PR

What do you think @anhu @dgarske?

@dgarske
Copy link
Contributor

dgarske commented Feb 17, 2025

The two new macros are from liboqs to determine if the final ML-KEM version is available or only the original Kyber version. In general, all liboqs related changes of this PR are simply present to make sure there is feature parity for the user via both the WolfSSL implementation of ML-KEM and liboqs.

Based on the hint from @anhu, I see three possibilities now:

  • Add the two new macros to .wolfssl_known_macro_extras (where should documentation go then?)
  • Remove them and just assume/require the final ML-KEM version in liboqs (dropping support for the original Kyber version via liboqs)
  • Completely remove the liboqs changes from this PR

What do you think @anhu @dgarske?

Hi @Frauschi ,

I think we already have macros for this. See WOLFSSL_NO_ML_KEM and WOLFSSL_KYBER_ORIGINAL from our wc_kyber.c. Note we will be eventually removing all liboqs support, since we have our own implementations now.

@Frauschi
Copy link
Contributor Author

I replaced the two liboqs macros with the WolfSSL ones (WOLFSSL_NO_ML_KEMand WOLFSSL_KYBER_ORIGINAL).

Removal of liboqs support should be independent of this PR as all liboqs related changes are wrapped in #ifdef HAVE_LIBOQS and are only there to have feature parity until it is removed (please note that these changes are mainly from August of last year, where the removal of liboqs wasn't on the horizon yet). So for removal, one simply has to delete those code segments.

@Frauschi
Copy link
Contributor Author

BTW, the TLSX changes in #8467 are partly equivalent to the latest changes I did in this PR to keep the generated KyberKey object during the handshake.

@dgarske
Copy link
Contributor

dgarske commented Feb 20, 2025

BTW, the TLSX changes in #8467 are partly equivalent to the latest changes I did in this PR to keep the generated KyberKey object during the handshake.

@Frauschi the other PR #8467 has been merged. Looks like your PR has some conflicts now. Would you mind resolving those? Thank you again for all your amazing work!

SparkiDev
SparkiDev previously approved these changes Feb 20, 2025
@Frauschi
Copy link
Contributor Author

Rebased and resolved the conflicts after #8467. Should be good to go now (hopefully).

Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Very nice work! Just a simple cast issue with g++. Otherwise this looks complete and high quality.

@dgarske dgarske assigned Frauschi and unassigned Frauschi Feb 21, 2025
Add support for X25519 and X448 based hybrid PQC + ECC key exchange
groups. Furthermore, two new combinations with SECP curves are added to
match OQS combinations.

This also incorporates the changed order of X25519 and X448 based
combinations to place the PQC material before the ECDH material. This is
motivated by the necessity to always have material of a FIPS approved
algorithm first.

Also, codepoints are updated to reflect the latest draft standards for
pure ML-KEM and some of the hybrids. With these changes and based on the
recent additions to both enable ML-KEM final and draft versions
simultaneously, a WolfSSL TLS server is now compatible with all recent
browsers that support either the draft version of ML-KEM (Chromium based
browsers and Firefox < version 132; only when the draft version is
enabled in the build) or the final version already (Firefox > version 132).

In the process of extending support, some code and logic cleanup
happened. Furthermore, some memory leaks within the hybrid code path have
been fixed.

Signed-off-by: Tobias Frauenschläger <[email protected]>
In case no user group ranking is set, all groups are now ranked equally
instead of the order in the `preferredGroup` array. This is the
behavior already indicated in the comment header of the function.

This change is necessary for applications that do not set their own
group ranking (via `wolfSSL_CTX_set_groups()` for example). When such an
application creates a TLS server and receives a ClientHello message with
multiple key shares, now the first key share is selected instead of the
one with the lowest index in the `preferredGroup` array.

Recent browsers with PQC support place two key shares in their
ClientHello message: a hybrid PQC + X25519 one and at least one
classic-only one. The hybrid one is the first one, indicating a
preference. Without this change, however, always the classic-only key
share has been selected, as these algorithms have a lower index in the
`preferredGroup` array compared to the PQC hybrids.

Tested using a patched version of NGINX.

This change also results in a different selection of a key share group
in case of a HelloRetryRequest message. For the tests, where static
ephemeral keys are used (`WOLFSSL_STATIC_EPHEMERAL`), an additional
check is necessary to make sure the correct key is used for the ECDH
calculation.

Signed-off-by: Tobias Frauenschläger <[email protected]>
@dgarske dgarske self-requested a review February 21, 2025 17:49
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Thank you @Frauschi . You are amazing

Copy link
Member

@anhu anhu left a comment

Choose a reason for hiding this comment

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

Some really great work @Frauschi !! Thank you so much!

@dgarske
Copy link
Contributor

dgarske commented Feb 21, 2025

Retest this please: "Found unhandled org.jenkinsci.plugins.workflow.support.steps.AgentOfflineException exception"

@dgarske dgarske merged commit 865f96a into wolfSSL:master Feb 21, 2025
177 of 178 checks passed
@Frauschi Frauschi deleted the pqc_hybrid_kex branch February 24, 2025 08:03
@Frauschi Frauschi mentioned this pull request Feb 24, 2025
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.

7 participants