Skip to content

Commit 88dc487

Browse files
embhornembhorn
committed
Check tag size in wc_AesEaxDecryptFinal
1 parent 265b8f6 commit 88dc487

5 files changed

Lines changed: 233 additions & 15 deletions

File tree

‎src/tls.c‎

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14872,12 +14872,23 @@ static int TLSX_GetSize(TLSX* list, byte* semaphore, byte msgType,
1487214872
* pushes the running total past 0xFFFF is detected rather than
1487314873
* silently wrapped (the TLS extensions block length prefix on the
1487414874
* wire is a 2-byte field). Callees that take a word16* accumulator
14875-
* are invoked via a per-iteration shim and their delta is added
14876-
* back into the word32 total. */
14875+
* are invoked via a per-iteration shim (`cbShim`) and their delta
14876+
* is added back into the word32 total.
14877+
*
14878+
* MAINTAINER NOTE: do NOT pass &length to any *_GET_SIZE function
14879+
* that expects a `word16*` out-parameter -- that would be a type
14880+
* mismatch (UB) and would silently bypass the overflow detection
14881+
* below. When adding a new extension case, either:
14882+
* - use `length += FOO_GET_SIZE(...)` when the helper returns a
14883+
* word16 by value, or
14884+
* - use the cbShim pattern: `cbShim = 0; ret = FOO_GET_SIZE(...,
14885+
* &cbShim); length += cbShim;`
14886+
*/
1487714887
word32 length = 0;
14878-
word16 cbShim;
14888+
word16 cbShim = 0;
1487914889
byte isRequest = (msgType == client_hello ||
1488014890
msgType == certificate_request);
14891+
(void)cbShim;
1488114892

1488214893
while ((extension = list)) {
1488314894
list = extension->next;
@@ -15071,16 +15082,18 @@ static int TLSX_GetSize(TLSX* list, byte* semaphore, byte msgType,
1507115082
break;
1507215083
}
1507315084

15074-
/* marks the extension as processed so ctx level */
15075-
/* extensions don't overlap with ssl level ones. */
15076-
TURN_ON(semaphore, TLSX_ToSemaphore((word16)extension->type));
15077-
1507815085
/* Early exit: stop accumulating as soon as the running total
15079-
* cannot possibly fit the 2-byte wire length. */
15086+
* cannot possibly fit the 2-byte wire length. Check *before*
15087+
* marking the extension as processed so the semaphore is not
15088+
* left in an inconsistent state on the error path. */
1508015089
if (length > WOLFSSL_MAX_16BIT) {
1508115090
WOLFSSL_MSG("TLSX_GetSize extension length exceeds word16");
1508215091
return BUFFER_E;
1508315092
}
15093+
15094+
/* marks the extension as processed so ctx level */
15095+
/* extensions don't overlap with ssl level ones. */
15096+
TURN_ON(semaphore, TLSX_ToSemaphore((word16)extension->type));
1508415097
}
1508515098

1508615099
if ((word32)*pLength + length > WOLFSSL_MAX_16BIT) {
@@ -15354,19 +15367,24 @@ static int TLSX_Write(TLSX* list, byte* output, byte* semaphore,
1535415367
if (ret != 0)
1535515368
break;
1535615369

15357-
if (offset < prevOffset) {
15370+
if (offset <= prevOffset) {
1535815371
WOLFSSL_MSG("TLSX_Write extension length exceeds word16");
1535915372
return BUFFER_E;
1536015373
}
1536115374
}
1536215375

15363-
if ((word32)*pOffset + (word32)offset > WOLFSSL_MAX_16BIT) {
15364-
WOLFSSL_MSG("TLSX_Write total extensions length exceeds word16");
15365-
return BUFFER_E;
15376+
/* Only validate and commit the aggregate offset when the loop
15377+
* completed without error; on the error path, leave *pOffset
15378+
* unchanged and return the original failure reason so callers
15379+
* see the real error instead of a masking BUFFER_E. */
15380+
if (ret == 0) {
15381+
if ((word32)*pOffset + (word32)offset > WOLFSSL_MAX_16BIT) {
15382+
WOLFSSL_MSG("TLSX_Write total extensions length exceeds word16");
15383+
return BUFFER_E;
15384+
}
15385+
*pOffset += offset;
1536615386
}
1536715387

15368-
*pOffset += offset;
15369-
1537015388
return ret;
1537115389
}
1537215390

‎tests/api.c‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10602,6 +10602,83 @@ static int test_tls_ext_word16_overflow(void)
1060210602
wolfSSL_free(ssl);
1060310603
wolfSSL_CTX_free(ctx);
1060410604
XFREE(big, NULL, DYNAMIC_TYPE_TMP_BUFFER);
10605+
ssl = NULL;
10606+
ctx = NULL;
10607+
big = NULL;
10608+
10609+
/* Boundary case: construct a SessionTicket extension sized so that the
10610+
* total extensions length in TLSX_GetRequestSize is exactly
10611+
* WOLFSSL_MAX_16BIT - OPAQUE16_LEN (0xFFFD) *before* the OPAQUE16_LEN
10612+
* block-prefix adjustment, which must succeed. This pins the `>`
10613+
* comparison in the overflow check -- mutating it to `>=` would
10614+
* incorrectly reject this valid case and fail this test. */
10615+
{
10616+
WOLFSSL_CTX* ctx2 = NULL;
10617+
WOLFSSL* ssl2 = NULL;
10618+
SessionTicket* ticket2 = NULL;
10619+
byte* buf = NULL;
10620+
word32 baseLen = 0;
10621+
word32 baseInternal = 0;
10622+
word32 tickSz = 0;
10623+
/* TLSX_GetRequestSize rejects when internal sum > 0xFFFD. */
10624+
const word32 target = (word32)WOLFSSL_MAX_16BIT - (word32)OPAQUE16_LEN;
10625+
/* Session ticket extension contributes: type (2) + len (2) + size. */
10626+
const word32 extHdr = (word32)HELLO_EXT_TYPE_SZ
10627+
+ (word32)OPAQUE16_LEN;
10628+
int ret;
10629+
10630+
ExpectNotNull(ctx2 = wolfSSL_CTX_new(wolfSSLv23_client_method()));
10631+
ExpectNotNull(ssl2 = wolfSSL_new(ctx2));
10632+
10633+
/* Measure baseline length with no session ticket extension. The
10634+
* returned value already includes the 2-byte block-length prefix
10635+
* when nonzero; strip it to get the raw internal sum. */
10636+
if (EXPECT_SUCCESS()) {
10637+
baseLen = 0;
10638+
ret = TLSX_GetRequestSize(ssl2, client_hello, &baseLen);
10639+
ExpectIntEQ(ret, 0);
10640+
baseInternal = (baseLen > 0)
10641+
? baseLen - (word32)OPAQUE16_LEN : 0;
10642+
}
10643+
10644+
/* Target: baseInternal + extHdr + tickSz == 0xFFFD. */
10645+
if (EXPECT_SUCCESS() && baseInternal + extHdr < target) {
10646+
tickSz = target - baseInternal - extHdr;
10647+
}
10648+
10649+
if (EXPECT_SUCCESS() && tickSz > 0 && tickSz <= WOLFSSL_MAX_16BIT) {
10650+
buf = (byte*)XMALLOC(tickSz, NULL, DYNAMIC_TYPE_TMP_BUFFER);
10651+
ExpectNotNull(buf);
10652+
if (buf != NULL)
10653+
XMEMSET(buf, 0x5A, tickSz);
10654+
}
10655+
10656+
if (EXPECT_SUCCESS() && buf != NULL) {
10657+
ticket2 = TLSX_SessionTicket_Create(0, buf, (word16)tickSz,
10658+
ssl2->heap);
10659+
ExpectNotNull(ticket2);
10660+
}
10661+
if (EXPECT_SUCCESS() && ticket2 != NULL) {
10662+
ExpectIntEQ(TLSX_UseSessionTicket(&ssl2->extensions, ticket2,
10663+
ssl2->heap), WOLFSSL_SUCCESS);
10664+
ticket2 = NULL;
10665+
}
10666+
10667+
/* Exact boundary: internal sum == 0xFFFD must succeed, and the
10668+
* final returned length is 0xFFFD + OPAQUE16_LEN == 0xFFFF. */
10669+
if (EXPECT_SUCCESS()) {
10670+
word32 lenBoundary = 0;
10671+
ret = TLSX_GetRequestSize(ssl2, client_hello, &lenBoundary);
10672+
ExpectIntEQ(ret, 0);
10673+
ExpectIntEQ(lenBoundary, (word32)WOLFSSL_MAX_16BIT);
10674+
}
10675+
10676+
if (ticket2 != NULL)
10677+
TLSX_SessionTicket_Free(ticket2, ssl2->heap);
10678+
wolfSSL_free(ssl2);
10679+
wolfSSL_CTX_free(ctx2);
10680+
XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER);
10681+
}
1060510682
#endif
1060610683
return EXPECT_RESULT();
1060710684
}

‎wolfcrypt/src/aes.c‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16849,6 +16849,11 @@ int wc_AesEaxDecryptAuth(const byte* key, word32 keySz, byte* out,
1684916849
return BAD_FUNC_ARG;
1685016850
}
1685116851

16852+
if (authTagSz < WOLFSSL_MIN_AUTH_TAG_SZ
16853+
|| authTagSz > WC_AES_BLOCK_SIZE) {
16854+
return BAD_FUNC_ARG;
16855+
}
16856+
1685216857
#if defined(WOLFSSL_SMALL_STACK)
1685316858
if ((eax = (AesEax *)XMALLOC(sizeof(AesEax),
1685416859
NULL,
@@ -17197,7 +17202,8 @@ int wc_AesEaxDecryptFinal(AesEax* eax,
1719717202
byte authTag[WC_AES_BLOCK_SIZE];
1719817203
#endif
1719917204

17200-
if (eax == NULL || authIn == NULL || authInSz > WC_AES_BLOCK_SIZE) {
17205+
if (eax == NULL || authIn == NULL || authInSz > WC_AES_BLOCK_SIZE
17206+
|| authInSz < WOLFSSL_MIN_AUTH_TAG_SZ) {
1720117207
return BAD_FUNC_ARG;
1720217208
}
1720317209

‎wolfcrypt/test/test.c‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19505,6 +19505,114 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t aes_eax_test(void)
1950519505
}
1950619506

1950719507
}
19508+
19509+
/* Regression test: wc_AesEaxDecryptAuth must reject authTagSz below
19510+
* WOLFSSL_MIN_AUTH_TAG_SZ (including zero), otherwise an attacker could
19511+
* bypass tag verification by supplying an empty tag. */
19512+
#if WOLFSSL_MIN_AUTH_TAG_SZ > 0
19513+
{
19514+
byte zero_ct[16];
19515+
byte zero_pt[16];
19516+
byte zero_tag[16];
19517+
XMEMSET(zero_ct, 0, sizeof(zero_ct));
19518+
XMEMSET(zero_tag, 0, sizeof(zero_tag));
19519+
19520+
ret = wc_AesEaxDecryptAuth(vectors[0].key,
19521+
(word32)vectors[0].key_length,
19522+
zero_pt,
19523+
zero_ct, (word32)sizeof(zero_ct),
19524+
vectors[0].iv,
19525+
(word32)vectors[0].iv_length,
19526+
zero_tag, 0,
19527+
vectors[0].aad,
19528+
(word32)vectors[0].aad_length);
19529+
if (ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG)) {
19530+
return WC_TEST_RET_ENC_EC(ret);
19531+
}
19532+
19533+
#if WOLFSSL_MIN_AUTH_TAG_SZ > 1
19534+
ret = wc_AesEaxDecryptAuth(vectors[0].key,
19535+
(word32)vectors[0].key_length,
19536+
zero_pt,
19537+
zero_ct, (word32)sizeof(zero_ct),
19538+
vectors[0].iv,
19539+
(word32)vectors[0].iv_length,
19540+
zero_tag, WOLFSSL_MIN_AUTH_TAG_SZ - 1,
19541+
vectors[0].aad,
19542+
(word32)vectors[0].aad_length);
19543+
if (ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG)) {
19544+
return WC_TEST_RET_ENC_EC(ret);
19545+
}
19546+
#endif
19547+
19548+
/* Upper bound: authTagSz > WC_AES_BLOCK_SIZE must be rejected.
19549+
* Pins the '>' operator in the validation against mutation to '>='
19550+
* and prevents an over-read of the caller-supplied tag buffer. */
19551+
ret = wc_AesEaxDecryptAuth(vectors[0].key,
19552+
(word32)vectors[0].key_length,
19553+
zero_pt,
19554+
zero_ct, (word32)sizeof(zero_ct),
19555+
vectors[0].iv,
19556+
(word32)vectors[0].iv_length,
19557+
zero_tag, WC_AES_BLOCK_SIZE + 1,
19558+
vectors[0].aad,
19559+
(word32)vectors[0].aad_length);
19560+
if (ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG)) {
19561+
return WC_TEST_RET_ENC_EC(ret);
19562+
}
19563+
19564+
/* Direct incremental-API coverage: wc_AesEaxDecryptFinal must also
19565+
* reject authInSz of zero and below WOLFSSL_MIN_AUTH_TAG_SZ. The
19566+
* one-shot API above is a separate code path. Heap-allocate the
19567+
* AesEax context to keep stack usage within Linux kernel limits. */
19568+
{
19569+
AesEax *eax = (AesEax *)XMALLOC(sizeof(*eax), HEAP_HINT,
19570+
DYNAMIC_TYPE_TMP_BUFFER);
19571+
if (eax == NULL) {
19572+
return WC_TEST_RET_ENC_NC;
19573+
}
19574+
XMEMSET(eax, 0, sizeof(*eax));
19575+
ret = wc_AesEaxInit(eax,
19576+
vectors[0].key, (word32)vectors[0].key_length,
19577+
vectors[0].iv, (word32)vectors[0].iv_length,
19578+
vectors[0].aad,
19579+
(word32)vectors[0].aad_length);
19580+
if (ret != 0) {
19581+
XFREE(eax, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
19582+
return WC_TEST_RET_ENC_EC(ret);
19583+
}
19584+
19585+
ret = wc_AesEaxDecryptFinal(eax, zero_tag, 0);
19586+
if (ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG)) {
19587+
wc_AesEaxFree(eax);
19588+
XFREE(eax, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
19589+
return WC_TEST_RET_ENC_EC(ret);
19590+
}
19591+
19592+
#if WOLFSSL_MIN_AUTH_TAG_SZ > 1
19593+
ret = wc_AesEaxDecryptFinal(eax, zero_tag,
19594+
WOLFSSL_MIN_AUTH_TAG_SZ - 1);
19595+
if (ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG)) {
19596+
wc_AesEaxFree(eax);
19597+
XFREE(eax, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
19598+
return WC_TEST_RET_ENC_EC(ret);
19599+
}
19600+
#endif
19601+
19602+
/* Upper bound: authInSz > WC_AES_BLOCK_SIZE must be rejected. */
19603+
ret = wc_AesEaxDecryptFinal(eax, zero_tag, WC_AES_BLOCK_SIZE + 1);
19604+
if (ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG)) {
19605+
wc_AesEaxFree(eax);
19606+
XFREE(eax, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
19607+
return WC_TEST_RET_ENC_EC(ret);
19608+
}
19609+
19610+
wc_AesEaxFree(eax);
19611+
XFREE(eax, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
19612+
}
19613+
}
19614+
#endif /* WOLFSSL_MIN_AUTH_TAG_SZ > 0 */
19615+
1950819616
return 0;
1950919617
}
1951019618

‎wolfssl/internal.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3189,6 +3189,10 @@ WOLFSSL_LOCAL int TLSX_SupportExtensions(WOLFSSL* ssl);
31893189
WOLFSSL_LOCAL int TLSX_PopulateExtensions(WOLFSSL* ssl, byte isRequest);
31903190

31913191
#if defined(WOLFSSL_TLS13) || !defined(NO_WOLFSSL_CLIENT)
3192+
#ifdef WOLFSSL_API_PREFIX_MAP
3193+
#define TLSX_GetRequestSize wolfSSL_TLSX_GetRequestSize
3194+
#define TLSX_WriteRequest wolfSSL_TLSX_WriteRequest
3195+
#endif
31923196
WOLFSSL_TEST_VIS int TLSX_GetRequestSize(WOLFSSL* ssl, byte msgType,
31933197
word32* pLength);
31943198
WOLFSSL_TEST_VIS int TLSX_WriteRequest(WOLFSSL* ssl, byte* output,
@@ -3597,6 +3601,11 @@ typedef struct TicketEncCbCtx {
35973601

35983602
#endif /* !WOLFSSL_NO_DEF_TICKET_ENC_CB && !NO_WOLFSSL_SERVER */
35993603

3604+
#ifdef WOLFSSL_API_PREFIX_MAP
3605+
#define TLSX_UseSessionTicket wolfSSL_TLSX_UseSessionTicket
3606+
#define TLSX_SessionTicket_Create wolfSSL_TLSX_SessionTicket_Create
3607+
#define TLSX_SessionTicket_Free wolfSSL_TLSX_SessionTicket_Free
3608+
#endif
36003609
WOLFSSL_TEST_VIS int TLSX_UseSessionTicket(TLSX** extensions,
36013610
SessionTicket* ticket, void* heap);
36023611
WOLFSSL_TEST_VIS SessionTicket* TLSX_SessionTicket_Create(word32 lifetime,

0 commit comments

Comments
 (0)