ML-KEM and ML-KEM hybrid #103

mamckee · 2024-12-30T22:57:34Z

This PR adds support for ML-KEM and hybrid ECDHE-MLKEM to the SymCrypt provider. This PR also refactors a significant portion of ECC key management and exposes the ECDH provider interface for ML-KEM hybrid operations. This implementation has been tested against BoringSSL and the OQS provider for compatibility.

These algorithms are not currently present in the default OpenSSL implementation. This PR adds the encoder and decoder interface, key management, and KEM interfaces for ML-KEM and ML-KEM hybrid. Their behavior is subject to change to align with the default OpenSSL behavior when OpenSSL adds their own implementation.

Code points are taken from IETF drafts and are subject to change. These are the same code points used by BoringSSL and the OQS provider. ML-KEM hybrid is implemented according to the latest IETF draft but is subject to changes.

The following algorithms are added:

Algorithm	Code point	OID
mlkem512	0x0200	2.16.840.1.101.3.4.4.1
mlkem768	0x0201	2.16.840.1.101.3.4.4.2
mlkem1024	0x0202	2.16.840.1.101.3.4.4.3
secp256r1mlkem768	0x11eb	2.16.840.1.101.3.4.4.4 (placeholder)
x25519mlkem768	0x11ec	2.16.840.1.101.3.4.4.5 (placeholder)
secp384r1mlkem1024	0x11ed	2.16.840.1.101.3.4.4.6 (placeholder)

draft-connolly-tls-mlkem-key-agreement (ML-KEM standalone usage and code points)
draft-kwiatkowski-tls-ecdhe-mlkem (ML-KEM hybrid implementation and code points)

samuel-lee-msft · 2025-02-26T22:23:29Z

SymCryptProvider/src/p_scossl_base.c

+    TLS_GROUP_ENTRY("mlkem768", SCOSSL_SN_MLKEM768, "MLKEM", scossl_tls_group_info_mlkem768),
+    TLS_GROUP_ENTRY("mlkem1024", SCOSSL_SN_MLKEM1024, "MLKEM", scossl_tls_group_info_mlkem1024),
+    TLS_GROUP_ENTRY("secp256r1mlkem768", SCOSSL_SN_P256_MLKEM768, "MLKEM", scossl_tls_group_info_secp256r1mlkem768),
+    TLS_GROUP_ENTRY("P256mlkem768", SCOSSL_SN_P256_MLKEM768, "MLKEM", scossl_tls_group_info_secp256r1mlkem768),


P256

Just curious if this is definitely names without the "-" in P256?

These strings are only for setting supported groups from configuration as far as I'm aware. Now that OpenSSL has an ML-KEM implementation I'll update these entries to match what they have: https://github.com/openssl/openssl/blob/c2ab75e30a211aa278f8da1f0f040f9368adb81d/providers/common/capabilities.c#L193-L194

samuel-lee-msft · 2025-02-26T22:27:45Z

