Skip to content

Commit 6711755

Browse files
aidangarskeaidangarske
committed
Fix PQC code review issues for v185 support
 - Add TPM2B_MLDSA_SIGNATURE type with proper 4627-byte buffer for ML-DSA-87 signatures instead of reusing TPM2B_MAX_BUFFER (1024 bytes) - Add bounds checking and byte skipping for MLDSA/MLKEM public key parsing in TPM2_Packet_ParsePublic to prevent buffer overflow - Add bounds checking for ML-DSA signature parsing in TPM2_Packet_ParseSignature with proper wire size tracking - Add bounds checking to Encapsulate/Decapsulate response parsing (sharedSecret and ciphertext buffers) - Add negative size validation for contextSz, digestSz, dataSz parameters in wrapper functions: wolfTPM2_SignSequenceStart, wolfTPM2_SignSequenceComplete, wolfTPM2_VerifySequenceStart, wolfTPM2_VerifySequenceComplete, wolfTPM2_SignDigest, wolfTPM2_VerifyDigestSignature - Fix misleading MAX_SIGNATURE_CTX_SIZE comment - this is for domain separation context (255 bytes), not signature size - Change TPMT_PUBLIC size check from assertion to warning for embedded systems compatibility
1 parent 6388241 commit 6711755

6 files changed

Lines changed: 128 additions & 25 deletions

File tree

‎src/tpm2.c‎

Lines changed: 54 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3552,13 +3552,43 @@ TPM_RC TPM2_Encapsulate(Encapsulate_In* in, Encapsulate_Out* out)
35523552
TPM2_Packet_ParseU32(&packet, &paramSz);
35533553
}
35543554

3555-
TPM2_Packet_ParseU16(&packet, &out->sharedSecret.size);
3556-
TPM2_Packet_ParseBytes(&packet, out->sharedSecret.buffer,
3557-
out->sharedSecret.size);
3555+
/* Parse sharedSecret with bounds checking */
3556+
{
3557+
UINT16 wireSize;
3558+
TPM2_Packet_ParseU16(&packet, &wireSize);
3559+
out->sharedSecret.size = wireSize;
3560+
if (out->sharedSecret.size >
3561+
(UINT16)sizeof(out->sharedSecret.buffer)) {
3562+
out->sharedSecret.size =
3563+
(UINT16)sizeof(out->sharedSecret.buffer);
3564+
}
3565+
TPM2_Packet_ParseBytes(&packet, out->sharedSecret.buffer,
3566+
out->sharedSecret.size);
3567+
/* Skip remaining bytes to keep packet aligned */
3568+
if (wireSize > out->sharedSecret.size) {
3569+
TPM2_Packet_ParseBytes(&packet, NULL,
3570+
wireSize - out->sharedSecret.size);
3571+
}
3572+
}
35583573

3559-
TPM2_Packet_ParseU16(&packet, &out->ciphertext.size);
3560-
TPM2_Packet_ParseBytes(&packet, out->ciphertext.buffer,
3561-
out->ciphertext.size);
3574+
/* Parse ciphertext with bounds checking */
3575+
{
3576+
UINT16 wireSize;
3577+
TPM2_Packet_ParseU16(&packet, &wireSize);
3578+
out->ciphertext.size = wireSize;
3579+
if (out->ciphertext.size >
3580+
(UINT16)sizeof(out->ciphertext.buffer)) {
3581+
out->ciphertext.size =
3582+
(UINT16)sizeof(out->ciphertext.buffer);
3583+
}
3584+
TPM2_Packet_ParseBytes(&packet, out->ciphertext.buffer,
3585+
out->ciphertext.size);
3586+
/* Skip remaining bytes to keep packet aligned */
3587+
if (wireSize > out->ciphertext.size) {
3588+
TPM2_Packet_ParseBytes(&packet, NULL,
3589+
wireSize - out->ciphertext.size);
3590+
}
3591+
}
35623592
}
35633593

35643594
TPM2_ReleaseLock(ctx);
@@ -3600,9 +3630,24 @@ TPM_RC TPM2_Decapsulate(Decapsulate_In* in, Decapsulate_Out* out)
36003630

36013631
TPM2_Packet_ParseU32(&packet, &paramSz);
36023632

