Skip to content

Commit 6deabcd

Browse files
committed
fwtpm: peer-review batch 4 (polish)
Input validation: - NV_DefineSpace: validate nvIndex in [NV_INDEX_FIRST, NV_INDEX_LAST] and reject reserved TPM_NT values - DA lockout allow-list: permit GetCapability, SelfTest, GetRandom, DictionaryAttackLockReset, StartAuthSession, FlushContext during DA - KDFa: guard keySz * 8 against overflow - FwAppendCreationHashAndTicket: bounds check before objName copy Crypto improvements: - ConstantCompare: use word32 accumulator instead of byte - FwDeriveWrapKey: use full parent privKey for HMAC (was truncated to 32 bytes of predictable DER headers for RSA keys) Hardening: - SwTpmConnect: open with O_CLOEXEC|O_NOFOLLOW, verify S_ISCHR - SwTpmTransmit: partial-write retry loop (matches receive side) - sigaction for SIGPIPE in fwtpm_io, fwtpm_tis, fwtpm_main - fwtpm_main: volatile sig_atomic_t for signal flag - TisShmCleanup: check SEM_FAILED (not just NULL) before sem_close - fwtpm_io: 30s select() timeout, connection-replace debug logging
1 parent b0c0bba commit 6deabcd

9 files changed

Lines changed: 123 additions & 37 deletions

File tree

src/fwtpm/fwtpm_command.c

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9597,6 +9597,22 @@ static TPM_RC FwCmd_NV_DefineSpace(FWTPM_CTX* ctx, TPM2_Packet* cmd,
95979597
}
95989598
}
95999599

9600+
/* Validate NV index handle range */
9601+
if (rc == 0) {
9602+
UINT32 nvIdx = publicInfo.nvPublic.nvIndex;
9603+
if (nvIdx < NV_INDEX_FIRST || nvIdx > NV_INDEX_LAST) {
9604+
rc = TPM_RC_NV_RANGE;
9605+
}
9606+
}
9607+
9608+
/* Validate NV type (TPM_NT) — reject reserved values */
9609+
if (rc == 0) {
9610+
UINT32 nt = (publicInfo.nvPublic.attributes & TPMA_NV_TPM_NT) >> 4;
9611+
if (nt > TPM_NT_PIN_PASS && nt != TPM_NT_PIN_FAIL) {
9612+
rc = TPM_RC_ATTRIBUTES;
9613+
}
9614+
}
9615+
96009616
/* Check for duplicate */
96019617
if (rc == 0 && FwFindNvIndex(ctx, publicInfo.nvPublic.nvIndex) != NULL) {
96029618
rc = TPM_RC_NV_DEFINED;
@@ -12557,12 +12573,22 @@ int FWTPM_ProcessCommand(FWTPM_CTX* ctx,
1255712573
}
1255812574
}
1255912575

12560-
/* DA lockout check: reject all auth attempts if lockout threshold exceeded */
12576+
/* DA lockout check: reject auth attempts if lockout threshold exceeded.
12577+
* Per TPM 2.0 spec, certain commands must still be allowed during lockout
12578+
* (e.g., GetCapability, DictionaryAttackLockReset for recovery). */
1256112579
#ifndef FWTPM_NO_DA
1256212580
if (ctx->daFailedTries >= ctx->daMaxTries && ctx->daMaxTries > 0) {
12563-
*rspSize = FwBuildErrorResponse(rspBuf,
12564-
TPM_ST_NO_SESSIONS, TPM_RC_LOCKOUT);
12565-
return TPM_RC_SUCCESS;
12581+
if (cmdCode != TPM_CC_GetCapability &&
12582+
cmdCode != TPM_CC_SelfTest &&
12583+
cmdCode != TPM_CC_GetRandom &&
12584+
cmdCode != TPM_CC_DictionaryAttackLockReset &&
12585+
cmdCode != TPM_CC_DictionaryAttackParameters &&
12586+
cmdCode != TPM_CC_StartAuthSession &&
12587+
cmdCode != TPM_CC_FlushContext) {
12588+
*rspSize = FwBuildErrorResponse(rspBuf,
12589+
TPM_ST_NO_SESSIONS, TPM_RC_LOCKOUT);
12590+
return TPM_RC_SUCCESS;
12591+
}
1256612592
}
1256712593
#endif
1256812594

