Skip to content

Commit d171b10

Browse files
FrauschiFrauschi
committed
Fixes for ML-DSA
When the input hash data in sized invalidly, the operations now fail with an error. Replace the dynamically-allocated `byte* ctx` pointer in WP11_MldsaParams with an inline `byte ctx[256]` array. PKCS#11 v3.2 (§2.3.12) caps the ML-DSA context length at 255 bytes, so heap allocation is unnecessary and introduced several memory-management hazards: - ctx was freed at the end of WP11_Mldsa_Sign/Verify before session teardown, leaving a dangling pointer if the session was reused - the cleanup in wp11_Session_Final checked the wrong mechanism set, meaning it could free ctx a second time - WP11_Session_SetMldsaParams freed ctx before re-initialising, which was safe only if the pointer was always valid (it wasn't on first call) Embedding the buffer in the struct eliminates all manual lifetime tracking.
1 parent fa81c8f commit d171b10

2 files changed

Lines changed: 76 additions & 30 deletions

File tree

‎src/internal.c‎

Lines changed: 71 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,7 @@ typedef struct WP11_PssParams {
356356
typedef struct WP11_MldsaParams {
357357
enum wc_HashType preHashType;
358358
word32 hedgeType;
359-
byte* ctx;
359+
byte ctx[256];
360360
byte ctxSz;
361361
} WP11_MldsaParams;
362362
#endif
@@ -911,14 +911,6 @@ static void wp11_Session_Final(WP11_Session* session)
911911
session->params.oaep.label = NULL;
912912
}
913913
#endif
914-
#ifdef WOLFPKCS11_MLDSA
915-
if ((session->mechanism == CKM_ML_DSA ||
916-
session->mechanism == CKM_HASH_ML_DSA) &&
917-
session->params.mldsa.ctx != NULL) {
918-
XFREE(session->params.mldsa.ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
919-
session->params.mldsa.ctx = NULL;
920-
}
921-
#endif
922914
#ifndef NO_AES
923915
#ifdef HAVE_AES_CBC
924916
if ((session->mechanism == CKM_AES_CBC ||
@@ -2856,6 +2848,76 @@ int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest)
28562848
break;
28572849
}
28582850
#endif
2851+
#ifdef WOLFPKCS11_MLDSA
2852+
case CKK_ML_DSA: {
2853+
byte* buf = NULL;
2854+
word32 bufSz = 0;
2855+
byte level = 0;
2856+
2857+
/* Get the level from the source key */
2858+
ret = wc_MlDsaKey_GetParams(src->data.mldsaKey, &level);
2859+
2860+
/* Determine raw key size */
2861+
if (ret == 0) {
2862+
if (src->objClass == CKO_PRIVATE_KEY) {
2863+
ret = wc_MlDsaKey_GetPrivLen(src->data.mldsaKey,
2864+
(int*)&bufSz);
2865+
}
2866+
else {
2867+
ret = wc_MlDsaKey_GetPubLen(src->data.mldsaKey,
2868+
(int*)&bufSz);
2869+
}
2870+
}
2871+
if (ret == 0) {
2872+
buf = (byte*)XMALLOC(bufSz, NULL,
2873+
DYNAMIC_TYPE_TMP_BUFFER);
2874+
if (buf == NULL)
2875+
ret = MEMORY_E;
2876+
}
2877+
2878+
/* Export raw key from source */
2879+
if (ret == 0) {
2880+
if (src->objClass == CKO_PRIVATE_KEY) {
2881+
ret = wc_MlDsaKey_ExportPrivRaw(src->data.mldsaKey,
2882+
buf, &bufSz);
2883+
}
2884+
else {
2885+
ret = wc_MlDsaKey_ExportPubRaw(src->data.mldsaKey,
2886+
buf, &bufSz);
2887+
}
2888+
}
2889+
2890+
/* Init destination key and import */
2891+
if (ret == 0) {
2892+
ret = wc_MlDsaKey_Init(dest->data.mldsaKey, NULL,
2893+
dest->devId);
2894+
}
2895+
if (ret == 0) {
2896+
ret = wc_MlDsaKey_SetParams(dest->data.mldsaKey, level);
2897+
if (ret != 0)
2898+
wc_MlDsaKey_Free(dest->data.mldsaKey);
2899+
}
2900+
if (ret == 0) {
2901+
if (src->objClass == CKO_PRIVATE_KEY) {
2902+
ret = wc_MlDsaKey_ImportPrivRaw(dest->data.mldsaKey,
2903+
buf, bufSz);
2904+
}
2905+
else {
2906+
ret = wc_MlDsaKey_ImportPubRaw(dest->data.mldsaKey,
2907+
buf, bufSz);
2908+
}
2909+
if (ret != 0)
2910+
wc_MlDsaKey_Free(dest->data.mldsaKey);
2911+
}
2912+
2913+
if (buf != NULL) {
2914+
wc_ForceZero(buf, bufSz);
2915+
XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER);
2916+
}
2917+
2918+
break;
2919+
}
2920+
#endif
28592921
#ifdef WOLFPKCS11_MLKEM
28602922
case CKK_ML_KEM: {
28612923
byte* buf = NULL;
@@ -7938,7 +8000,6 @@ int WP11_Session_SetMldsaParams(WP11_Session* session, CK_VOID_PTR params,
79388000
int ret = 0;
79398001
WP11_MldsaParams* mldsa = &session->params.mldsa;
79408002

7941-
XFREE(mldsa->ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
79428003
XMEMSET(mldsa, 0, sizeof(*mldsa));
79438004

79448005
if (params != NULL) {
@@ -7952,19 +8013,12 @@ int WP11_Session_SetMldsaParams(WP11_Session* session, CK_VOID_PTR params,
79528013
if (ctx->ulContextLen > 255) {
79538014
ret = BAD_FUNC_ARG;
79548015
}
7955-
if (ret == 0) {
7956-
mldsa->ctx = (byte*)XMALLOC(ctx->ulContextLen, NULL,
7957-
DYNAMIC_TYPE_TMP_BUFFER);
7958-
if (mldsa->ctx == NULL)
7959-
ret = MEMORY_E;
7960-
}
79618016
if (ret == 0) {
79628017
XMEMCPY(mldsa->ctx, ctx->pContext, ctx->ulContextLen);
79638018
mldsa->ctxSz = ctx->ulContextLen;
79648019
}
79658020
}
79668021
else {
7967-
mldsa->ctx = NULL;
79688022
mldsa->ctxSz = 0;
79698023
}
79708024

@@ -7981,19 +8035,12 @@ int WP11_Session_SetMldsaParams(WP11_Session* session, CK_VOID_PTR params,
79818035
if (ctx->ulContextLen > 255) {
79828036
ret = BAD_FUNC_ARG;
79838037
}
7984-
if (ret == 0) {
7985-
mldsa->ctx = (byte*)XMALLOC(ctx->ulContextLen, NULL,
7986-
DYNAMIC_TYPE_TMP_BUFFER);
7987-
if (mldsa->ctx == NULL)
7988-
ret = MEMORY_E;
7989-
}
79908038
if (ret == 0) {
79918039
XMEMCPY(mldsa->ctx, ctx->pContext, ctx->ulContextLen);
79928040
mldsa->ctxSz = ctx->ulContextLen;
79938041
}
79948042
}
79958043
else {
7996-
mldsa->ctx = NULL;
79978044
mldsa->ctxSz = 0;
79988045
}
79998046

@@ -8010,7 +8057,6 @@ int WP11_Session_SetMldsaParams(WP11_Session* session, CK_VOID_PTR params,
80108057
else {
80118058
mldsa->preHashType = WC_HASH_TYPE_NONE;
80128059
mldsa->hedgeType = CKH_HEDGE_PREFERRED;
8013-
mldsa->ctx = NULL;
80148060
mldsa->ctxSz = 0;
80158061
}
80168062

@@ -12952,8 +12998,6 @@ int WP11_Mldsa_Sign(unsigned char* data, word32 dataLen, unsigned char* sig,
1295212998
Rng_Free(&rng);
1295312999
}
1295413000

12955-
XFREE(params->ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
12956-
params->ctx = NULL;
1295713001
params->ctxSz = 0;
1295813002

1295913003
if (priv->onToken)
@@ -13002,8 +13046,6 @@ int WP11_Mldsa_Verify(unsigned char* sig, word32 sigLen, unsigned char* data,
1300213046
}
1300313047
}
1300413048

13005-
XFREE(params->ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
13006-
params->ctx = NULL;
1300713049
params->ctxSz = 0;
1300813050

1300913051
if (pub->onToken)

‎tests/pkcs11v3test.c‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -669,7 +669,11 @@ static CK_RV mldsa_sign_verify(CK_SESSION_HANDLE session, CK_OBJECT_HANDLE privK
669669
}
670670
if (ret == CKR_OK) {
671671
ret = funcList->C_Verify(session, data, dataSz - 1, sig, sigSz);
672-
CHECK_CKR_FAIL(ret, CKR_SIGNATURE_INVALID, "ML-DSA Verify bad data");
672+
if (mech->mechanism == CKM_HASH_ML_DSA) {
673+
CHECK_CKR_FAIL(ret, CKR_FUNCTION_FAILED, "ML-DSA Verify bad data");
674+
} else {
675+
CHECK_CKR_FAIL(ret, CKR_SIGNATURE_INVALID, "ML-DSA Verify bad data");
676+
}
673677
}
674678
if (ret == CKR_OK) {
675679
XMEMCPY(sigBad, sig, sigSz);

0 commit comments

Comments
 (0)