3603-
TPM2_Packet_ParseU16(&packet, &out->sharedSecret.size);
3604-
TPM2_Packet_ParseBytes(&packet, out->sharedSecret.buffer,
3605-
out->sharedSecret.size);
3633+
/* Parse sharedSecret with bounds checking */
3634+
{
3635+
UINT16 wireSize;
3636+
TPM2_Packet_ParseU16(&packet, &wireSize);
3637+
out->sharedSecret.size = wireSize;
3638+
if (out->sharedSecret.size >
3639+
(UINT16)sizeof(out->sharedSecret.buffer)) {
3640+
out->sharedSecret.size =
3641+
(UINT16)sizeof(out->sharedSecret.buffer);
3642+
}
3643+
TPM2_Packet_ParseBytes(&packet, out->sharedSecret.buffer,
3644+
out->sharedSecret.size);
3645+
/* Skip remaining bytes to keep packet aligned */
3646+
if (wireSize > out->sharedSecret.size) {
3647+
TPM2_Packet_ParseBytes(&packet, NULL,
3648+
wireSize - out->sharedSecret.size);
3649+
}
3650+
}
36063651
}
36073652

36083653
TPM2_ReleaseLock(ctx);

‎src/tpm2_packet.c‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -866,21 +866,39 @@ void TPM2_Packet_ParsePublic(TPM2_Packet* packet, TPM2B_PUBLIC* pub)
866866
#ifdef WOLFTPM_V185
867867
case TPM_ALG_MLDSA:
868868
case TPM_ALG_HASH_MLDSA:
869-
TPM2_Packet_ParseU16(packet, &pub->publicArea.unique.mldsa.size);
869+
{
870+
UINT16 wireSize;
871+
TPM2_Packet_ParseU16(packet, &wireSize);
872+
pub->publicArea.unique.mldsa.size = wireSize;
870873
if (pub->publicArea.unique.mldsa.size > MAX_MLDSA_PUB_SIZE) {
871874
pub->publicArea.unique.mldsa.size = MAX_MLDSA_PUB_SIZE;
872875
}
873876
TPM2_Packet_ParseBytes(packet, pub->publicArea.unique.mldsa.buffer,
874877
pub->publicArea.unique.mldsa.size);
878+
/* Skip remaining bytes to keep packet position synchronized */
879+
if (wireSize > pub->publicArea.unique.mldsa.size) {
880+
TPM2_Packet_ParseBytes(packet, NULL,
881+
wireSize - pub->publicArea.unique.mldsa.size);
882+
}
875883
break;
884+
}
876885
case TPM_ALG_MLKEM:
877-
TPM2_Packet_ParseU16(packet, &pub->publicArea.unique.mlkem.size);
886+
{
887+
UINT16 wireSize;
888+
TPM2_Packet_ParseU16(packet, &wireSize);
889+
pub->publicArea.unique.mlkem.size = wireSize;
878890
if (pub->publicArea.unique.mlkem.size > MAX_MLKEM_PUB_SIZE) {
879891
pub->publicArea.unique.mlkem.size = MAX_MLKEM_PUB_SIZE;
880892
}
881893
TPM2_Packet_ParseBytes(packet, pub->publicArea.unique.mlkem.buffer,
882894
pub->publicArea.unique.mlkem.size);
895+
/* Skip remaining bytes to keep packet position synchronized */
896+
if (wireSize > pub->publicArea.unique.mlkem.size) {
897+
TPM2_Packet_ParseBytes(packet, NULL,
898+
wireSize - pub->publicArea.unique.mlkem.size);
899+
}
883900
break;
901+
}
884902
#endif /* WOLFTPM_V185 */
885903
default:
886904
/* TPMS_DERIVE derive; ? */
@@ -1004,9 +1022,20 @@ void TPM2_Packet_ParseSignature(TPM2_Packet* packet, TPMT_SIGNATURE* sig)
10041022
case TPM_ALG_MLDSA:
10051023
case TPM_ALG_HASH_MLDSA:
10061024
TPM2_Packet_ParseU16(packet, &sig->signature.mldsa.hash);
1007-
TPM2_Packet_ParseU16(packet, &sig->signature.mldsa.signature.size);
1025+
TPM2_Packet_ParseU16(packet, &wireSize);
1026+
sig->signature.mldsa.signature.size = wireSize;
1027+
if (sig->signature.mldsa.signature.size >
1028+
sizeof(sig->signature.mldsa.signature.buffer)) {
1029+
sig->signature.mldsa.signature.size =
1030+
sizeof(sig->signature.mldsa.signature.buffer);
1031+
}
10081032
TPM2_Packet_ParseBytes(packet, sig->signature.mldsa.signature.buffer,
10091033
sig->signature.mldsa.signature.size);
1034+
/* Skip remaining bytes to keep packet position synchronized */
1035+
if (wireSize > sig->signature.mldsa.signature.size) {
1036+
TPM2_Packet_ParseBytes(packet, NULL,
1037+
wireSize - sig->signature.mldsa.signature.size);
1038+
}
10101039
break;
10111040
#endif /* WOLFTPM_V185 */
10121041
default:

‎src/tpm2_wrap.c‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4483,9 +4483,12 @@ int wolfTPM2_SignSequenceStart(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* key,
44834483
return BAD_FUNC_ARG;
44844484
}
44854485

4486-
if (contextSz > (int)sizeof(signSeqStartIn.context.buffer)) {
4486+
if (contextSz < 0 || contextSz > (int)sizeof(signSeqStartIn.context.buffer)) {
44874487
return BUFFER_E;
44884488
}
4489+
if (contextSz > 0 && context == NULL) {
4490+
return BAD_FUNC_ARG;
4491+
}
44894492

44904493
/* set session auth for key */
44914494
wolfTPM2_SetAuthHandle(dev, 0, &key->handle);
@@ -4542,9 +4545,12 @@ int wolfTPM2_SignSequenceComplete(WOLFTPM2_DEV* dev,
45424545
return BAD_FUNC_ARG;
45434546
}
45444547

4545-
if (dataSz > (int)sizeof(signSeqCompleteIn.buffer.buffer)) {
4548+
if (dataSz < 0 || dataSz > (int)sizeof(signSeqCompleteIn.buffer.buffer)) {
45464549
return BUFFER_E;
45474550
}
4551+
if (dataSz > 0 && data == NULL) {
4552+
return BAD_FUNC_ARG;
4553+
}
45484554

45494555
/* set session auth for key */
45504556
wolfTPM2_SetAuthHandle(dev, 0, &key->handle);
@@ -4619,9 +4625,12 @@ int wolfTPM2_VerifySequenceStart(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* key,
46194625
return BAD_FUNC_ARG;
46204626
}
46214627

4622-
if (contextSz > (int)sizeof(verifySeqStartIn.context.buffer)) {
4628+
if (contextSz < 0 || contextSz > (int)sizeof(verifySeqStartIn.context.buffer)) {
46234629
return BUFFER_E;
46244630
}
4631+
if (contextSz > 0 && context == NULL) {
4632+
return BAD_FUNC_ARG;
4633+
}
46254634

46264635
XMEMSET(&verifySeqStartIn, 0, sizeof(verifySeqStartIn));
46274636
verifySeqStartIn.keyHandle = key->handle.hndl;
@@ -4676,9 +4685,12 @@ int wolfTPM2_VerifySequenceComplete(WOLFTPM2_DEV* dev,
46764685
return BAD_FUNC_ARG;
46774686
}
46784687

4679-
if (dataSz > (int)sizeof(verifySeqCompleteIn.buffer.buffer)) {
4688+
if (dataSz < 0 || dataSz > (int)sizeof(verifySeqCompleteIn.buffer.buffer)) {
46804689
return BUFFER_E;
46814690
}
4691+
if (dataSz > 0 && data == NULL) {
4692+
return BAD_FUNC_ARG;
4693+
}
46824694

46834695
XMEMSET(&verifySeqCompleteIn, 0, sizeof(verifySeqCompleteIn));
46844696
verifySeqCompleteIn.sequenceHandle = sequenceHandle;
@@ -4793,13 +4805,16 @@ int wolfTPM2_SignDigest(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* key,
47934805
return BAD_FUNC_ARG;
47944806
}
47954807

4796-
if (digestSz > (int)sizeof(signDigestIn.digest.buffer)) {
4808+
if (digestSz <= 0 || digestSz > (int)sizeof(signDigestIn.digest.buffer)) {
47974809
return BUFFER_E;
47984810
}
47994811

4800-
if (contextSz > (int)sizeof(signDigestIn.context.buffer)) {
4812+
if (contextSz < 0 || contextSz > (int)sizeof(signDigestIn.context.buffer)) {
48014813
return BUFFER_E;
48024814
}
4815+
if (contextSz > 0 && context == NULL) {
4816+
return BAD_FUNC_ARG;
4817+
}
48034818

48044819
/* set session auth for key */
48054820
wolfTPM2_SetAuthHandle(dev, 0, &key->handle);
@@ -4877,13 +4892,16 @@ int wolfTPM2_VerifyDigestSignature(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* key,
48774892
return BAD_FUNC_ARG;
48784893
}
48794894

4880-
if (digestSz > (int)sizeof(verifyDigestSigIn.digest.buffer)) {
4895+
if (digestSz <= 0 || digestSz > (int)sizeof(verifyDigestSigIn.digest.buffer)) {
48814896
return BUFFER_E;
48824897
}
48834898

4884-
if (contextSz > (int)sizeof(verifyDigestSigIn.context.buffer)) {
4899+
if (contextSz < 0 || contextSz > (int)sizeof(verifyDigestSigIn.context.buffer)) {
48854900
return BUFFER_E;
48864901
}
4902+
if (contextSz > 0 && context == NULL) {
4903+
return BAD_FUNC_ARG;
4904+
}
48874905

48884906
XMEMSET(&verifyDigestSigIn, 0, sizeof(verifyDigestSigIn));
48894907
verifyDigestSigIn.keyHandle = key->handle.hndl;

‎tests/unit_tests.c‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1436,8 +1436,11 @@ static void test_wolfTPM2_PQC_Sizes(void)
14361436

14371437
/* Verify TPMT_PUBLIC size is reasonable for embedded targets */
14381438
printf(" TPMT_PUBLIC size with PQC: %zu bytes\n", sizeof(TPMT_PUBLIC));
1439-
/* Flag if > 5KB, which could blow embedded stacks */
1440-
AssertTrue(sizeof(TPMT_PUBLIC) < 5120);
1439+
/* Warn if > 5KB, which could be large for embedded stacks */
1440+
if (sizeof(TPMT_PUBLIC) >= 5120) {
1441+
printf(" WARNING: TPMT_PUBLIC size (%zu bytes) may be large for "
1442+
"embedded stacks\n", sizeof(TPMT_PUBLIC));
1443+
}
14411444

14421445
/* Verify key buffer sizes are correct */
14431446
AssertIntEQ(MAX_MLDSA_PUB_SIZE, 2592); /* ML-DSA-87 */

‎wolftpm/tpm2.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -972,6 +972,12 @@ typedef struct TPM2B_PRIVATE_KEY_MLKEM {
972972
UINT16 size; /* shall be 64 */
973973
BYTE buffer[MAX_MLKEM_PRIV_SEED_SIZE]; /* 64-byte private seed (d||z) */
974974
} TPM2B_PRIVATE_KEY_MLKEM;
975+
976+
/* TPM2B_MLDSA_SIGNATURE - ML-DSA signature (up to 4627 bytes for ML-DSA-87) */
977+
typedef struct TPM2B_MLDSA_SIGNATURE {
978+
UINT16 size;
979+
BYTE buffer[MAX_MLDSA_SIG_SIZE];
980+
} TPM2B_MLDSA_SIGNATURE;
975981
#endif /* WOLFTPM_V185 */
976982

977983

@@ -1469,7 +1475,7 @@ typedef TPMS_SIGNATURE_ECC TPMS_SIGNATURE_ECDAA;
14691475
/* ML-DSA (Dilithium) Signature Structure */
14701476
typedef struct TPMS_SIGNATURE_ML_DSA {
14711477
TPMI_ALG_HASH hash;
1472-
TPM2B_MAX_BUFFER signature; /* ML-DSA signature is variable length */
1478+
TPM2B_MLDSA_SIGNATURE signature; /* ML-DSA signature up to 4627 bytes */
14731479
} TPMS_SIGNATURE_ML_DSA;
14741480
#endif /* WOLFTPM_V185 */
14751481

‎wolftpm/tpm2_types.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -725,9 +725,11 @@ typedef int64_t INT64;
725725
#define MAX_MLKEM_PRIV_SEED_SIZE 64 /* Private seed (d||z) */
726726
#endif
727727

728-
/* CRITICAL: MAX_SIGNATURE_CTX_SIZE must support ML-DSA-87 signatures (4627 bytes) */
728+
/* MAX_SIGNATURE_CTX_SIZE is for the domain separation context parameter
729+
* passed to ML-DSA sign/verify operations. Set large enough for general use.
730+
* Note: ML-DSA signatures themselves can be up to 4627 bytes (ML-DSA-87). */
729731
#ifndef MAX_SIGNATURE_CTX_SIZE
730-
#define MAX_SIGNATURE_CTX_SIZE MAX_MLDSA_SIG_SIZE /* 4627 */
732+
#define MAX_SIGNATURE_CTX_SIZE 255 /* Domain separation context max */
731733
#endif
732734

733735
#ifndef MAX_KEM_CIPHERTEXT_SIZE

0 commit comments

Comments
 (0)