static const OSSL_PARAM p_scossl_supported_group_list[][11] = {

Not sure why this was 11 before - it looks like TLS_GROUP_ENTRY previously defined 10 OSSL_PARAMs including the OSSL_PARAM_END, and only now defines 11.

Seems error prone to have the magic number.

Refers to: SymCryptProvider/src/p_scossl_base.c:186 in 3b662e7. [](commit_id = 3b662e7, deletion_comment = False)

mlindgren · 2025-02-26T23:41:02Z

ScosslCommon/src/scossl_ecc.c

+    if (curve == NULL)
+    {
+        return 0;
+    }


The _In_ annotation on this function indicates that curve is a required parameter which must not be NULL. In Windows and SymCrypt, we'd typically not do runtime parameter validation here, but would instead rely on assertions and/or PREfast to catch invalid calls.

That said, if this is public function, it probably makes sense to be more defensive. I'm also not sure what the OpenSSL philosophy is regarding input validation.

Fair point. This is an internal function so I can ensure we do the validation outside of this function. I don't think this needs to be changed to _In_opt_

samuel-lee-msft · 2025-02-26T23:43:09Z

SymCryptProvider/src/p_scossl_names.h

+//
+
+// Digests
+#define SCOSSL_ALG_NAME_MD5 SN_md5"SSL3-MD5:1.2.840.113549.2.5"


"

Do we not need ":" separators here? (same for all the new definitions of digest ALG_NAMEs other than CSHAKE)

samuel-lee-msft · 2025-02-26T23:48:06Z

SymCryptProvider/src/p_scossl_names.h

+
+// KDF
+#define SCOSSL_ALG_NAME_HKDF        SN_hkdf
+#define SCOSSL_ALG_NAME_KBKKDF      "KBKDF"


KBKKDF

NP: KBKDF

samuel-lee-msft · 2025-02-26T23:53:48Z

SymCryptProvider/src/p_scossl_base.c

@@ -586,23 +729,15 @@ SCOSSL_STATUS OSSL_provider_init(_In_ const OSSL_CORE_HANDLE *handle,
    {
        SYMCRYPT_MODULE_INIT();
        if (!scossl_dh_init_static() ||
-            !scossl_ecc_init_static())
+            !scossl_ecc_init_static() ||
+            !p_scossl_register_extended_algorithms())
        {
            ERR_raise(ERR_LIB_PROV, ERR_R_INIT_FAIL);


ERR_raise(ERR_LIB_PROV, ERR_R_INIT_FAIL);

Leaks p_ctx, p_ctx->coreBioMeth, and static dlgroups / ecurves.
Probably should call p_scossl_teardown. Consider refactoring away from early returns to cleanup section style.

This was fixed in #104. I'll rebase after the current round of comments.

samuel-lee-msft · 2025-02-27T00:02:59Z

SymCryptProvider/src/p_scossl_base.c

+    }
+
+    p_ctx->handle = handle;
+    p_ctx->libctx = OSSL_LIB_CTX_new_child(handle, in);


p_ctx->libctx

Q: should this be explicitly freed in p_scossl_teardown?

Also addressed in #104 and will be updated with a rebase.

samuel-lee-msft · 2025-02-27T00:08:25Z

SymCryptProvider/src/kem/p_scossl_mlkem.c

+{
+    for (SIZE_T i = 0; i < sizeof(p_scossl_mlkem_groups) / sizeof(SCOSSL_MLKEM_GROUP_INFO); i++)
+    {
+        p_scossl_mlkem_groups[i].nid = OBJ_create(p_scossl_mlkem_groups[i].oid, p_scossl_mlkem_groups[i].groupName, p_scossl_mlkem_groups[i].groupName);


p_scossl_mlkem_groups[i].groupName

NP: Is it correct to pass the shortname into this longname parameter, or should we leave it as NULL?

It's OK for now since it's only used internally, but it should be updated to match the incoming OpenSSL implementation: https://github.com/openssl/openssl/blob/c2ab75e30a211aa278f8da1f0f040f9368adb81d/include/openssl/obj_mac.h#L6575

samuel-lee-msft · 2025-02-27T00:16:54Z

SymCryptProvider/src/p_scossl_bio.c

+                                     size_t *bytes_read)
+{
+    if (core_bio_read_ex == NULL)
+        return 0;


0

NP: SCOSSL_FAILURE / SCOSSL_SUCCESS for returns in this file?

This is just required for supporting encoder/decoder so I probably won't bother with annotating parameters but I think that makes sense.

p_scossl_bio_core_gets, p_scossl_bio_core_puts, and p_scossl_bio_core_ctrl should keep their current return types as their return isn't success/fail.

samuel-lee-msft · 2025-02-27T00:25:51Z

SymCryptProvider/src/p_scossl_bio.c

+        switch (dispatch->function_id)
+        {
+        case OSSL_FUNC_BIO_READ_EX:
+            core_bio_read_ex = (OSSL_FUNC_BIO_read_ex_fn *)dispatch->function;


(OSSL_FUNC_BIO_read_ex_fn *)dispatch->function

NP: OSSL_FUNC_BIO_read_ex(dispatch)?

samuel-lee-msft · 2025-02-27T21:01:10Z

SymCryptProvider/src/decoder/p_scossl_decode_common.c

+    }
+
+    BIO_free(bio);
+    ctx->desc->freeKeyCtx(keyCtx);


NP: Probably doesn't matter, but probably should free keyCtx before bio?

It doesn't really matter but it does look cleaner.

samuel-lee-msft · 2025-02-28T02:42:43Z

SymCryptProvider/src/decoder/p_scossl_decode_common.h

+
+typedef struct scossl_decode_ctx_st
+{
+    SCOSSL_PROVCTX *provctx;


SCOSSL_PROVCTX *provctx;

It seems like we currently have a recipe for potential use-after-free with the number of provider structures keeping a pointer to the single provider context associated with a OSSL_provider_init call. (i.e. can we ever legitimately have a situation where p_scossl_teardown is called while p_scossl_decode is in progress?)

Should we have a refcount-ed provctx, or is OpenSSL guaranteed to ensure that all provider structures are freed before the provider is torn down? Perhaps it's not a huge deal if SCOSSL is the default provider, but could definitely be more of an issue for a place where an application dynamically loads/unloads SCOSSL.

I don't think there's a legitimate case. The OpenSSL default provider doesn't refcount this so we should be ok.

It seems like the providers were originally built with that being a valid use case (which is why we saw use-after-free issues in openssl speed), but it seems like the assumption now is that no provider operations should ever take place after p_scossl_teardown

samuel-lee-msft · 2025-02-28T21:28:30Z

SymCryptProvider/src/p_scossl_bio.c

+        return NULL;
+    }
+
+    if ((bio = BIO_new(provctx->coreBioMeth)) != NULL)


BIO_new(provctx->coreBioMeth)

Should we use:
BIO_new_ex(provctx->libctx, provctx->coreBioMeth)?

Yeah that's probably better.

samuel-lee-msft · 2025-02-28T21:36:12Z

SymCryptProvider/src/p_scossl_ecc.c

+        pbPrivateKey, cbPrivateKey,
+        NULL, 0,
+        numFormat,
+        pointFormat,


pointFormat

NP: Technically this is a don't care value when cbPublicKey is 0 (the private key is just a number, not a point)

samuel-lee-msft · 2025-03-01T00:30:29Z

SymCryptProvider/src/p_scossl_ecc.c

+        copyCtx->isX25519 = keyCtx->isX25519;
+        copyCtx->libctx = keyCtx->libctx;
+        copyCtx->modifiedPrivateBits = keyCtx->modifiedPrivateBits;
+        copyCtx->conversionFormat = keyCtx->conversionFormat;


Huge NP: this should probably only be copied when copying private key material

There might be a weird edge case where a key's conversion format is set, the key is duplicated, and the private key is imported later.

I'm not sure if that's even a valid case but I don't think it hurts to just copy the value since it technically falls under OSSL_KEYMGMT_SELECT_OTHER_PARAMETERS

I think I misclicked when leaving the comment - I meant this to refer to the modifiedPrivateBits field

samuel-lee-msft · 2025-03-01T00:39:45Z

SymCryptProvider/src/p_scossl_ecc.c

+    if (copyCtx != NULL)
+    {
+#ifdef KEYSINUSE_ENABLED
+        copyCtx->keysinuseLock = CRYPTO_THREAD_lock_new();


Should copy isImported in here too?

Yeah, this is missing in RSA too.

mamckee · 2025-03-01T01:15:27Z

static const OSSL_PARAM p_scossl_supported_group_list[][11] = {

Yeah that was easy to miss... I'll pull that out into a constant defined by the TLS_GROUP_ENTRY definition.

In reply to: 2686350183

Refers to: SymCryptProvider/src/p_scossl_base.c:186 in 3b662e7. [](commit_id = 3b662e7, deletion_comment = False)

samuel-lee-msft · 2025-03-01T01:16:01Z

SymCryptProvider/src/p_scossl_ecc.c

+    return SCOSSL_SUCCESS;
+}
+
+SIZE_T p_scossl_ecc_get_max_size(_In_ SCOSSL_ECC_KEY_CTX *keyCtx, BOOL isEcdh)


p_scossl_ecc_get_max_size

NP: name to indicate this is the size of the result of an ECC operation? I was a bit perplexed by what this function was for.

samuel-lee-msft · 2025-03-01T01:21:42Z

SymCryptProvider/src/p_scossl_ecc.c

+    {
+        return SymCryptEcurveSizeofScalarMultiplier(keyCtx->curve);
+    }
+    else if (keyCtx->isX25519)


else

Why else?

samuel-lee-msft · 2025-03-01T01:22:15Z

SymCryptProvider/src/p_scossl_ecc.c

+    }
+
+    cbKey = SymCryptEcurveSizeofFieldElement(keyCtx->curve);
+    if (!keyCtx->isX25519)


if (!keyCtx->isX25519)

Isn't this always true given the above code?

In p_scossl_ecc_get_encoded_public_key we compute the same thing with:

pointFormat = keyCtx->conversionFormat == POINT_CONVERSION_COMPRESSED ? SYMCRYPT_ECPOINT_FORMAT_X : SYMCRYPT_ECPOINT_FORMAT_XY; // Allocate one extra byte for point compression type cbPublicKey = SymCryptEckeySizeofPublicKey(keyCtx->key, pointFormat) + 1;

Which I think is probably a bit clearer

samuel-lee-msft · 2025-03-01T01:42:29Z

SymCryptProvider/src/p_scossl_ecc.c

 {
    SYMCRYPT_NUMBER_FORMAT numFormat;
    SYMCRYPT_ECPOINT_FORMAT pointFormat;
-    PBYTE pbPublicKey, pbPublicKeyStart;
+    PBYTE pbPublicKeyStart = NULL;


pbPublicKeyStart

Just for tracking the logic here, suggest minor renaming:
pbPublicKeyStart -> pbEncodedPublicKey
pbPublicKey -> pbRawPublicKey

And then track cbEncodedPublicKey and cbRawPublicKey?
Tweaking cbPublicKey with ++s and --s seems brittle if new functionality is ever added in here.

samuel-lee-msft · 2025-03-01T01:44:24Z

SymCryptProvider/src/p_scossl_ecc.c

+    ret = SCOSSL_SUCCESS;
+
+cleanup:
+    if (!ret && allocatedKey)


!ret

NP: ret != SCOSSL_SUCCESS

samuel-lee-msft · 2025-03-01T01:50:32Z

SymCryptProvider/src/p_scossl_ecc.c

+    if (keyCtx->key != NULL)
+    {
+        SymCryptEckeyFree(keyCtx->key);
+    }


NP: Prefer to keep SymCryptEckeyFree(keyCtx->key); right next to the SymCryptEckeyAllocate(keyCtx->curve), or set keyCtx->key to NULL after free.
Want to make it harder to introduce a double free in future (i.e. if adding some new parameter validation which goto cleanup between Free and Allocate).

samuel-lee-msft · 2025-03-01T01:57:55Z

SymCryptProvider/src/p_scossl_ecc.c

+
+            // Preserve original bits for export
+            keyCtx->modifiedPrivateBits = pbPrivateKey[0] & 0x07;
+            keyCtx->modifiedPrivateBits |= pbPrivateKey[cbPrivateKey-1] & 0xc0;


cbPrivateKey

cbPrivateKey is still 0 here?

It actually looks like there is no need for cbPrivateKey at all, and you can just use cbEncodedPrivateKey everywhere instead in this function?

samuel-lee-msft · 2025-03-01T01:58:19Z

SymCryptProvider/src/p_scossl_ecc.c

+    // to copy and decode the public key.
+    if (keyCtx->isX25519)
+    {
+        OPENSSL_secure_clear_free(pbPrivateKey, cbPrivateKey);


cbPrivateKey

This should be cbEncodedPrivateKey

mamckee requested review from mlindgren, ccalik and samuel-lee-msft December 30, 2024 22:57

mamckee added 21 commits January 17, 2025 18:23

Boilerplate mlkem kem and keymgmt interfaces

2305e45

ML-KEM key management

09dfe6f

ML-KEM KEM interface

5bc0a0d

Update to latest APIs

b42de06

ML-KEM decoder

abe8cf7

MLKEM PEM and DER encoder. Refactor decoder

f4ba041

Consolidate ML-KEM decoder

8bd3af6

MLKEM text encoder and bugfixes

495b852

Start of hybrid implementation

4ddd8ca

ML-KEM hyrbid key management

78edcf2

Expose classic ECDH functions for ML-KEM hybrid

b6c3c2d

ML-KEM hybrid

600c0d6

Cleanup ecc usage from mlkem keymgmt

62ae32d

Cleanup ML-KEM hybrid and register algorithms

739ce6b

Bugfixes for compatibility with public PQ endpoints

6c5ff94

Fix ordering for non-x25519 hybrid and add P384MLKEM1024

05f1c24

Rebase cleanup

7054d72

Cleanup

e89449f

Cleanup and refactoring

0a98fd5

Fix TLS names

bb3e09b

Use new debug logging for symcrypt errors

3b662e7

mamckee force-pushed the mamckee-ml-kem branch from edb7ed4 to 3b662e7 Compare January 17, 2025 20:28

samuel-lee-msft reviewed Feb 26, 2025

View reviewed changes

mlindgren reviewed Feb 26, 2025

View reviewed changes

samuel-lee-msft reviewed Feb 26, 2025

View reviewed changes

samuel-lee-msft reviewed Feb 27, 2025

View reviewed changes

samuel-lee-msft reviewed Feb 28, 2025

View reviewed changes

samuel-lee-msft reviewed Mar 1, 2025

View reviewed changes

PR comments

82ef0fa

ML-KEM and ML-KEM hybrid #103

Are you sure you want to change the base?

ML-KEM and ML-KEM hybrid #103

Conversation

mamckee commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel-lee-msft commented Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

mamckee Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

samuel-lee-msft Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel-lee-msft Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamckee Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

samuel-lee-msft Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamckee commented Mar 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel-lee-msft commented Feb 26, 2025 •

edited

Loading

mamckee Mar 1, 2025 •

edited

Loading

samuel-lee-msft Feb 26, 2025 •

edited

Loading

samuel-lee-msft Feb 26, 2025 •

edited

Loading

mamckee Mar 1, 2025 •

edited

Loading

samuel-lee-msft Feb 27, 2025 •

edited

Loading