Add some missing PKCS7 length checks and bound DTLS 1.3 ACK list.#10833
Open
kareem-wolfssl wants to merge 3 commits into
Open
Add some missing PKCS7 length checks and bound DTLS 1.3 ACK list.#10833kareem-wolfssl wants to merge 3 commits into
kareem-wolfssl wants to merge 3 commits into
Conversation
…dData. Reported by: Jorge Milla (Pig-Tail) <jorge@jmilla.es>
…pedData. Reported by: Jorge Milla (Pig-Tail) <jorge@jmilla.es>
Reported by: Jorge Milla (Pig-Tail) <jorge@jmilla.es>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens input-bound validation in PKCS7 decoding paths and prevents unbounded growth of the DTLS 1.3 ACK handoff queue when using write-dup mode, reducing the risk of out-of-bounds reads and memory growth under repeated scheduled-work cycles.
Changes:
- Add missing bounds checks before copying the EnvelopedData IV and AuthEnvelopedData authTag in non-streaming PKCS7 decode paths.
- Add a guard to cap the aggregate size of the DTLS 1.3 write-dup ACK transfer queue by dropping additional batches once the queue is at the limit.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| wolfcrypt/src/pkcs7.c | Adds buffer-length validation before copying IV/authTag bytes during PKCS7 decode to avoid out-of-bounds reads. |
| src/dtls13.c | Bounds growth of the write-dup ACK list by conditionally splicing/dropping batches during scheduled work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+13614
to
+13618
| /* ensure IV bytes are within the input buffer before copying */ | ||
| if (length < 0 || (word32)length > pkiMsgSz || | ||
| idx > pkiMsgSz - (word32)length) { | ||
| ret = BUFFER_E; | ||
| break; |
Comment on lines
15252
to
15260
| localIdx = idx; | ||
|
|
||
| /* Get authTag OCTET STRING */ | ||
| if (ret == 0 && localIdx >= pkiMsgSz) { | ||
| ret = BUFFER_E; | ||
| } | ||
| if (ret == 0 && pkiMsg[localIdx] != ASN_OCTET_STRING) { | ||
| ret = ASN_PARSE_E; | ||
| } |
Comment on lines
+2819
to
+2839
| /* Walk to the tail, counting what is already queued. */ | ||
| tail = (struct Dtls13RecordNumber**)&ssl->dupWrite->sendAckList; | ||
| while (*tail != NULL) | ||
| while (*tail != NULL) { | ||
| tail = &(*tail)->next; | ||
| *tail = ssl->dtls13Rtx.seenRecords; | ||
| ssl->dtls13Rtx.seenRecords = NULL; | ||
| ssl->dtls13Rtx.seenRecordsCount = 0; | ||
| count++; | ||
| } | ||
| /* Each transferred batch is already capped at | ||
| * DTLS13_ACK_MAX_RECORDS. Only splice a new batch while the queue | ||
| * is below the cap so repeated scheduled-work cycles cannot grow | ||
| * it without bound (aggregate stays under 2 * | ||
| * DTLS13_ACK_MAX_RECORDS). Otherwise drop the batch: ACKs are | ||
| * best-effort and the peer retransmits if one is lost. */ | ||
| if (count < DTLS13_ACK_MAX_RECORDS) { | ||
| *tail = ssl->dtls13Rtx.seenRecords; | ||
| ssl->dtls13Rtx.seenRecords = NULL; | ||
| ssl->dtls13Rtx.seenRecordsCount = 0; | ||
| } | ||
| else { | ||
| /* Queue already at the cap: drop this batch. */ | ||
| Dtls13RtxFlushAcks(ssl); | ||
| } |
Frauschi
approved these changes
Jul 2, 2026
Contributor
|
Jenkins retest this please |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes zd#22079
Testing
Built in tests
Checklist