Skip to content

Commit 58682a2

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 e3fdc75 commit 58682a2

3 files changed

Lines changed: 78 additions & 34 deletions

File tree

‎src/crypto.c‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5923,10 +5923,6 @@ CK_RV C_Verify(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pData,
59235923

59245924
ret = WP11_Mldsa_Verify(pSignature, (int)ulSignatureLen, pData,
59255925
(int)ulDataLen, &stat, obj, session);
5926-
if (ret < 0) {
5927-
stat = 0;
5928-
ret = 0;
5929-
}
59305926
break;
59315927
#endif
59325928
#ifndef NO_HMAC

‎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 ||
@@ -2858,6 +2850,76 @@ int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest)
28582850
break;
28592851
}
28602852
#endif
2853+
#ifdef WOLFPKCS11_MLDSA
2854+
case CKK_ML_DSA: {
2855+
byte* buf = NULL;
2856+
word32 bufSz = 0;
2857+
byte level = 0;
2858+
2859+
/* Get the level from the source key */
2860+
ret = wc_MlDsaKey_GetParams(src->data.mldsaKey, &level);
2861+
2862+
/* Determine raw key size */
2863+
if (ret == 0) {
2864+
if (src->objClass == CKO_PRIVATE_KEY) {
2865+
ret = wc_MlDsaKey_GetPrivLen(src->data.mldsaKey,
2866+
(int*)&bufSz);
2867+
}
2868+
else {
2869+
ret = wc_MlDsaKey_GetPubLen(src->data.mldsaKey,
2870+
(int*)&bufSz);
2871+
}
2872+
}
2873+
if (ret == 0) {
2874+
buf = (byte*)XMALLOC(bufSz, NULL,
2875+
DYNAMIC_TYPE_TMP_BUFFER);
2876+
if (buf == NULL)
2877+
ret = MEMORY_E;
2878+
}
2879+
2880+
/* Export raw key from source */
2881+
if (ret == 0) {
2882+
if (src->objClass == CKO_PRIVATE_KEY) {
2883+
ret = wc_MlDsaKey_ExportPrivRaw(src->data.mldsaKey,
2884+
buf, &bufSz);
2885+
}
2886+
else {
2887+
ret = wc_MlDsaKey_ExportPubRaw(src->data.mldsaKey,
2888+
buf, &bufSz);
2889+
}
2890+
}
2891+
2892+
/* Init destination key and import */
2893+
if (ret == 0) {
2894+
ret = wc_MlDsaKey_Init(dest->data.mldsaKey, NULL,
2895+
dest->devId);
2896+
}
2897+
if (ret == 0) {
2898+
ret = wc_MlDsaKey_SetParams(dest->data.mldsaKey, level);
2899+
if (ret != 0)
2900+
wc_MlDsaKey_Free(dest->data.mldsaKey);
2901+
}
2902+
if (ret == 0) {
2903+
if (src->objClass == CKO_PRIVATE_KEY) {
2904+
ret = wc_MlDsaKey_ImportPrivRaw(dest->data.mldsaKey,
2905+
buf, bufSz);
2906+
}
2907+
else {
2908+
ret = wc_MlDsaKey_ImportPubRaw(dest->data.mldsaKey,
2909+
buf, bufSz);
2910+
}
2911+
if (ret != 0)
2912+
wc_MlDsaKey_Free(dest->data.mldsaKey);
2913+
}
2914+
2915+
if (buf != NULL) {
2916+
wc_ForceZero(buf, bufSz);
2917+
XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER);
2918+
}
2919+
2920+
break;
2921+
}
2922+
#endif
28612923
#ifdef WOLFPKCS11_MLKEM
28622924
case CKK_ML_KEM: {
28632925
byte* buf = NULL;
@@ -8025,7 +8087,6 @@ int WP11_Session_SetMldsaParams(WP11_Session* session, CK_VOID_PTR params,
80258087
int ret = 0;
80268088
WP11_MldsaParams* mldsa = &session->params.mldsa;
80278089

8028-
XFREE(mldsa->ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
80298090
XMEMSET(mldsa, 0, sizeof(*mldsa));
80308091

80318092
if (params != NULL) {
@@ -8039,19 +8100,12 @@ int WP11_Session_SetMldsaParams(WP11_Session* session, CK_VOID_PTR params,
80398100
if (ctx->ulContextLen > 255) {
80408101
ret = BAD_FUNC_ARG;
80418102
}
8042-
if (ret == 0) {
8043-
mldsa->ctx = (byte*)XMALLOC(ctx->ulContextLen, NULL,
8044-
DYNAMIC_TYPE_TMP_BUFFER);
8045-
if (mldsa->ctx == NULL)
8046-
ret = MEMORY_E;
8047-
}
80488103
if (ret == 0) {
80498104
XMEMCPY(mldsa->ctx, ctx->pContext, ctx->ulContextLen);
80508105
mldsa->ctxSz = ctx->ulContextLen;
80518106
}
80528107
}
80538108
else {
8054-
mldsa->ctx = NULL;
80558109
mldsa->ctxSz = 0;
80568110
}
80578111

@@ -8068,19 +8122,12 @@ int WP11_Session_SetMldsaParams(WP11_Session* session, CK_VOID_PTR params,
80688122
if (ctx->ulContextLen > 255) {
80698123
ret = BAD_FUNC_ARG;
80708124
}
8071-
if (ret == 0) {
8072-
mldsa->ctx = (byte*)XMALLOC(ctx->ulContextLen, NULL,
8073-
DYNAMIC_TYPE_TMP_BUFFER);
8074-
if (mldsa->ctx == NULL)
8075-
ret = MEMORY_E;
8076-
}
80778125
if (ret == 0) {
80788126
XMEMCPY(mldsa->ctx, ctx->pContext, ctx->ulContextLen);
80798127
mldsa->ctxSz = ctx->ulContextLen;
80808128
}
80818129
}
80828130
else {
8083-
mldsa->ctx = NULL;
80848131
mldsa->ctxSz = 0;
80858132
}
80868133

@@ -8097,7 +8144,6 @@ int WP11_Session_SetMldsaParams(WP11_Session* session, CK_VOID_PTR params,
80978144
else {
80988145
mldsa->preHashType = WC_HASH_TYPE_NONE;
80998146
mldsa->hedgeType = CKH_HEDGE_PREFERRED;
8100-
mldsa->ctx = NULL;
81018147
mldsa->ctxSz = 0;
81028148
}
81038149

@@ -13041,8 +13087,6 @@ int WP11_Mldsa_Sign(unsigned char* data, word32 dataLen, unsigned char* sig,
1304113087
Rng_Free(&rng);
1304213088
}
1304313089

13044-
XFREE(params->ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
13045-
params->ctx = NULL;
1304613090
params->ctxSz = 0;
1304713091

1304813092
if (priv->onToken)
@@ -13091,8 +13135,6 @@ int WP11_Mldsa_Verify(unsigned char* sig, word32 sigLen, unsigned char* data,
1309113135
}
1309213136
}
1309313137

13094-
XFREE(params->ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
13095-
params->ctx = NULL;
1309613138
params->ctxSz = 0;
1309713139

1309813140
if (pub->onToken)

‎tests/pkcs11v3test.c‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -669,7 +669,13 @@ 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+
/* Invalid hash digest size is not allowed, so operation fails */
674+
CHECK_CKR_FAIL(ret, CKR_FUNCTION_FAILED, "ML-DSA Verify bad data");
675+
} else {
676+
/* Invalid data size results in invalid signature*/
677+
CHECK_CKR_FAIL(ret, CKR_SIGNATURE_INVALID, "ML-DSA Verify bad data");
678+
}
673679
}
674680
if (ret == CKR_OK) {
675681
XMEMCPY(sigBad, sig, sigSz);

0 commit comments

Comments
 (0)