src/fwtpm/fwtpm_crypto.c

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,9 @@ int FwAppendCreationHashAndTicket(FWTPM_CTX* ctx, TPM2_Packet* rsp,
353353
ticketDataSz = chSz;
354354
}
355355
if (objNameSz > 0) {
356+
if (ticketDataSz + objNameSz > (int)sizeof(ticketData)) {
357+
return TPM_RC_SIZE;
358+
}
356359
XMEMCPY(ticketData + ticketDataSz, objName, objNameSz);
357360
ticketDataSz += objNameSz;
358361
}
@@ -827,10 +830,13 @@ int FwDeriveWrapKey(const FWTPM_Object* parent,
827830

828831
rc = wc_HmacInit(hmac, NULL, INVALID_DEVID);
829832

830-
/* AES key = HMAC-SHA256(parentPriv, "fwTPM-wrap-key") */
833+
/* AES key = HMAC-SHA256(parentPriv, "fwTPM-wrap-key")
834+
* Use full parent private key as HMAC key — HMAC handles arbitrary-length
835+
* keys via internal hashing. The previous 32-byte truncation used
836+
* predictable ASN.1 DER header bytes for RSA keys. */
831837
if (rc == 0) {
832838
rc = wc_HmacSetKey(hmac, WC_SHA256, parent->privKey,
833-
(parent->privKeySize > 32) ? 32 : parent->privKeySize);
839+
parent->privKeySize);
834840
}
835841
if (rc == 0) {
836842
rc = wc_HmacUpdate(hmac, (const byte*)"fwTPM-wrap-key", 14);

src/fwtpm/fwtpm_io.c

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -536,7 +536,13 @@ int FWTPM_IO_ServerLoop(FWTPM_CTX* ctx)
536536
#ifndef _WIN32
537537
/* Ignore SIGPIPE so write to closed socket returns error instead
538538
* of killing the process */
539-
signal(SIGPIPE, SIG_IGN);
539+
{
540+
struct sigaction sa;
541+
sa.sa_handler = SIG_IGN;
542+
sigemptyset(&sa.sa_mask);
543+
sa.sa_flags = 0;
544+
sigaction(SIGPIPE, &sa, NULL);
545+
}
540546
#endif
541547

542548
/* Multiplexed select loop: handles listen sockets AND active client fds
@@ -562,22 +568,35 @@ int FWTPM_IO_ServerLoop(FWTPM_CTX* ctx)
562568
if (platFd > maxFd) maxFd = platFd;
563569
}
564570

565-
if (select(maxFd + 1, &readFds, NULL, NULL, NULL) < 0) {
566-
if (errno == EINTR) {
567-
continue;
571+
{
572+
struct timeval tv;
573+
int selRc;
574+
tv.tv_sec = 30;
575+
tv.tv_usec = 0;
576+
selRc = select(maxFd + 1, &readFds, NULL, NULL, &tv);
577+
if (selRc < 0) {
578+
if (errno == EINTR) {
579+
continue;
580+
}
581+
#ifdef DEBUG_WOLFTPM
582+
printf("fwTPM: select error %d (%s)\n", errno, strerror(errno));
583+
#endif
584+
rc = TPM_RC_FAILURE;
585+
break;
586+
}
587+
if (selRc == 0) {
588+
continue; /* timeout — recheck ctx->running */
568589
}
569-
#ifdef DEBUG_WOLFTPM
570-
printf("fwTPM: select error %d (%s)\n", errno, strerror(errno));
571-
#endif
572-
rc = TPM_RC_FAILURE;
573-
break;
574590
}
575591

