Add ML-KEM768 KATs from BoringSSL #3

andrewkdinh · 2024-11-10T18:35:58Z

Add KATs for ML-KEM768 under CCLA from https://boringssl.googlesource.com/boringssl/

Please advise for how I should credit BoringSSL in the test files.

These KATs test key generation, encapsulation, and decapsulation for the ML-KEM768 provider.

Relevant notes:

Added functionality to the ML-KEM provider to export/import. These may not be fully implemented yet (see Add encode/decode logic for ML-KEM openssl/project#1036)
Exposed some more low-level ML-KEM API's to allow for deterministic encapsulation/key generation
Actually run 'mlkem_internal_test' with make test
Fixes Port BoringSSL KAT tests for ML-KEM 768 openssl/project#1045

Checklist

tests are added or updated

Although this cannot really happen check for 0 block size to avoid division by 0. Fixes Coverity 1633936 Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> (Merged from openssl#25822)

andrewkdinh · 2024-11-10T18:44:18Z

Tagging everybody from the initial PR

@mattcaswell @baentsch @slontis @paulidale @t8m

baentsch · 2024-11-11T09:58:31Z

Thanks for this work @andrewkdinh ! Could you please re-base so the diff is easier to read?

t8m · 2024-11-11T10:22:35Z

Thanks for this work @andrewkdinh ! Could you please re-base so the diff is easier to read?

It would make sense to move this to openssl/openssl after the initial PR is merged into the feature branch.

andrewkdinh · 2024-11-11T20:43:03Z

@baentsch I've re-based this now

@t8m Ok, I think I'll need to create a separate PR in openssl/openssl once openssl#25848 is merged

providers/implementations/keymgmt/mlkem_kmgmt.c

baentsch · 2024-11-12T05:39:27Z

providers/implementations/keymgmt/mlkem_kmgmt.c

-     *  { OSSL_FUNC_KEYMGMT_EXPORT, (void (*)(void))mlkem_import },
-     */
+     * TODO(ML-KEM): https://github.com/openssl/private/issues/698
+     * Export/import functionality has been partially implemented.


Worthwhile stating here which things are missing?

The encode/decode logic pretty much matches what I saw in other providers. But from https://github.com/openssl/private/issues/698:

prove interoperability -- and for that we need reference to a standard (please fill in the above if you can: I didn't find anything, so if we want to do this without a standard (?), we need to agree on/how to do interop testing with other implementations).

I haven't done anything like this

ACK. Suggest referencing to openssl#25885 now (contains draft spec references).

test/evp_test.c

baentsch · 2024-11-12T07:02:15Z

test/recipes/30-test_evp.t

@@ -130,6 +130,9 @@ my @defltfiles = qw(
                     evppkey_kdf_scrypt.txt
                     evppkey_kdf_tls1_prf.txt
                     evppkey_rsa.txt
+                     evppkey_mlkem768_keygen.txt


shouldn't they be done (pushed to defltfiles) only if no_mlkem is not set?

Updated: https://github.com/baentsch/openssl/compare/b8e8d90a0a369a69f399885419df3d793006fe3a..3cc382434d4b8f576fc6213e5741b55ffe6ce396

Not quite OK: The new variable name is "no_mlkem" (not declared as such). After fixing that, the tests fail for me (both, test_evp and test_evp_extra): Do I need to do some special setup to run them? I used make TESTS='test_evp_extra test_evp' test...

Oops, there was a couple incorrect things after fixing a rebase. All tests succeed for me when I run make test... can you try again with the latest commit?

providers/implementations/keymgmt/mlkem_kmgmt.c

baentsch · 2024-11-12T12:34:12Z

@andrewkdinh fyi, with the three code changes as per the above, the tests all work -- for ML-KEM. Failures in SM2 and FFDHE...

andrewkdinh · 2024-11-12T19:05:17Z

@baentsch All tests pass for me with make test. But this can also be verified when I open the PR to openssl/openssl when CI runs

baentsch · 2024-11-13T06:55:43Z

@baentsch All tests pass for me with make test. But this can also be verified when I open the PR to openssl/openssl when CI runs

@andrewkdinh : Confirmed for ML-KEM. Thanks! I'd be all for opening this as a public PR now. One heads-up (nit, I hope), though: These 2 tests (unrelated to MLKEM) fail for me (just did clean build using ./config && make -j && make test on a vanilla x64 ubuntu box):


        # INFO:  @ test/testutil/stanza.c:21
        # Reading ../../test/recipes/30-test_evp_data/evppkey_ffdhe.txt
        # INFO:  @ test/testutil/stanza.c:122
        # Starting "RFC7919 DH tests" tests at line 15
../../util/wrap.pl ../../test/evp_test -config ../../test/default-and-legacy.cnf ../../test/recipes/30-test_evp_data/evppkey_ffdhe.txt => 139
not ok 25 - running evp_test -config ../../test/default-and-legacy.cnf evppkey_ffdhe.txt
# ------------------------------------------------------------------------------
        # INFO:  @ test/testutil/stanza.c:21
        # Reading ../../test/recipes/30-test_evp_data/evppkey_sm2.txt
        # INFO:  @ test/testutil/stanza.c:122
        # Starting "SM2 tests" tests at line 19
        # INFO:  @ test/testutil/stanza.c:122
        # Starting "SM2 key generation tests" tests at line 78
../../util/wrap.pl ../../test/evp_test -config ../../test/default-and-legacy.cnf ../../test/recipes/30-test_evp_data/evppkey_sm2.txt => 139
not ok 78 - running evp_test -config ../../test/default-and-legacy.cnf evppkey_sm2.txt

@t8m @mattcaswell: What does it take to merge openssl#25848 such as to allow @andrewkdinh to raise this code as a public PR towards the feature branch?

Based on code from BoringSSL covered under Google CCLA Original code at https://boringssl.googlesource.com/boringssl/+/HEAD/crypto/mlkem VSCode automatic formatting (andrewd@openssl.org) Just do some basic formatting to make diffs easier to read later: convert from 2 to 4 spaces, add newlines after function declarations, and move function open curly brace to new line (andrewd@openssl.org) Move variable init to beginning of each function (andrewd@openssl.org) replace CBB API fixing up constants and parameter lists replace BORINGSSL_keccac calls with EVP calls added library symbols and low-level test case switch boringssl constant time routines for OpenSSL ones data type assertion and negative test added moved mlkem.h to include/crypto changed function naming to be in line with ossl convention remove Google license terms based on CCLA add constant_time_lt_32 convert asserts to ossl_asserts where possible add bssl keccak, pubK recreation, formatting add provider interface to utilize mlkem768 code enabling TLS1.3 use revert to OpenSSL DigestXOF use EVP_MD_xof() to determine digest finalisation (pauli@openssl.org) change APIs to return error codes; reference new IANA number; move static asserts to one place remove boringssl keccak for good fix coding style and return value checks ANSI C compatibility changes remove static cache objects all internal retval functions used leading to some new retval functions Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#25848)

baentsch · 2024-11-13T11:01:35Z

@andrewkdinh Please redirect this PR towards the mlkem feature branch now: I'm anxious to (first review, of course, and) land this code (and APIs and provider improvements) there to base the next steps on that (ml-kem-1024, probably to ascertain our approach "generalizes" well). Thanks in advance!

andrewkdinh · 2024-11-13T11:25:47Z

@baentsch I need to wait until your PR is fully synced into feature/ml-kem so that the diff is just my changes (usually <24 hours): openssl/openssl@feature/ml-kem...andrewkdinh:openssl:mlkem-kats

I'll also be looking into the failing tests; strange that it didn't fail for me locally

mattcaswell · 2024-11-13T11:41:37Z

@baentsch I need to wait until your PR is fully synced into feature/ml-kem so that the diff is just my changes (usually <24 hours): openssl/openssl@feature/ml-kem...andrewkdinh:openssl:mlkem-kats

@baentsch's PR has been merged to the feature/ml-kem branch. You should be able to create your PR now.

Add KATs for ML-KEM768 under CCLA from https://boringssl.googlesource.com/boringssl/ These KATs test key generation, encapsulation, and decapsulation for the ML-KEM768 provider. Relevant notes: - Added functionality to the ML-KEM provider to export/import. These may not be fully implemented yet (see openssl/private#698) - Exposed some more low-level ML-KEM API's to allow for deterministic encapsulation/key generation - Actually run 'mlkem_internal_test' with `make test` - Fixes openssl/private#704

andrewkdinh · 2024-11-13T12:50:31Z

Opened openssl#25938 to replace this PR

Here the undefined value "npa" passed to a function WPACKET_sub_memcpy_u16(pkt, npa, npalen). However the value is not really used, because "npalen" is zero, but the call statememt itself is considered an invalid operation by the new sanitizer. The original sanitizer error report was: ==49175==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x55a276b29d6f in tls_construct_stoc_next_proto_neg /home/runner/work/openssl/openssl/ssl/statem/extensions_srvr.c:1518:21 #1 0x55a276b15d7d in tls_construct_extensions /home/runner/work/openssl/openssl/ssl/statem/extensions.c:909:15 #2 0x55a276b513dc in tls_construct_server_hello /home/runner/work/openssl/openssl/ssl/statem/statem_srvr.c:2471:10 #3 0x55a276b2e160 in write_state_machine /home/runner/work/openssl/openssl/ssl/statem/statem.c:896:26 open-quantum-safe#4 0x55a276b2e160 in state_machine /home/runner/work/openssl/openssl/ssl/statem/statem.c:490:21 open-quantum-safe#5 0x55a276b2f562 in ossl_statem_accept /home/runner/work/openssl/openssl/ssl/statem/statem.c:309:12 open-quantum-safe#6 0x55a276a9f867 in SSL_do_handshake /home/runner/work/openssl/openssl/ssl/ssl_lib.c:4890:19 open-quantum-safe#7 0x55a276a9f605 in SSL_accept /home/runner/work/openssl/openssl/ssl/ssl_lib.c:2169:12 open-quantum-safe#8 0x55a276a3d4db in create_bare_ssl_connection /home/runner/work/openssl/openssl/test/helpers/ssltestlib.c:1281:24 open-quantum-safe#9 0x55a276a3d7cb in create_ssl_connection /home/runner/work/openssl/openssl/test/helpers/ssltestlib.c:1350:10 open-quantum-safe#10 0x55a276a64c0b in test_npn /home/runner/work/openssl/openssl/test/sslapitest.c:12266:14 open-quantum-safe#11 0x55a276b9fc20 in run_tests /home/runner/work/openssl/openssl/test/testutil/driver.c:377:21 open-quantum-safe#12 0x55a276ba0b10 in main /home/runner/work/openssl/openssl/test/testutil/main.c:31:15 Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26269)

baentsch force-pushed the bsslmlkem768 branch from c9864c2 to d1e5958 Compare November 11, 2024 08:24

andrewkdinh force-pushed the mlkem-kats branch 2 times, most recently from d8525e4 to 989f3f2 Compare November 11, 2024 20:40

andrewkdinh force-pushed the mlkem-kats branch 3 times, most recently from 49e7aac to b8e8d90 Compare November 11, 2024 23:12

baentsch mentioned this pull request Nov 12, 2024

ML-KEM768 openssl/openssl#25848

Closed