Update ML-KEM for compatibility with OpenSSL 3.5 implementation #153
Conversation
|
FYI We recently added support for MLKEM to https://github.com/golang-fips/openssl, so you can use it to test that you will be at least compatible with the Microsoft build of Go. #Resolved |
|
|
||
| #define SCOSSL_SN_P384_MLKEM1024 "id-alg-secp384r1-ml-kem-1024" | ||
| #define SCOSSL_LN_P384_MLKEM1024 "P384-ML-KEM-1024" | ||
| #define SCOSSL_OID_P384_MLKEM1024 "2.16.840.1.101.3.4.4.6" |
There was a problem hiding this comment.
This section is curious to me - looking at latest OpenSSL and 3.5, the SN and OIDs for hybrid key exchange do not seem to be defined, also the LN is different.
It seems particularly funky to define these OIDs for hybrids with the NIST algorithm identifiers: https://csrc.nist.gov/projects/computer-security-objects-register/algorithm-registration
NIST owns the 2.16.840.1.101.3 OID space and does not define these hybrids.
i.e.
PROV_NAMES_X25519MLKEM768 is "X25519MLKEM768", not "X25519-ML-KEM-768"
PROV_NAMES_SecP256r1MLKEM768 is "SecP256r1MLKEM768", not "P256-ML-KEM-768"
PROV_NAMES_SecP384r1MLKEM1024 is "SecP384r1MLKEM1024" #Pending
There was a problem hiding this comment.
As far as the OIDs go, since we don't need to encode or decode, we can just remove them. I believe these were the same OIDs that the oqs provider were using as a placeholder, but it looks like no OIDs are on the table for these hybrids anyways.
The SNs are in line with other openssl SNs, and the hybrids are registered with the name that provider name used in the default provider. OpenSSL doesn't define any SNs or NIDs for these algorithms, but having a NID for these is convenient.
The provider name and SN don't always match, but usually both are used as the algorithm name in the dispatch table. I know the order can matter (e.g. for digests, the first name is used for EVP_MD_name, so SHA256 returns "SHA2-256" instead of the SN) but I'm not sure about KEM. I can make the provider name come first in the list to be safe.
| #define SCOSSL_ALG_NAME_MLKEM1024 SCOSSL_LN_MLKEM1024":MLKEM1024:"SCOSSL_SN_MLKEM1024":"SCOSSL_OID_MLKEM1024 | ||
|
|
||
| // TLS ML-KEM hybrids. HPKE uses a different combiner mechanism that will be implemented separately. | ||
| #define SCOSSL_ALG_NAME_X25519_MLKEM768 SCOSSL_LN_X25519_MLKEM768":X25519MLKEM768:"SCOSSL_SN_X25519_MLKEM768":"SCOSSL_OID_X25519_MLKEM768 |
| OSSL_PARAM_END}; | ||
|
|
||
| static const OSSL_PARAM p_scossl_mlkem_hybrid_keymgmt_gettable_param_types[] = { | ||
| OSSL_PARAM_int(OSSL_PKEY_PARAM_SECURITY_BITS, NULL), |
There was a problem hiding this comment.
Missing OSSL_PKEY_PARAM_BITS I think?
I would personally be happy to not expose OSSL_PKEY_PARAM_BITS or OSSL_PKEY_PARAM_SECURITY_BITS from SCOSSL ML-KEM / hybrid-ML-KEM if there's no app-compat implication, the numbers are kind of arbitrary. #Pending
There was a problem hiding this comment.
There were a couple places these were fetched in testing, but I would rather just assume there's an app compat concern and just do the same thing as the default provider. We've had callers depend on all kinds of things we didn't expect and they're simple enough parameters to support.
| const SCOSSL_MLKEM_GROUP_INFO *groupInfo; | ||
| SYMCRYPT_MLKEM_PARAMS mlkemParams; | ||
|
|
||
| BYTE pbSeed[SCOSSL_MLKEM_PRIVATE_SEED_LENGTH]; |
| return SCOSSL_FAILURE; | ||
| } | ||
| if ((p = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_ML_KEM_SEED)) != NULL && | ||
| !OSSL_PARAM_get_octet_string(p, (void **)&pbSeed, SCOSSL_MLKEM_PRIVATE_SEED_LENGTH, &genCtx->cbSeed)) |
There was a problem hiding this comment.
Yes. I must have misread the OpenSSL behavior since it's written a bit weird and thought that the max size was set but not validated. It should fail still in keygen, but it would be better to fail here for consistency.
| { | ||
| SCOSSL_PROV_LOG_SYMCRYPT_ERROR("SymCryptMlKemkeySetValue failed", scError); | ||
| goto cleanup; | ||
| } |
There was a problem hiding this comment.
It's a little odd to me that we stay in a state where the keygen_ctx will always produce the same key after the seed is set once. The OpenSSL documentation is scarce here.
Should we wipe the keygen_ctx's copy of the seed on success here? #Pending
There was a problem hiding this comment.
Yeah, looks like that's what OpenSSL is doing too.
| { | ||
| ERR_raise(ERR_LIB_PROV, PROV_R_FAILED_TO_SET_PARAMETER); | ||
| goto cleanup; | ||
| } |
There was a problem hiding this comment.
I think we have some opportunities for funkiness (spurious failure, use after free, double free, etc.) when multiple parameters are being exported, unless with secure_clear_free and set pbKey/cbKey to NULL/0 on success in these blocks.
p_scossl_mlkem_keymgmt_get_encoded_key can look at stale values in pbKey and cbKey #Pending
There was a problem hiding this comment.
p_scossl_mlkem_keymgmt_export has similar pattern to what we have here, just setting pbKey to NULL after each OPENSSL_secure_clear_free call at the start of the blocks - I think that's fine.
It's honestly a little funky that p_scossl_mlkem_keymgmt_get_encoded_key looks at existing values in pbKey/cbKey rather than unconditionally allocating
There was a problem hiding this comment.
Yeah I think there was a case that needed this in the combined keymgmt but I forgot to remove the check here. The hyrbrid version doesn't check the existing value.
I'll just free these on success too.
| {OSSL_FUNC_KEYMGMT_EXPORT_TYPES, (void (*)(void))p_scossl_mlkem_keymgmt_impexp_types}, | ||
| {OSSL_FUNC_KEYMGMT_IMPORT, (void (*)(void))p_scossl_mlkem_keymgmt_import}, | ||
| {OSSL_FUNC_KEYMGMT_EXPORT, (void (*)(void))p_scossl_mlkem_keymgmt_export}, | ||
| {0, NULL}}; |
There was a problem hiding this comment.
Is this still needed? #Pending
There was a problem hiding this comment.
For OpenSSL 3.3 yes. I think we'll still have some OpenSSL 3.3 testing scenarios until Azure Linux updates to OpenSSL 3.5.
There was a problem hiding this comment.
I'm reviewing on codeflow btw - this comment is asking whether p_scossl_mlkem_keymgmt_functions is still needed now it looks like only p_scossl_mlkem512_keymgmt_functions, p_scossl_mlkem768_keymgmt_functions, p_scossl_mlkem1024_keymgmt_functions are used?
There was a problem hiding this comment.
I see now, sorry I was on github before so I didn't see the comment was pointing here.
| if (paramPrivKey != NULL) | ||
| { | ||
| OPENSSL_secure_clear_free(pbKey, cbKey); | ||
|
|
There was a problem hiding this comment.
Same comment here w.r.t. setting pbKey to NULL #Pending
| goto cleanup; | ||
| } | ||
|
|
||
| // OpenSSL always exports hybrid priate keys as decapsulation keys |
| OPENSSL_secure_clear_free(pbKey, cbKey); | ||
| } | ||
|
|
||
| return SCOSSL_SUCCESS; |
|
|
||
| if (ctx->keyCtx->format != SYMCRYPT_MLKEMKEY_FORMAT_PRIVATE_SEED && | ||
| ctx->keyCtx->format != SYMCRYPT_MLKEMKEY_FORMAT_DECAPSULATION_KEY) | ||
| if (keyCtx->format != SYMCRYPT_MLKEMKEY_FORMAT_PRIVATE_SEED && |
| } | ||
|
|
||
| // Performs ML-KEM encapsulation using the previously initialized context. If | ||
| // this is a hybrid group, then hybrid encapsulation is performed. |
There was a problem hiding this comment.
nit: Performs hybrid ML-KEM encapsulation using the previously initialized context. #Pending
| SCOSSL_ECDH_CTX *classicKeyexchCtx; | ||
| } SCOSSL_MLKEM_HYBRID_CTX; | ||
|
|
||
| static const OSSL_PARAM p_scossl_mlkem_param_types[] = { |
samuel-lee-msft
left a comment
samuel-lee-msft
left a comment
There was a problem hiding this comment.
LGTM modulo feedback!
This PR updates the SymCrypt provider ML-KEM and ML-KEM hybrid to match the behavior of the OpenSSL default provider implementation, added in OpenSSL 3.5. This PR also fixes minor bugs and behavior differences found when testing the SymCrypt provider with OpenSSL 3.5's new ML-KEM tests.