Skip to content

Commit 3932343

Browse files
authored
Merge pull request wolfSSL#721 from danielinux/buffer-hardening
Multiple fixes: buffer bound checks
2 parents 1c756b3 + ff00406 commit 3932343

File tree

22 files changed

+1336
-66
lines changed

22 files changed

+1336
-66
lines changed

.github/workflows/test-library.yml

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,6 @@ jobs:
3333
asym: [ed25519, ecc256, ecc384, ecc521, rsa2048, rsa3072, rsa4096, ed448]
3434
hash: [sha256, sha384, sha3]
3535

36-
# See https://github.com/wolfSSL/wolfBoot/issues/614 regarding exclusions:
37-
exclude:
38-
- math: "SPMATH=1 WOLFBOOT_SMALL_STACK=1"
39-
- math: "SPMATHALL=1 WOLFBOOT_SMALL_STACK=1"
40-
4136
steps:
4237
- uses: actions/checkout@v4
4338
with:
@@ -201,4 +196,3 @@ jobs:
201196
fi
202197
203198
echo "Got expected non-zero exit and 'Failure' message."
204-

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -612,7 +612,7 @@ include/target.h: $(TARGET_H_TEMPLATE) FORCE
612612
sed -e "s/@WOLFBOOT_DTS_UPDATE_ADDRESS@/$(WOLFBOOT_DTS_UPDATE_ADDRESS)/g" | \
613613
sed -e "s/@WOLFBOOT_LOAD_ADDRESS@/$(WOLFBOOT_LOAD_ADDRESS)/g" | \
614614
sed -e "s/@WOLFBOOT_LOAD_DTS_ADDRESS@/$(WOLFBOOT_LOAD_DTS_ADDRESS)/g" | \
615-
sed -e "s/@WOLFBOOT_RAMBOOT_MAX_SIZE@/$(WOLFBOOT_RAMBOOT_MAX_SIZE)/g" | \
615+
sed -e "s|@WOLFBOOT_RAMBOOT_MAX_SIZE_DEFINE@|$(if $(strip $(WOLFBOOT_RAMBOOT_MAX_SIZE)),#define WOLFBOOT_RAMBOOT_MAX_SIZE $(WOLFBOOT_RAMBOOT_MAX_SIZE),/* WOLFBOOT_RAMBOOT_MAX_SIZE undefined */)|g" | \
616616
sed -e "s/@WOLFBOOT_PARTITION_SELF_HEADER_ADDRESS@/$(WOLFBOOT_PARTITION_SELF_HEADER_ADDRESS)/g" \
617617
> $@
618618

