Skip to content

Commit 0d4418a

Browse files
embhornembhorn
committed
Check tag size in wc_AesEaxDecryptFinal
1 parent 4bb9883 commit 0d4418a

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
@@ -14770,12 +14770,23 @@ static int TLSX_GetSize(TLSX* list, byte* semaphore, byte msgType,
1477014770
* pushes the running total past 0xFFFF is detected rather than
1477114771
* silently wrapped (the TLS extensions block length prefix on the
1477214772
* wire is a 2-byte field). Callees that take a word16* accumulator
14773-
* are invoked via a per-iteration shim and their delta is added
14774-
* back into the word32 total. */
14773+
* are invoked via a per-iteration shim (`cbShim`) and their delta
14774+
* is added back into the word32 total.
14775+
*
14776+
* MAINTAINER NOTE: do NOT pass &length to any *_GET_SIZE function
14777+
* that expects a `word16*` out-parameter -- that would be a type
14778+
* mismatch (UB) and would silently bypass the overflow detection
14779+
* below. When adding a new extension case, either:
14780+
* - use `length += FOO_GET_SIZE(...)` when the helper returns a
14781+
* word16 by value, or
14782+
* - use the cbShim pattern: `cbShim = 0; ret = FOO_GET_SIZE(...,
14783+
* &cbShim); length += cbShim;`
14784+
*/
1477514785
word32 length = 0;
14776-
word16 cbShim;
14786+
word16 cbShim = 0;
1477714787
byte isRequest = (msgType == client_hello ||
1477814788
msgType == certificate_request);
14789+
(void)cbShim;
1477914790

1478014791
while ((extension = list)) {
1478114792
list = extension->next;
@@ -14964,16 +14975,18 @@ static int TLSX_GetSize(TLSX* list, byte* semaphore, byte msgType,
1496414975
break;
1496514976
}
1496614977

14967-
/* marks the extension as processed so ctx level */
14968-
/* extensions don't overlap with ssl level ones. */
14969-
TURN_ON(semaphore, TLSX_ToSemaphore((word16)extension->type));
14970-
1497114978
/* Early exit: stop accumulating as soon as the running total
14972-
* cannot possibly fit the 2-byte wire length. */
14979+
* cannot possibly fit the 2-byte wire length. Check *before*
14980+
* marking the extension as processed so the semaphore is not
14981+
* left in an inconsistent state on the error path. */
1497314982
if (length > WOLFSSL_MAX_16BIT) {
1497414983
WOLFSSL_MSG("TLSX_GetSize extension length exceeds word16");
1497514984
return BUFFER_E;
1497614985
}
14986+
14987+
/* marks the extension as processed so ctx level */
14988+
/* extensions don't overlap with ssl level ones. */
14989+
TURN_ON(semaphore, TLSX_ToSemaphore((word16)extension->type));
1497714990
}
1497814991

1497914992
if ((word32)*pLength + length > WOLFSSL_MAX_16BIT) {
@@ -15241,19 +15254,24 @@ static int TLSX_Write(TLSX* list, byte* output, byte* semaphore,
1524115254
if (ret != 0)
1524215255
break;
1524315256

15244-
if (offset < prevOffset) {
15257+
if (offset <= prevOffset) {
1524515258
WOLFSSL_MSG("TLSX_Write extension length exceeds word16");
1524615259
return BUFFER_E;
1524715260
}
1524815261
}
1524915262

15250-
if ((word32)*pOffset + (word32)offset > WOLFSSL_MAX_16BIT) {
15251-
WOLFSSL_MSG("TLSX_Write total extensions length exceeds word16");
15252-
return BUFFER_E;
15263+
/* Only validate and commit the aggregate offset when the loop
15264+
* completed without error; on the error path, leave *pOffset
15265+
* unchanged and return the original failure reason so callers
15266+
* see the real error instead of a masking BUFFER_E. */
15267+
if (ret == 0) {
15268+
if ((word32)*pOffset + (word32)offset > WOLFSSL_MAX_16BIT) {
15269+
WOLFSSL_MSG("TLSX_Write total extensions length exceeds word16");
15270+
return BUFFER_E;
15271+
}
15272+
*pOffset += offset;
1525315273
}
1525415274

15255-
*pOffset += offset;
15256-
1525715275
return ret;
1525815276
}
1525915277

‎tests/api.c‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10528,6 +10528,83 @@ static int test_tls_ext_word16_overflow(void)
1052810528
wolfSSL_free(ssl);
1052910529
wolfSSL_CTX_free(ctx);
1053010530
XFREE(big, NULL, DYNAMIC_TYPE_TMP_BUFFER);
10531+
ssl = NULL;
10532+
ctx = NULL;
10533+
big = NULL;
10534+
10535+
/* Boundary case: construct a SessionTicket extension sized so that the
10536+
* total extensions length in TLSX_GetRequestSize is exactly
10537+
* WOLFSSL_MAX_16BIT - OPAQUE16_LEN (0xFFFD) *before* the OPAQUE16_LEN
10538+
* block-prefix adjustment, which must succeed. This pins the `>`
10539+
* comparison in the overflow check -- mutating it to `>=` would
10540+
* incorrectly reject this valid case and fail this test. */
10541+
{
10542+
WOLFSSL_CTX* ctx2 = NULL;
10543+
WOLFSSL* ssl2 = NULL;
10544+
SessionTicket* ticket2 = NULL;
10545+
byte* buf = NULL;
10546+
word32 baseLen = 0;
10547+
word32 baseInternal = 0;
10548+
word32 tickSz = 0;
10549+
/* TLSX_GetRequestSize rejects when internal sum > 0xFFFD. */
10550+
const word32 target = (word32)WOLFSSL_MAX_16BIT - (word32)OPAQUE16_LEN;
10551+
/* Session ticket extension contributes: type (2) + len (2) + size. */
10552+
const word32 extHdr = (word32)HELLO_EXT_TYPE_SZ
10553+
+ (word32)OPAQUE16_LEN;
10554+
int ret;
10555+
10556+
ExpectNotNull(ctx2 = wolfSSL_CTX_new(wolfSSLv23_client_method()));
10557+
ExpectNotNull(ssl2 = wolfSSL_new(ctx2));
10558+
10559+
/* Measure baseline length with no session ticket extension. The
10560+
* returned value already includes the 2-byte block-length prefix
10561+
* when nonzero; strip it to get the raw internal sum. */
10562+
if (EXPECT_SUCCESS()) {
10563+
baseLen = 0;
10564+
ret = TLSX_GetRequestSize(ssl2, client_hello, &baseLen);
10565+
ExpectIntEQ(ret, 0);
10566+
baseInternal = (baseLen > 0)
10567+
? baseLen - (word32)OPAQUE16_LEN : 0;
10568+
}
10569+
10570+
/* Target: baseInternal + extHdr + tickSz == 0xFFFD. */
10571+
if (EXPECT_SUCCESS() && baseInternal + extHdr < target) {
10572+
tickSz = target - baseInternal - extHdr;
10573+
}
10574+
10575+
if (EXPECT_SUCCESS() && tickSz > 0 && tickSz <= WOLFSSL_MAX_16BIT) {
10576+
buf = (byte*)XMALLOC(tickSz, NULL, DYNAMIC_TYPE_TMP_BUFFER);
10577+
ExpectNotNull(buf);
10578+
if (buf != NULL)
10579+
XMEMSET(buf, 0x5A, tickSz);
10580+
}
10581+
10582+
if (EXPECT_SUCCESS() && buf != NULL) {
10583+
ticket2 = TLSX_SessionTicket_Create(0, buf, (word16)tickSz,
10584+
ssl2->heap);
10585+
ExpectNotNull(ticket2);
10586+
}
10587+
if (EXPECT_SUCCESS() && ticket2 != NULL) {
10588+
ExpectIntEQ(TLSX_UseSessionTicket(&ssl2->extensions, ticket2,
10589+
ssl2->heap), WOLFSSL_SUCCESS);
10590+
ticket2 = NULL;
10591+
}
10592+
10593+
/* Exact boundary: internal sum == 0xFFFD must succeed, and the
10594+
* final returned length is 0xFFFD + OPAQUE16_LEN == 0xFFFF. */
10595+
if (EXPECT_SUCCESS()) {
10596+
word32 lenBoundary = 0;
10597+
ret = TLSX_GetRequestSize(ssl2, client_hello, &lenBoundary);
10598+
ExpectIntEQ(ret, 0);
10599+
ExpectIntEQ(lenBoundary, (word32)WOLFSSL_MAX_16BIT);
10600+
}
10601+
10602+
if (ticket2 != NULL)
10603+
TLSX_SessionTicket_Free(ticket2, ssl2->heap);
10604+
wolfSSL_free(ssl2);
10605+
wolfSSL_CTX_free(ctx2);
10606+
XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER);
10607+
}
1053110608
#endif
1053210609
return EXPECT_RESULT();
1053310610
}

‎wolfcrypt/src/aes.c‎

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

16826+
if (authTagSz < WOLFSSL_MIN_AUTH_TAG_SZ
16827+
|| authTagSz > WC_AES_BLOCK_SIZE) {
16828+
return BAD_FUNC_ARG;
16829+
}
16830+
1682616831
#if defined(WOLFSSL_SMALL_STACK)
1682716832
if ((eax = (AesEax *)XMALLOC(sizeof(AesEax),
1682816833
NULL,
@@ -17171,7 +17176,8 @@ int wc_AesEaxDecryptFinal(AesEax* eax,
1717117176
byte authTag[WC_AES_BLOCK_SIZE];
1717217177
#endif
1717317178

17174-
if (eax == NULL || authIn == NULL || authInSz > WC_AES_BLOCK_SIZE) {
17179+
if (eax == NULL || authIn == NULL || authInSz > WC_AES_BLOCK_SIZE
17180+
|| authInSz < WOLFSSL_MIN_AUTH_TAG_SZ) {
1717517181
return BAD_FUNC_ARG;
1717617182
}
1717717183

‎wolfcrypt/test/test.c‎

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

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

‎wolfssl/internal.h‎

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

31793179
#if defined(WOLFSSL_TLS13) || !defined(NO_WOLFSSL_CLIENT)
3180+
#ifdef WOLFSSL_API_PREFIX_MAP
3181+
#define TLSX_GetRequestSize wolfSSL_TLSX_GetRequestSize
3182+
#define TLSX_WriteRequest wolfSSL_TLSX_WriteRequest
3183+
#endif
31803184
WOLFSSL_TEST_VIS int TLSX_GetRequestSize(WOLFSSL* ssl, byte msgType,
31813185
word32* pLength);
31823186
WOLFSSL_TEST_VIS int TLSX_WriteRequest(WOLFSSL* ssl, byte* output,
@@ -3585,6 +3589,11 @@ typedef struct TicketEncCbCtx {
35853589

35863590
#endif /* !WOLFSSL_NO_DEF_TICKET_ENC_CB && !NO_WOLFSSL_SERVER */
35873591

3592+
#ifdef WOLFSSL_API_PREFIX_MAP
3593+
#define TLSX_UseSessionTicket wolfSSL_TLSX_UseSessionTicket
3594+
#define TLSX_SessionTicket_Create wolfSSL_TLSX_SessionTicket_Create
3595+
#define TLSX_SessionTicket_Free wolfSSL_TLSX_SessionTicket_Free
3596+
#endif
35883597
WOLFSSL_TEST_VIS int TLSX_UseSessionTicket(TLSX** extensions,
35893598
SessionTicket* ticket, void* heap);
35903599
WOLFSSL_TEST_VIS SessionTicket* TLSX_SessionTicket_Create(word32 lifetime,

0 commit comments

Comments
 (0)