Fix ssl_DecodePacketInternal chain processing#10017
Open
embhorn wants to merge 1 commit intowolfSSL:masterfrom
Open
Fix ssl_DecodePacketInternal chain processing#10017embhorn wants to merge 1 commit intowolfSSL:masterfrom
embhorn wants to merge 1 commit intowolfSSL:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a heap overflow in ssl_DecodePacketInternal when processing chained input by preventing size_t → int truncation during total-length accumulation, and adds a regression test to ensure oversized chains are rejected.
Changes:
- Accumulate iovec chain length using
size_twith overflow/INT_MAXbounds checks before allocating/copying. - Add a sniffer chain-input regression test covering overflow/oversize scenarios.
- Register the new test case behind the sniffer chain-input feature macros.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/sniffer.c |
Fixes chain-length accumulation to avoid truncation and detect overflow/oversize before allocation/copy. |
tests/api.c |
Adds and registers a regression test that exercises oversize chain inputs. |
Comments suppressed due to low confidence (4)
tests/api.c:1
- The comment says the total is exactly at the INT_MAX boundary, but the configured lengths sum to INT_MAX + 1 (since
chain[1].iov_len = 1andchainSzis 2). Either update the comment to match the actual boundary-overrun being tested, or adjust the inputs to truly hit exactlyINT_MAXif that’s the intended case.
tests/api.c:1 - The test asserts the return code, but it doesn’t verify that
dataremains untouched on error (which is part of the safety guarantee described in the comments), and it also reusesdataacross both calls without resetting/cleaning it. Consider assertingdata == NULLafter each failing call (and/or resettingdatabefore the second call) to both strengthen the regression coverage and avoid inadvertent leaks if behavior changes.
tests/api.c:1 - The test asserts the return code, but it doesn’t verify that
dataremains untouched on error (which is part of the safety guarantee described in the comments), and it also reusesdataacross both calls without resetting/cleaning it. Consider assertingdata == NULLafter each failing call (and/or resettingdatabefore the second call) to both strengthen the regression coverage and avoid inadvertent leaks if behavior changes.
tests/api.c:1 - The test asserts the return code, but it doesn’t verify that
dataremains untouched on error (which is part of the safety guarantee described in the comments), and it also reusesdataacross both calls without resetting/cleaning it. Consider assertingdata == NULLafter each failing call (and/or resettingdatabefore the second call) to both strengthen the regression coverage and avoid inadvertent leaks if behavior changes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Problem: The chain accumulation loop summed size_t (64-bit) iov_len values into an int (32-bit) length, causing silent truncation. An undersized buffer was allocated, then full data was copied causing a heap overflow.
Fixes zd21388
Testing
Added
test_sniffer_chain_input_overflowChecklist