docs/TPM.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ int wolfBoot_unseal_auth(const uint8_t* pubkey_hint, const uint8_t* policy, uint
4747
int index, uint8_t* secret, int* secret_sz, const byte* auth, int authSz);
4848
```
4949
50+
For `wolfBoot_unseal_auth()`, `*secret_sz` is an in/out parameter: set it to the
51+
capacity of `secret` before the call, and on success it is updated to the
52+
number of bytes unsealed.
53+
5054
By default this index will be based on an NV Index at `(0x01400300 + index)`.
5155
The default NV base can be overridden with `WOLFBOOT_TPM_SEAL_NV_BASE`.
5256

hal/stm32h7.c

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323

2424
static uint32_t stm32h7_cache[STM32H7_WORD_SIZE / sizeof(uint32_t)];
2525

26+
#define FLASH_BANK_1 0
27+
#define FLASH_BANK_2 1
28+
2629
static void RAMFUNCTION flash_set_waitstates(unsigned int waitstates)
2730
{
2831
uint32_t reg = FLASH_ACR;
@@ -38,7 +41,7 @@ static RAMFUNCTION void flash_wait_last(void)
3841

3942
static RAMFUNCTION void flash_wait_complete(uint8_t bank)
4043
{
41-
if (bank == 0) {
44+
if (bank == FLASH_BANK_1) {
4245
while ((FLASH_SR1 & FLASH_SR_QW) == FLASH_SR_QW);
4346
}
4447
else {
@@ -48,7 +51,7 @@ static RAMFUNCTION void flash_wait_complete(uint8_t bank)
4851

4952
static void RAMFUNCTION flash_clear_errors(uint8_t bank)
5053
{
51-
if (bank == 0) {
54+
if (bank == FLASH_BANK_1) {
5255
FLASH_SR1 |= (FLASH_SR_WRPERR | FLASH_SR_PGSERR | FLASH_SR_STRBERR |
5356
FLASH_SR_INCERR | FLASH_SR_OPERR | FLASH_SR_RDPERR |
5457
FLASH_SR_RDSERR | FLASH_SR_SNECCERR | FLASH_SR_DBECCERR);
@@ -62,7 +65,7 @@ static void RAMFUNCTION flash_clear_errors(uint8_t bank)
6265

6366
static void RAMFUNCTION flash_program_on(uint8_t bank)
6467
{
65-
if (bank == 0) {
68+
if (bank == FLASH_BANK_1) {
6669
FLASH_CR1 |= FLASH_CR_PG;
6770
while ((FLASH_CR1 & FLASH_CR_PG) == 0)
6871
;
@@ -76,7 +79,7 @@ static void RAMFUNCTION flash_program_on(uint8_t bank)
7679

7780
static void RAMFUNCTION flash_program_off(uint8_t bank)
7881
{
79-
if (bank == 0) {
82+
if (bank == FLASH_BANK_1) {
8083
FLASH_CR1 &= ~FLASH_CR_PG;
8184
}
8285
else {
@@ -88,22 +91,22 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len)
8891
{
8992
int i = 0, ii =0;
9093
uint32_t *src, *dst;
91-
uint8_t bank=0;
94+
uint8_t bank = FLASH_BANK_1;
9295
uint8_t *vbytes = (uint8_t *)(stm32h7_cache);
9396
int off;
9497
uint32_t base_addr;
9598

9699
if ((address & FLASH_BANK2_BASE_REL) != 0) {
97-
bank = 1;
100+
bank = FLASH_BANK_2;
98101
}
99102

100103
while (i < len) {
101104
if ((len - i > 32) && ((((address + i) & 0x1F) == 0) &&
102105
((((uint32_t)data) + i) & 0x1F) == 0))
103106
{
104107
flash_wait_last();
105-
flash_clear_errors(0);
106-
flash_clear_errors(1);
108+
flash_clear_errors(FLASH_BANK_1);
109+
flash_clear_errors(FLASH_BANK_2);
107110
flash_program_on(bank);
108111
flash_wait_complete(bank);
109112
src = (uint32_t *)(data + i);
@@ -140,8 +143,8 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len)
140143

141144
/* Actual write from cache to FLASH */
142145
flash_wait_last();
143-
flash_clear_errors(0);
144-
flash_clear_errors(1);
146+
flash_clear_errors(FLASH_BANK_1);
147+
flash_clear_errors(FLASH_BANK_2);
145148
flash_program_on(bank);
146149
flash_wait_complete(bank);
147150
ISB();
@@ -160,7 +163,7 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len)
160163

161164
void RAMFUNCTION hal_flash_unlock(void)
162165
{
163-
flash_wait_complete(1);
166+
flash_wait_complete(FLASH_BANK_1);
164167
if ((FLASH_CR1 & FLASH_CR_LOCK) != 0) {
165168
FLASH_KEYR1 = FLASH_KEY1;
166169
DMB();
@@ -170,7 +173,7 @@ void RAMFUNCTION hal_flash_unlock(void)
170173
;
171174
}
172175

173-
flash_wait_complete(2);
176+
flash_wait_complete(FLASH_BANK_2);
174177
if ((FLASH_CR2 & FLASH_CR_LOCK) != 0) {
175178
FLASH_KEYR2 = FLASH_KEY1;
176179
DMB();
@@ -183,11 +186,11 @@ void RAMFUNCTION hal_flash_unlock(void)
183186

184187
void RAMFUNCTION hal_flash_lock(void)
185188
{
186-
flash_wait_complete(1);
189+
flash_wait_complete(FLASH_BANK_1);
187190
if ((FLASH_CR1 & FLASH_CR_LOCK) == 0)
188191
FLASH_CR1 |= FLASH_CR_LOCK;
189192

190-
flash_wait_complete(2);
193+
flash_wait_complete(FLASH_BANK_2);
191194
if ((FLASH_CR2 & FLASH_CR_LOCK) == 0)
192195
FLASH_CR2 |= FLASH_CR_LOCK;
193196
}
@@ -211,7 +214,7 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len)
211214
(((p >> 17) << FLASH_CR_SNB_SHIFT) | FLASH_CR_SER | 0x00);
212215
DMB();
213216
FLASH_CR1 |= FLASH_CR_STRT;
214-
flash_wait_complete(1);
217+
flash_wait_complete(FLASH_BANK_1);
215218
}
216219
if ((p>= FLASH_BANK2_BASE_REL) &&
217220
(p <= (FLASH_TOP - FLASHMEM_ADDRESS_SPACE))) {
@@ -222,7 +225,7 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len)
222225
(((p >> 17) << FLASH_CR_SNB_SHIFT) | FLASH_CR_SER | 0x00);
223226
DMB();
224227
FLASH_CR2 |= FLASH_CR_STRT;
225-
flash_wait_complete(2);
228+
flash_wait_complete(FLASH_BANK_2);
226229
}
227230
}
228231
return 0;
@@ -619,4 +622,3 @@ int hal_flash_otp_read(uint32_t flashAddress, void* data, uint32_t length)
619622
}
620623

621624
#endif /* FLASH_OTP_KEYSTORE */
622-

include/target.h.in

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@
118118
#endif
119119

120120
/* Optional RAM-boot image size cap for targets without partitions */
121-
#define WOLFBOOT_RAMBOOT_MAX_SIZE @WOLFBOOT_RAMBOOT_MAX_SIZE@
121+
@WOLFBOOT_RAMBOOT_MAX_SIZE_DEFINE@
122122
#define WOLFBOOT_LOAD_DTS_ADDRESS @WOLFBOOT_LOAD_DTS_ADDRESS@
123123

124124

src/image.c

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,6 +1143,7 @@ static void key_sha384(uint8_t key_slot, uint8_t *hash)
11431143
int pubkey_sz = keystore_get_size(key_slot);
11441144
wc_Sha384 sha384_ctx;
11451145

1146+
memset(hash, 0, SHA384_DIGEST_SIZE);
11461147
if (!pubkey || (pubkey_sz < 0))
11471148
return;
11481149

@@ -1237,6 +1238,7 @@ static void key_sha3_384(uint8_t key_slot, uint8_t *hash)
12371238
int pubkey_sz = keystore_get_size(key_slot);
12381239
wc_Sha3 sha3_ctx;
12391240

1241+
memset(hash, 0, WC_SHA3_384_DIGEST_SIZE);
12401242
if (!pubkey || (pubkey_sz < 0))
12411243
return;
12421244
wc_InitSha3_384(&sha3_ctx, NULL, INVALID_DEVID);
@@ -1306,9 +1308,9 @@ int wolfBoot_open_image_address(struct wolfBoot_image *img, uint8_t *image)
13061308

13071309
#ifdef WOLFBOOT_FIXED_PARTITIONS
13081310
if (img->fw_size > (WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE)) {
1309-
wolfBoot_printf("Image size %d > max %d\n",
1311+
wolfBoot_printf("Image size %u > max %u\n",
13101312
(unsigned int)img->fw_size,
1311-
(WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE));
1313+
(unsigned int)(WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE));
13121314
img->fw_size = WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE;
13131315
return -1;
13141316
}
@@ -1317,6 +1319,14 @@ int wolfBoot_open_image_address(struct wolfBoot_image *img, uint8_t *image)
13171319
}
13181320
img->trailer = img->hdr + WOLFBOOT_PARTITION_SIZE;
13191321
#else
1322+
#ifdef WOLFBOOT_RAMBOOT_MAX_SIZE
1323+
if (img->fw_size > WOLFBOOT_RAMBOOT_MAX_SIZE) {
1324+
wolfBoot_printf("Image size %u > max %u\n",
1325+
(unsigned int)img->fw_size,
1326+
(unsigned int)WOLFBOOT_RAMBOOT_MAX_SIZE);
1327+
return -1;
1328+
}
1329+
#endif
13201330
if (img->hdr == NULL) {
13211331
img->hdr = image;
13221332
}

src/libwolfboot.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,8 @@ static int RAMFUNCTION partition_magic_write(uint8_t part, uintptr_t addr)
374374
XMEMCPY(NVM_CACHE, (void*)addr_read, NVM_CACHE_SIZE);
375375
XMEMCPY(NVM_CACHE + off, &wolfboot_magic_trail, sizeof(uint32_t));
376376
ret = hal_flash_write(addr_write, NVM_CACHE, WOLFBOOT_SECTOR_SIZE);
377+
if (ret != 0)
378+
return ret;
377379
nvm_cached_sector = !nvm_cached_sector;
378380
ret = hal_flash_erase(addr_read, WOLFBOOT_SECTOR_SIZE);
379381
return ret;

src/string.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,9 @@ char *strcat(char *dest, const char *src)
9898
{
9999
size_t i = 0;
100100
size_t j = strlen(dest);
101+
size_t src_len = strlen(src);
101102

102-
for (i = 0; i < strlen(src); i++) {
103+
for (i = 0; i < src_len; i++) {
103104
dest[j++] = src[i];
104105
}
105106
dest[j] = '\0';
@@ -186,6 +187,10 @@ char *strncpy(char *dst, const char *src, size_t n)
186187
break;
187188
}
188189

190+
while (++i < n) {
191+
dst[i] = '\0';
192+
}
193+
189194
return dst;
190195
}
191196

src/tpm.c

Lines changed: 52 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,19 @@ WOLFTPM2_KEY wolftpm_srk;
4444
#endif
4545

4646
#if defined(WOLFBOOT_TPM_SEAL) || defined(WOLFBOOT_TPM_KEYSTORE)
47+
static int wolfBoot_constant_compare(const uint8_t* a, const uint8_t* b,
48+
uint32_t len)
49+
{
50+
uint32_t i;
51+
uint8_t diff = 0;
52+
53+
for (i = 0; i < len; i++) {
54+
diff |= a[i] ^ b[i];
55+
}
56+
57+
return diff;
58+
}
59+
4760
void wolfBoot_print_hexstr(const unsigned char* bin, unsigned long sz,
4861
unsigned long maxLine)
4962
{
@@ -605,6 +618,8 @@ int wolfBoot_store_blob(TPMI_RH_NV_AUTH authHandle, uint32_t nvIndex,
605618
if (authSz > 0) {
606619
if (auth == NULL)
607620
return BAD_FUNC_ARG;
621+
if (authSz > sizeof(nv.handle.auth.buffer))
622+
return BAD_FUNC_ARG;
608623
nv.handle.auth.size = authSz;
609624
memcpy(nv.handle.auth.buffer, auth, authSz);
610625
}
@@ -685,6 +700,8 @@ int wolfBoot_read_blob(uint32_t nvIndex, WOLFTPM2_KEYBLOB* blob,
685700
if (authSz > 0) {
686701
if (auth == NULL)
687702
return BAD_FUNC_ARG;
703+
if (authSz > sizeof(nv.handle.auth.buffer))
704+
return BAD_FUNC_ARG;
688705
nv.handle.auth.size = authSz;
689706
memcpy(nv.handle.auth.buffer, auth, authSz);
690707
}
@@ -696,9 +713,14 @@ int wolfBoot_read_blob(uint32_t nvIndex, WOLFTPM2_KEYBLOB* blob,
696713
(uint8_t*)&blob->pub.size, &readSz, pos);
697714
if (rc == 0) {
698715
pos += readSz;
699-
readSz = blob->pub.size;
700-
rc = wolfTPM2_NVReadAuth(&wolftpm_dev, &nv, nv.handle.hndl,
701-
pubAreaBuffer, &readSz, pos);
716+
if (blob->pub.size > sizeof(pubAreaBuffer)) {
717+
rc = BUFFER_E;
718+
}
719+
else {
720+
readSz = blob->pub.size;
721+
rc = wolfTPM2_NVReadAuth(&wolftpm_dev, &nv, nv.handle.hndl,
722+
pubAreaBuffer, &readSz, pos);
723+
}
702724
}
703725
if (rc == 0) {
704726
pos += readSz;
@@ -712,9 +734,14 @@ int wolfBoot_read_blob(uint32_t nvIndex, WOLFTPM2_KEYBLOB* blob,
712734
}
713735
if (rc == 0) {
714736
pos += sizeof(blob->priv.size);
715-
readSz = blob->priv.size;
716-
rc = wolfTPM2_NVReadAuth(&wolftpm_dev, &nv, nv.handle.hndl,
717-
blob->priv.buffer, &readSz, pos);
737+
if (blob->priv.size > sizeof(blob->priv.buffer)) {
738+
rc = BUFFER_E;
739+
}
740+
else {
741+
readSz = blob->priv.size;
742+
rc = wolfTPM2_NVReadAuth(&wolftpm_dev, &nv, nv.handle.hndl,
743+
blob->priv.buffer, &readSz, pos);
744+
}
718745
}
719746
if (rc == 0) {
720747
pos += blob->priv.size;
@@ -744,6 +771,8 @@ int wolfBoot_delete_blob(TPMI_RH_NV_AUTH authHandle, uint32_t nvIndex,
744771
if (authSz > 0) {
745772
if (auth == NULL)
746773
return BAD_FUNC_ARG;
774+
if (authSz > sizeof(nv.handle.auth.buffer))
775+
return BAD_FUNC_ARG;
747776
nv.handle.auth.size = authSz;
748777
memcpy(nv.handle.auth.buffer, auth, authSz);
749778
}
@@ -925,6 +954,7 @@ int wolfBoot_unseal_blob(const uint8_t* pubkey_hint,
925954
const uint8_t* auth, int authSz)
926955
{
927956
int rc, i;
957+
int secret_capacity;
928958
WOLFTPM2_SESSION policy_session;
929959
uint32_t key_type;
930960
TPM_ALG_ID pcrAlg = WOLFBOOT_TPM_PCR_ALG;
@@ -949,8 +979,13 @@ int wolfBoot_unseal_blob(const uint8_t* pubkey_hint,
949979
return -1;
950980
}
951981

982+
secret_capacity = *secret_sz;
952983
*secret_sz = 0; /* init */
953984

985+
if (secret_capacity < 0) {
986+
return BAD_FUNC_ARG;
987+
}
988+
954989
/* extract pcrMask and populate PCR selection array */
955990
memcpy(&pcrMask, policy, sizeof(pcrMask));
956991
memset(pcrArray, 0, sizeof(pcrArray));
@@ -1069,9 +1104,16 @@ int wolfBoot_unseal_blob(const uint8_t* pubkey_hint,
10691104
rc = TPM2_Unseal(&unsealIn, &unsealOut);
10701105
}
10711106
if (rc == 0) {
1072-
*secret_sz = unsealOut.outData.size;
1073-
memcpy(secret, unsealOut.outData.buffer, *secret_sz);
1107+
if (unsealOut.outData.size > WOLFBOOT_MAX_SEAL_SZ ||
1108+
(int)unsealOut.outData.size > secret_capacity) {
1109+
rc = BUFFER_E;
1110+
}
1111+
else {
1112+
*secret_sz = unsealOut.outData.size;
1113+
memcpy(secret, unsealOut.outData.buffer, *secret_sz);
1114+
}
10741115
}
1116+
TPM2_ForceZero(&unsealOut, sizeof(unsealOut));
10751117

10761118
wolfTPM2_UnloadHandle(&wolftpm_dev, &seal_blob->handle);
10771119
wolfTPM2_UnloadHandle(&wolftpm_dev, &policy_session.handle);
@@ -1486,7 +1528,8 @@ int wolfBoot_check_rot(int key_slot, uint8_t* pubkey_hint)
14861528
if (rc == 0) {
14871529
/* verify the hint (hash) matches */
14881530
if (digestSz == WOLFBOOT_SHA_DIGEST_SIZE &&
1489-
memcmp(digest, pubkey_hint, WOLFBOOT_SHA_DIGEST_SIZE) == 0) {
1531+
wolfBoot_constant_compare(digest, pubkey_hint,
1532+
WOLFBOOT_SHA_DIGEST_SIZE) == 0) {
14901533
wolfBoot_printf("TPM Root of Trust valid (id %d)\n", key_slot);
14911534
}
14921535
else {

0 commit comments

Comments
 (0)