576592
/* Accept new platform connection */
577593
if (FD_ISSET(ctx->io.platListenFd, &readFds)) {
578594
int newFd = accept(ctx->io.platListenFd, NULL, NULL);
579595
if (newFd >= 0) {
580596
if (platFd >= 0) {
597+
#ifdef DEBUG_WOLFTPM
598+
printf("fwTPM: platform connection replaced\n");
599+
#endif
581600
close(platFd);
582601
}
583602
platFd = newFd;
@@ -589,6 +608,9 @@ int FWTPM_IO_ServerLoop(FWTPM_CTX* ctx)
589608
int newFd = accept(ctx->io.listenFd, NULL, NULL);
590609
if (newFd >= 0) {
591610
if (cmdFd >= 0) {
611+
#ifdef DEBUG_WOLFTPM
612+
printf("fwTPM: command connection replaced\n");
613+
#endif
592614
close(cmdFd);
593615
}
594616
cmdFd = newFd;

src/fwtpm/fwtpm_main.c

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,16 @@
3838
#include <string.h>
3939
#include <signal.h>
4040

41-
/* Global context pointer for signal handler */
41+
/* Global context pointer for signal handler.
42+
* Only the volatile flag is set in the handler; actual cleanup
43+
* happens in the main loop after the flag is observed. */
44+
static volatile sig_atomic_t g_terminate = 0;
4245
static FWTPM_CTX* g_ctx = NULL;
4346

4447
static void sigterm_handler(int sig)
4548
{
4649
(void)sig;
50+
g_terminate = 1;
4751
if (g_ctx != NULL) {
4852
g_ctx->running = 0;
4953
}
@@ -155,8 +159,14 @@ int main(int argc, char* argv[])
155159

156160
/* Install signal handler for graceful shutdown with NV save */
157161
g_ctx = &ctx;
158-
signal(SIGTERM, sigterm_handler);
159-
signal(SIGINT, sigterm_handler);
162+
{
163+
struct sigaction sa;
164+
sa.sa_handler = sigterm_handler;
165+
sigemptyset(&sa.sa_mask);
166+
sa.sa_flags = 0;
167+
sigaction(SIGTERM, &sa, NULL);
168+
sigaction(SIGINT, &sa, NULL);
169+
}
160170

161171
/* Initialize socket transport */
162172
rc = FWTPM_IO_Init(&ctx);

src/fwtpm/fwtpm_tis.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,13 @@ int FWTPM_TIS_ServerLoop(FWTPM_CTX* ctx)
417417
ctx->running = 1;
418418

419419
#ifndef _WIN32
420-
signal(SIGPIPE, SIG_IGN);
420+
{
421+
struct sigaction sa;
422+
sa.sa_handler = SIG_IGN;
423+
sigemptyset(&sa.sa_mask);
424+
sa.sa_flags = 0;
425+
sigaction(SIGPIPE, &sa, NULL);
426+
}
421427
#endif
422428

423429
printf("fwTPM TIS: Server ready, waiting for register accesses...\n");

src/fwtpm/fwtpm_tis_shm.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,12 +158,12 @@ static void TisShmCleanup(void* ctx)
158158
{
159159
FWTPM_TIS_SHM_CTX* shm = (FWTPM_TIS_SHM_CTX*)ctx;
160160

161-
if (shm->semRsp != NULL) {
161+
if (shm->semRsp != NULL && shm->semRsp != SEM_FAILED) {
162162
sem_close(shm->semRsp);
163163
sem_unlink(FWTPM_TIS_SEM_RSP);
164164
shm->semRsp = NULL;
165165
}
166-
if (shm->semCmd != NULL) {
166+
if (shm->semCmd != NULL && shm->semCmd != SEM_FAILED) {
167167
sem_close(shm->semCmd);
168168
sem_unlink(FWTPM_TIS_SEM_CMD);
169169
shm->semCmd = NULL;

src/tpm2_crypto.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,14 @@ int TPM2_KDFa_ex(
6464
word32 counter = 0;
6565
int hLen, copyLen, lLen = 0;
6666
byte uint32Buf[sizeof(UINT32)];
67-
UINT32 sizeInBits = keySz * 8;
67+
UINT32 sizeInBits;
6868
UINT32 pos;
6969
byte hash[WC_MAX_DIGEST_SIZE];
7070

71-
if (key == NULL) {
71+
if (key == NULL || keySz > (0xFFFFFFFFUL / 8)) {
7272
return BAD_FUNC_ARG;
7373
}
74+
sizeInBits = keySz * 8;
7475

7576
hashType = TPM2_GetHashType(hashAlg);
7677
if (hashType == (int)WC_HASH_TYPE_NONE) {

src/tpm2_swtpm.c

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@
6464
#ifdef WOLFTPM_SWTPM_UART
6565
#include <fcntl.h>
6666
#include <termios.h>
67+
#include <sys/stat.h>
6768
#endif
6869

6970
#include <wolftpm/tpm2_socket.h>
@@ -99,17 +100,23 @@ static TPM_RC SwTpmTransmit(TPM2_CTX* ctx, const void* buffer, ssize_t bufSz)
99100
return BAD_FUNC_ARG;
100101
}
101102

102-
wrc = write(ctx->tcpCtx.fd, buffer, bufSz);
103-
if (bufSz != wrc) {
104-
rc = TPM_RC_FAILURE;
105-
}
106-
107-
#ifdef WOLFTPM_DEBUG_VERBOSE
108-
if (wrc < 0) {
109-
printf("Failed to send the TPM command to fd %d, got errno %d ="
110-
"%s\n", ctx->tcpCtx.fd, errno, strerror(errno));
103+
{
104+
const char* ptr = (const char*)buffer;
105+
int remaining = bufSz;
106+
while (remaining > 0) {
107+
wrc = write(ctx->tcpCtx.fd, ptr, remaining);
108+
if (wrc <= 0) {
109+
#ifdef WOLFTPM_DEBUG_VERBOSE
110+
printf("Failed to send the TPM command to fd %d, got errno %d ="
111+
"%s\n", ctx->tcpCtx.fd, errno, strerror(errno));
112+
#endif
113+
rc = TPM_RC_FAILURE;
114+
break;
115+
}
116+
remaining -= (int)wrc;
117+
ptr += wrc;
118+
}
111119
}
112-
#endif
113120

114121
return rc;
115122
}
@@ -170,13 +177,21 @@ static TPM_RC SwTpmConnect(TPM2_CTX* ctx, const char* host, const char* port)
170177
/* Note: TPM2_SWTPM_HOST env var is checked by caller
171178
* (TPM2_SWTPM_SendCommand) before invoking SwTpmConnect */
172179

173-
fd = open(host, O_RDWR | O_NOCTTY);
180+
fd = open(host, O_RDWR | O_NOCTTY | O_CLOEXEC | O_NOFOLLOW);
174181
if (fd < 0) {
175182
#ifdef DEBUG_WOLFTPM
176183
printf("Failed to open UART device %s: %s\n", host, strerror(errno));
177184
#endif
178185
return TPM_RC_FAILURE;
179186
}
187+
/* Verify the opened path is a character device */
188+
{
189+
struct stat st;
190+
if (fstat(fd, &st) != 0 || !S_ISCHR(st.st_mode)) {
191+
close(fd);
192+
return TPM_RC_FAILURE;
193+
}
194+
}
180195

181196
/* Configure serial port: 8N1, raw mode, no flow control */
182197
XMEMSET(&tty, 0, sizeof(tty));

src/tpm2_util.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,11 @@ int TPM2_GetHashType(TPMI_ALG_HASH hashAlg)
9797
int TPM2_ConstantCompare(const byte* a, const byte* b, word32 len)
9898
{
9999
word32 i;
100-
volatile byte result = 0;
100+
volatile word32 result = 0;
101101
for (i = 0; i < len; i++) {
102-
result |= a[i] ^ b[i];
102+
result |= (word32)(a[i] ^ b[i]);
103103
}
104-
return (int)result;
104+
return (result != 0) ? 1 : 0;
105105
}
106106

107107
/* This routine fills the first len bytes of the memory area pointed by mem

0 commit comments

Comments
 (0)