From 7a61b2e087822be219fbbc72f6fc1ca39ef78eb1 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Tue, 7 Apr 2026 14:56:59 +0200 Subject: [PATCH] Refactor record padding handling to eliminate middle padding pattern Move padSz index advancement from individual message handlers to a single location at the end of record processing in ProcessReply. Previously, each handler (DoFinished, DoApplicationData, DoAlert, DoCertificate, DoServerHello, etc.) advanced the index by padSz, which then had to be corrected when processing multiple messages in one record. This "middle padding" pattern was error-prone. Now curSize is reduced by padSz after decryption/verification so it reflects content size only. Message handlers advance the index by content size only. padSz is added once when the record is fully processed. The correction code for multi-message records is removed. --- src/internal.c | 205 ++++++++++++++----------------------------------- src/tls13.c | 28 ++----- 2 files changed, 62 insertions(+), 171 deletions(-) diff --git a/src/internal.c b/src/internal.c index f19d8fa10d..c7656e0baa 100644 --- a/src/internal.c +++ b/src/internal.c @@ -11905,10 +11905,7 @@ int MsgCheckEncryption(WOLFSSL* ssl, byte type, byte encrypted) static WC_INLINE int isLastMsg(const WOLFSSL* ssl, word32 msgSz) { - word32 extra = 0; - if (IsEncryptionOn(ssl, 0)) - extra = ssl->keys.padSz; - return (ssl->buffers.inputBuffer.idx - ssl->curStartIdx) + msgSz + extra + return (ssl->buffers.inputBuffer.idx - ssl->curStartIdx) + msgSz == ssl->curSize; } @@ -17698,9 +17695,6 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, ssl->options.serverState = SERVER_CERT_COMPLETE; } - if (IsEncryptionOn(ssl, 0)) - args->idx += ssl->keys.padSz; - /* Advance state and proceed */ ssl->options.asyncState = TLS_ASYNC_END; } /* case TLS_ASYNC_FINALIZE */ @@ -17964,12 +17958,6 @@ static int DoCertificateStatus(WOLFSSL* ssl, byte* input, word32* inOutIdx, SendAlert(ssl, alert_fatal, bad_certificate_status_response); } - if (IsEncryptionOn(ssl, 0)) { - if (*inOutIdx + ssl->keys.padSz > size) - return BUFFER_E; - *inOutIdx += ssl->keys.padSz; - } - WOLFSSL_LEAVE("DoCertificateStatus", ret); WOLFSSL_END(WC_FUNC_CERTIFICATE_STATUS_DO); @@ -17984,10 +17972,9 @@ static int DoCertificateStatus(WOLFSSL* ssl, byte* input, word32* inOutIdx, #if !defined(NO_TLS) && !defined(WOLFSSL_NO_TLS12) -static int DoHelloRequest(WOLFSSL* ssl, const byte* input, word32* inOutIdx, - word32 size, word32 totalSz) +static int DoHelloRequest(WOLFSSL* ssl, word32 size) { - (void)input; + (void)size; WOLFSSL_START(WC_FUNC_HELLO_REQUEST_DO); WOLFSSL_ENTER("DoHelloRequest"); @@ -17995,17 +17982,6 @@ static int DoHelloRequest(WOLFSSL* ssl, const byte* input, word32* inOutIdx, if (size) /* must be 0 */ return BUFFER_ERROR; - if (IsEncryptionOn(ssl, 0)) { - /* If size == totalSz then we are in DtlsMsgDrain so no need to worry - * about padding */ - /* access beyond input + size should be checked against totalSz */ - if (size != totalSz && - *inOutIdx + ssl->keys.padSz > totalSz) - return BUFFER_E; - - *inOutIdx += ssl->keys.padSz; - } - if (ssl->options.side == WOLFSSL_SERVER_END) { SendAlert(ssl, alert_fatal, unexpected_message); /* try */ WOLFSSL_ERROR_VERBOSE(FATAL_ERROR); @@ -18040,7 +18016,7 @@ int DoFinished(WOLFSSL* ssl, const byte* input, word32* inOutIdx, word32 size, * If size == totalSz then we are in DtlsMsgDrain so no need to worry about * padding */ if (size != totalSz) { - if (*inOutIdx + size + ssl->keys.padSz > totalSz) + if (*inOutIdx + size > totalSz) return BUFFER_E; } @@ -18083,8 +18059,7 @@ int DoFinished(WOLFSSL* ssl, const byte* input, word32* inOutIdx, word32 size, } #endif - /* force input exhaustion at ProcessReply consuming padSz */ - *inOutIdx += size + ssl->keys.padSz; + *inOutIdx += size; if (ssl->options.side == WOLFSSL_CLIENT_END) { ssl->options.serverState = SERVER_FINISHED_COMPLETE; @@ -18737,8 +18712,7 @@ int DoHandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, return INCOMPLETE_DATA; } - expectedIdx = *inOutIdx + size + - (ssl->keys.encryptionOn ? ssl->keys.padSz : 0); + expectedIdx = *inOutIdx + size; #if !defined(NO_WOLFSSL_SERVER) && \ defined(HAVE_SECURE_RENEGOTIATION) && \ @@ -18903,21 +18877,13 @@ int DoHandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, case hello_request: WOLFSSL_MSG("processing hello request"); - ret = DoHelloRequest(ssl, input, inOutIdx, size, totalSz); + ret = DoHelloRequest(ssl, size); break; #ifndef NO_WOLFSSL_CLIENT case hello_verify_request: WOLFSSL_MSG("processing hello verify request"); ret = DoHelloVerifyRequest(ssl, input,inOutIdx, size); - if (IsEncryptionOn(ssl, 0)) { - /* access beyond input + size should be checked against totalSz - */ - if (*inOutIdx + ssl->keys.padSz > totalSz) - return BUFFER_E; - - *inOutIdx += ssl->keys.padSz; - } break; case server_hello: @@ -18989,8 +18955,6 @@ int DoHandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, AddLateName("ServerHelloDone", &ssl->timeoutInfo); #endif ssl->options.serverState = SERVER_HELLODONE_COMPLETE; - if (IsEncryptionOn(ssl, 0)) - *inOutIdx += ssl->keys.padSz; break; case finished: @@ -19022,16 +18986,6 @@ int DoHandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, } } #endif - /* If size == totalSz then we are in DtlsMsgDrain so no need to worry - * about padding */ - if (IsEncryptionOn(ssl, 0)) { - /* access beyond input + size should be checked against totalSz - */ - if (size != totalSz && - *inOutIdx + ssl->keys.padSz > totalSz) - return BUFFER_E; - *inOutIdx += ssl->keys.padSz; - } break; case client_key_exchange: @@ -19133,6 +19087,8 @@ static int DoHandShakeMsg(WOLFSSL* ssl, byte* input, word32* inOutIdx, } inputLength = ssl->buffers.inputBuffer.length - *inOutIdx; + if (IsEncryptionOn(ssl, 0)) + inputLength -= ssl->keys.padSz; /* If there is a pending fragmented handshake message, * pending message size will be non-zero. */ @@ -19893,11 +19849,6 @@ static int DoDtlsHandShakeMsg(WOLFSSL* ssl, byte* input, word32* inOutIdx, input + *inOutIdx, size, type, fragOffset, fragSz, ssl->heap); *inOutIdx += fragSz; - if (*inOutIdx + ssl->keys.padSz > totalSz) { - WOLFSSL_ERROR(BUFFER_E); - return BUFFER_E; - } - *inOutIdx += ssl->keys.padSz; ret = 0; #ifndef WOLFSSL_DTLS_RESEND_ONLY_TIMEOUT /* If we receive an out of order last flight msg then retransmit */ @@ -19936,10 +19887,6 @@ static int DoDtlsHandShakeMsg(WOLFSSL* ssl, byte* input, word32* inOutIdx, /* Already saw this message and processed it. It can be ignored. */ WOLFSSL_MSG("Already saw this message and processed it"); *inOutIdx += fragSz; - if (*inOutIdx + ssl->keys.padSz > totalSz) { - WOLFSSL_ERROR(BUFFER_E); - return BUFFER_E; - } #ifndef WOLFSSL_DTLS_RESEND_ONLY_TIMEOUT if (IsDtlsNotSctpMode(ssl) && VerifyForDtlsMsgPoolSend(ssl, type, fragOffset)) { @@ -19947,7 +19894,6 @@ static int DoDtlsHandShakeMsg(WOLFSSL* ssl, byte* input, word32* inOutIdx, ret = DtlsMsgPoolSend(ssl, 0); } #endif - *inOutIdx += ssl->keys.padSz; } else if (fragSz < size) { /* Since this branch is in order, but fragmented, dtls_rx_msg_list will @@ -19971,11 +19917,6 @@ static int DoDtlsHandShakeMsg(WOLFSSL* ssl, byte* input, word32* inOutIdx, input + *inOutIdx, size, type, fragOffset, fragSz, ssl->heap); *inOutIdx += fragSz; - if (*inOutIdx + ssl->keys.padSz > totalSz) { - WOLFSSL_ERROR(BUFFER_E); - return BUFFER_E; - } - *inOutIdx += ssl->keys.padSz; ret = 0; if (ssl->dtls_rx_msg_list != NULL && ssl->dtls_rx_msg_list->ready) ret = DtlsMsgDrain(ssl); @@ -19992,9 +19933,9 @@ static int DoDtlsHandShakeMsg(WOLFSSL* ssl, byte* input, word32* inOutIdx, WOLFSSL_ERROR(BUFFER_ERROR); return BUFFER_ERROR; } - if (idx + fragSz + ssl->keys.padSz > totalSz) + if (idx + fragSz > totalSz) return BUFFER_E; - *inOutIdx = idx + fragSz + ssl->keys.padSz; + *inOutIdx = idx + fragSz; /* In async mode always store the message and process it with * DtlsMsgDrain because in case of a WC_PENDING_E it will be * easier this way. */ @@ -22147,7 +22088,7 @@ int DoApplicationData(WOLFSSL* ssl, byte* input, word32* inOutIdx, int sniff) } #endif - dataSz = (int)(msgSz - ssl->keys.padSz); + dataSz = (int)msgSz; if (dataSz < 0) { WOLFSSL_MSG("App data buffer error, malicious input?"); if (sniff == NO_SNIFF) { @@ -22202,8 +22143,6 @@ int DoApplicationData(WOLFSSL* ssl, byte* input, word32* inOutIdx, int sniff) ssl->buffers.clearOutputBuffer.length = (unsigned int)dataSz; } - idx += ssl->keys.padSz; - #ifdef HAVE_LIBZ /* decompress could be bigger, overwrite after verify */ if (ssl->options.usingCompression) @@ -22494,9 +22433,6 @@ static int DoAlert(WOLFSSL* ssl, byte* input, word32* inOutIdx, int* type) } #endif - if (IsEncryptionOn(ssl, 0)) - dataSz -= ssl->keys.padSz; - /* make sure can read the message */ if (dataSz != ALERT_SIZE) { #ifdef WOLFSSL_EXTRA_ALERTS @@ -22565,9 +22501,6 @@ static int DoAlert(WOLFSSL* ssl, byte* input, word32* inOutIdx, int* type) WOLFSSL_ERROR(*type); } } - if (IsEncryptionOn(ssl, 0)) - *inOutIdx += ssl->keys.padSz; - return level; } @@ -23575,6 +23508,11 @@ default: } + /* Reduce curSize to content only, excluding padding/MAC overhead. + * padSz is accounted for once at the end of record processing. */ + if (IsEncryptionOn(ssl, 0)) + ssl->curSize -= (word16)ssl->keys.padSz; + /* in case > 1 msg per record */ ssl->curStartIdx = ssl->buffers.inputBuffer.idx; @@ -23632,7 +23570,7 @@ default: } #if defined(HAVE_ENCRYPT_THEN_MAC) && !defined(WOLFSSL_AEAD_ONLY) if (IsEncryptionOn(ssl, 0) && ssl->options.startedETMRead) { - if ((ssl->curSize - ssl->keys.padSz > MAX_PLAINTEXT_SZ) + if ((ssl->curSize > MAX_PLAINTEXT_SZ) #ifdef WOLFSSL_ASYNC_CRYPT && ssl->buffers.inputBuffer.length != ssl->buffers.inputBuffer.idx @@ -23650,7 +23588,7 @@ default: #endif /* TLS13 plaintext limit is checked earlier before decryption */ if (!IsAtLeastTLSv1_3(ssl->version) - && ssl->curSize - ssl->keys.padSz > MAX_PLAINTEXT_SZ + && ssl->curSize > MAX_PLAINTEXT_SZ #ifdef WOLFSSL_ASYNC_CRYPT && ssl->buffers.inputBuffer.length != ssl->buffers.inputBuffer.idx @@ -23855,11 +23793,6 @@ default: return LENGTH_ERROR; } - if (IsEncryptionOn(ssl, 0) && ssl->options.handShakeDone) { - ssl->buffers.inputBuffer.idx += ssl->keys.padSz; - ssl->curSize -= (word16)ssl->keys.padSz; - } - if (ssl->curSize != 1) { WOLFSSL_MSG("Malicious or corrupted ChangeCipher msg"); WOLFSSL_ERROR_VERBOSE(LENGTH_ERROR); @@ -24027,7 +23960,6 @@ default: ssl->buffers.inputBuffer.idx - ssl->keys.padSz, &processedSize); ssl->buffers.inputBuffer.idx += processedSize; - ssl->buffers.inputBuffer.idx += ssl->keys.padSz; if (ret != 0) return ret; break; @@ -24041,53 +23973,44 @@ default: ssl->options.processReply = doProcessInit; - /* input exhausted */ - if (ssl->buffers.inputBuffer.idx >= ssl->buffers.inputBuffer.length -#ifdef WOLFSSL_DTLS - || (ssl->options.dtls && - /* If app data was processed then return now to avoid - * dropping any app data. */ - (ssl->curRL.type == application_data || - /* client: if we processed a finished message, return to - * allow higher layers to establish the crypto - * parameters of the connection. The remaining data - * may be app data that we would drop without the - * crypto setup. */ - (ssl->options.side == WOLFSSL_CLIENT_END && - ssl->options.serverState == SERVER_FINISHED_COMPLETE && - ssl->options.handShakeState != HANDSHAKE_DONE))) -#endif - ) { - /* Shrink input buffer when we successfully finish record - * processing */ - if ((ret == 0) && ssl->buffers.inputBuffer.dynamicFlag) - ShrinkInputBuffer(ssl, NO_FORCED_FREE); - return ret; - } /* more messages per record */ - else if ((ssl->buffers.inputBuffer.idx - ssl->curStartIdx) + if ((ssl->buffers.inputBuffer.idx - ssl->curStartIdx) < ssl->curSize) { WOLFSSL_MSG("More messages in record"); ssl->options.processReply = runProcessingOneMessage; - - if (IsEncryptionOn(ssl, 0)) { - /* With encryption on, we advance the index by the value - * of ssl->keys.padSz. Since padding only appears once, we - * only can do this at the end of record parsing. We have to - * reset the index to the start of the next message here. */ - if (ssl->buffers.inputBuffer.idx >= ssl->keys.padSz) { - ssl->buffers.inputBuffer.idx -= ssl->keys.padSz; - } - else { - WOLFSSL_MSG("\tBuffer advanced not enough error"); - WOLFSSL_ERROR_VERBOSE(FATAL_ERROR); - return FATAL_ERROR; - } - } } - /* more records */ else { + /* Done with this record. Advance past padding/MAC. */ + if (IsEncryptionOn(ssl, 0)) + ssl->buffers.inputBuffer.idx += ssl->keys.padSz; + + /* input exhausted */ + if (ssl->buffers.inputBuffer.idx >= + ssl->buffers.inputBuffer.length +#ifdef WOLFSSL_DTLS + || (ssl->options.dtls && + /* If app data was processed then return now to avoid + * dropping any app data. */ + (ssl->curRL.type == application_data || + /* client: if we processed a finished message, return + * to allow higher layers to establish the + * crypto parameters of the connection. The + * remaining data may be app data that we would + * drop without the crypto setup. */ + (ssl->options.side == WOLFSSL_CLIENT_END && + ssl->options.serverState == + SERVER_FINISHED_COMPLETE && + ssl->options.handShakeState != HANDSHAKE_DONE))) +#endif + ) { + /* Shrink input buffer when we successfully finish record + * processing */ + if ((ret == 0) && ssl->buffers.inputBuffer.dynamicFlag) + ShrinkInputBuffer(ssl, NO_FORCED_FREE); + return ret; + } + /* more records */ WOLFSSL_MSG("More records in input"); } #ifdef WOLFSSL_ASYNC_CRYPT @@ -24102,10 +24025,14 @@ default: if (ret == WC_NO_ERR_TRACE(APP_DATA_READY)) return ret; #endif - /* It is safe to shrink the input buffer here now. local vars will - * be reset to the new starting value. */ - if (ret == 0 && ssl->buffers.inputBuffer.dynamicFlag) + /* It is safe to shrink the input buffer here now, but only + * when done with the current record. During multi-message + * record processing, shrinking would invalidate curStartIdx + * and curSize. */ + if (ssl->options.processReply != runProcessingOneMessage + && ret == 0 && ssl->buffers.inputBuffer.dynamicFlag) { ShrinkInputBuffer(ssl, NO_FORCED_FREE); + } continue; default: WOLFSSL_MSG("Bad process input state, programming error"); @@ -32163,9 +32090,6 @@ static void MakePSKPreMasterSecret(Arrays* arrays, byte use_psk_key) ssl->options.serverState = SERVER_HELLO_COMPLETE; - if (IsEncryptionOn(ssl, 0)) - *inOutIdx += ssl->keys.padSz; - #ifdef HAVE_SECRET_CALLBACK if (ssl->sessionSecretCb != NULL #ifdef HAVE_SESSION_TICKET @@ -32497,9 +32421,6 @@ static void MakePSKPreMasterSecret(Arrays* arrays, byte use_psk_key) ssl->options.sendVerify = SEND_BLANK_CERT; } - if (IsEncryptionOn(ssl, 0)) - *inOutIdx += ssl->keys.padSz; - WOLFSSL_LEAVE("DoCertificateRequest", 0); WOLFSSL_END(WC_FUNC_CERTIFICATE_REQUEST_DO); @@ -33739,9 +33660,6 @@ static int DoServerKeyExchange(WOLFSSL* ssl, const byte* input, case TLS_ASYNC_FINALIZE: { - if (IsEncryptionOn(ssl, 0)) - args->idx += ssl->keys.padSz; - /* Advance state and proceed */ ssl->options.asyncState = TLS_ASYNC_END; } /* case TLS_ASYNC_FINALIZE */ @@ -35509,9 +35427,6 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #endif } - if (IsEncryptionOn(ssl, 0)) - *inOutIdx += ssl->keys.padSz; - ssl->expect_session_ticket = 0; return 0; @@ -39340,9 +39255,6 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl) case TLS_ASYNC_FINALIZE: { - if (IsEncryptionOn(ssl, 0)) - args->idx += ssl->keys.padSz; - ssl->options.havePeerVerify = 1; /* Set final index */ @@ -42404,9 +42316,6 @@ static int DefTicketEncCb(WOLFSSL* ssl, byte key_name[WOLFSSL_TICKET_NAME_SZ], case TLS_ASYNC_FINALIZE: { - if (IsEncryptionOn(ssl, 0)) - args->idx += ssl->keys.padSz; - ret = MakeMasterSecret(ssl); /* Check for error */ diff --git a/src/tls13.c b/src/tls13.c index 1d7e0fd3e1..2b8cdfe189 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -6005,9 +6005,6 @@ static int DoTls13EncryptedExtensions(WOLFSSL* ssl, const byte* input, /* Move index to byte after message. */ *inOutIdx = i + totalExtSz; - /* Always encrypted. */ - *inOutIdx += ssl->keys.padSz; - #ifdef WOLFSSL_EARLY_DATA if (ssl->earlyData != no_early_data) { TLSX* ext = TLSX_Find(ssl->extensions, TLSX_EARLY_DATA); @@ -6148,9 +6145,6 @@ static int DoTls13CertificateRequest(WOLFSSL* ssl, const byte* input, #endif } - /* This message is always encrypted so add encryption padding. */ - *inOutIdx += ssl->keys.padSz; - #ifdef WOLFSSL_POST_HANDSHAKE_AUTH { /* CertReqCtx has one byte at end for context value. @@ -11619,9 +11613,6 @@ static int DoTls13CertificateVerify(WOLFSSL* ssl, byte* input, args->idx += args->sz; *inOutIdx = args->idx; - /* Encryption is always on: add padding */ - *inOutIdx += ssl->keys.padSz; - /* Advance state and proceed */ ssl->options.asyncState = TLS_ASYNC_END; @@ -11819,8 +11810,7 @@ int DoTls13Finished(WOLFSSL* ssl, const byte* input, word32* inOutIdx, } } - /* Force input exhaustion at ProcessReply by consuming padSz. */ - *inOutIdx += size + ssl->keys.padSz; + *inOutIdx += size; #ifndef NO_WOLFSSL_SERVER if (ssl->options.side == WOLFSSL_SERVER_END && @@ -12273,8 +12263,6 @@ static int DoTls13KeyUpdate(WOLFSSL* ssl, const byte* input, word32* inOutIdx, /* Move index to byte after message. */ *inOutIdx += totalSz; - /* Always encrypted. */ - *inOutIdx += ssl->keys.padSz; /* Future traffic uses new decryption keys. */ if ((ret = DeriveTls13Keys(ssl, update_traffic_key, DECRYPT_SIDE_ONLY, 1)) @@ -12425,9 +12413,6 @@ static int DoTls13EndOfEarlyData(WOLFSSL* ssl, const byte* input, ssl->earlyData = done_early_data; - /* Always encrypted. */ - *inOutIdx += ssl->keys.padSz; - ret = SetKeysSide(ssl, DECRYPT_SIDE_ONLY); WOLFSSL_LEAVE("DoTls13EndOfEarlyData", ret); @@ -12599,9 +12584,6 @@ static int DoTls13NewSessionTicket(WOLFSSL* ssl, const byte* input, AddSession(ssl); #endif - /* Always encrypted. */ - *inOutIdx += ssl->keys.padSz; - ssl->expect_session_ticket = 0; #else (void)ssl; @@ -12609,7 +12591,7 @@ static int DoTls13NewSessionTicket(WOLFSSL* ssl, const byte* input, WOLFSSL_ENTER("DoTls13NewSessionTicket"); - *inOutIdx += size + ssl->keys.padSz; + *inOutIdx += size; #endif /* HAVE_SESSION_TICKET */ WOLFSSL_LEAVE("DoTls13NewSessionTicket", 0); @@ -13994,7 +13976,7 @@ int DoTls13HandShakeMsg(WOLFSSL* ssl, byte* input, word32* inOutIdx, input + *inOutIdx - HANDSHAKE_HEADER_SZ, inputLength); ssl->arrays->pendingMsgOffset = inputLength; - *inOutIdx += inputLength + ssl->keys.padSz - HANDSHAKE_HEADER_SZ; + *inOutIdx += inputLength - HANDSHAKE_HEADER_SZ; return 0; } @@ -14018,7 +14000,7 @@ int DoTls13HandShakeMsg(WOLFSSL* ssl, byte* input, word32* inOutIdx, XMEMCPY(ssl->arrays->pendingMsg + ssl->arrays->pendingMsgOffset, input + *inOutIdx, inputLength); ssl->arrays->pendingMsgOffset += inputLength; - *inOutIdx += inputLength + ssl->keys.padSz; + *inOutIdx += inputLength; if (ssl->arrays->pendingMsgOffset == ssl->arrays->pendingMsgSz) { @@ -14033,7 +14015,7 @@ int DoTls13HandShakeMsg(WOLFSSL* ssl, byte* input, word32* inOutIdx, ret == WC_NO_ERR_TRACE(OCSP_WANT_READ)) { /* setup to process fragment again */ ssl->arrays->pendingMsgOffset -= inputLength; - *inOutIdx -= inputLength + ssl->keys.padSz; + *inOutIdx -= inputLength; } else #endif