Add ML-KEM768 KATs from BoringSSL#3
Add ML-KEM768 KATs from BoringSSL#3andrewkdinh wants to merge 4 commits intobaentsch:bsslmlkem768from
Conversation
Although this cannot really happen check for 0 block size to avoid division by 0. Fixes Coverity 1633936 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from openssl#25822)
|
Tagging everybody from the initial PR |
baentsch
force-pushed
the
bsslmlkem768
branch
from
c9864c2 to
d1e5958
Compare
|
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
force-pushed
the
mlkem-kats
branch
2 times, most recently
from
d8525e4 to
989f3f2
Compare
|
@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 |
andrewkdinh
force-pushed
the
mlkem-kats
branch
3 times, most recently
from
49e7aac to
b8e8d90
Compare
| * { 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. |
There was a problem hiding this comment.
Worthwhile stating here which things are missing?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
ACK. Suggest referencing to openssl#25885 now (contains draft spec references).
test/recipes/30-test_evp.t
Outdated
| evppkey_kdf_scrypt.txt | ||
| evppkey_kdf_tls1_prf.txt | ||
| evppkey_rsa.txt | ||
| evppkey_mlkem768_keygen.txt |
There was a problem hiding this comment.
shouldn't they be done (pushed to defltfiles) only if no_mlkem is not set?
There was a problem hiding this comment.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
andrewkdinh
force-pushed
the
mlkem-kats
branch
2 times, most recently
from
962aaeb to
3cc3824
Compare
|
@andrewkdinh fyi, with the three code changes as per the above, the tests all work -- for ML-KEM. Failures in SM2 and FFDHE... |
andrewkdinh
force-pushed
the
mlkem-kats
branch
from
3cc3824 to
c294963
Compare
|
@baentsch All tests pass for me with |
@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 @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 ([email protected]) 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 ([email protected]) Move variable init to beginning of each function ([email protected]) 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 ([email protected]) 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 <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#25848)
|
@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! |
|
@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 |
@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
force-pushed
the
mlkem-kats
branch
from
c294963 to
5365052
Compare
|
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
#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ý <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#26269)
The new malloc failure test caught an asan error in this code: Direct leak of 40 byte(s) in 1 object(s) allocated from: 2025-08-07T03:22:20.3655117Z #0 0x7fb88d8fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69 2025-08-07T03:22:20.3655796Z #1 0x5584f0e4670a in CRYPTO_malloc crypto/mem.c:211 2025-08-07T03:22:20.3656291Z #2 0x5584f0e4679d in CRYPTO_zalloc crypto/mem.c:231 2025-08-07T03:22:20.3657040Z #3 0x5584f11c4c10 in EVP_RAND_CTX_new crypto/evp/evp_rand.c:353 2025-08-07T03:22:20.3657656Z #4 0x5584f0e93b27 in rand_new_drbg crypto/rand/rand_lib.c:666 2025-08-07T03:22:20.3658289Z open-quantum-safe#5 0x5584f0e949d0 in rand_get0_public crypto/rand/rand_lib.c:843 2025-08-07T03:22:20.3658914Z open-quantum-safe#6 0x5584f0e9305b in RAND_bytes_ex crypto/rand/rand_lib.c:490 2025-08-07T03:22:20.3659486Z open-quantum-safe#7 0x5584f0b2405f in SSL_CTX_new_ex ssl/ssl_lib.c:4191 2025-08-07T03:22:20.3660183Z open-quantum-safe#8 0x5584f0ae313c in create_ssl_ctx_pair test/helpers/ssltestlib.c:958 2025-08-07T03:22:20.3660871Z open-quantum-safe#9 0x5584f0adeaf6 in do_handshake test/handshake-memfail.c:56 2025-08-07T03:22:20.3661539Z open-quantum-safe#10 0x5584f0adee50 in test_alloc_failures test/handshake-memfail.c:125 2025-08-07T03:22:20.3662161Z open-quantum-safe#11 0x5584f0cd9da8 in run_tests test/testutil/driver.c:342 2025-08-07T03:22:20.3662664Z open-quantum-safe#12 0x5584f0cda9e5 in main test/testutil/main.c:31 2025-08-07T03:22:20.3663450Z open-quantum-safe#13 0x7fb88d42a1c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f) 2025-08-07T03:22:20.3664630Z open-quantum-safe#14 0x7fb88d42a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f) 2025-08-07T03:22:20.3666608Z open-quantum-safe#15 0x5584f0ade864 in _start (/home/runner/work/openssl/openssl/test/handshake-memfail+0x22a864) (BuildId: 19659a44d8bed2c082918d25425f77e3a98df534) It occurs because when rand_get0_public/rand_get0_private sets an EVP_RAND_CTX object in its thread local storage, it neglects to check the return code of the operation, which may fail when the associated sparse array is expanded. fix it by checking the return code and failing the get0_[public|private] operation so the failure is graceful. Fixes openssl/project#1315 Reviewed-by: Paul Yang <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#28195)
4 participants
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:
make testChecklist