Fix Null Pointer Dereference in TCP Packet Handling #36751
+36
−1
Conversation
This file contains 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
|
Review changes with |
|
PR #36751: Size comparison from 9e671ad to 404db44 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Dec 9, 2024
|
PR #36751: Size comparison from 5d42d92 to 6bfe4f5 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
pidarped
approved these changes
Dec 10, 2024
| if (messageSize == 0) | ||
| { | ||
| // No payload but considered a valid message. Return success to keep the connection alive. | ||
| return CHIP_NO_ERROR; |
There was a problem hiding this comment.
But ... doesn't this leave the four bytes 0, 0, 0, 0 in state->mReceived? As in, shouldn't this check-and-return be after the Consume call?
There was a problem hiding this comment.
That is right. Good catch! Since this is a complete packet, the size field should be consumed first(contrary to the previous if-check-and-return).
There was a problem hiding this comment.
Based on the earlier if statements, the messageSize == 0 check was placed here, assuming it was the correct position. However, since this case assumes the message is a complete message, should this check actually come after the Consume call instead?
There was a problem hiding this comment.
Yes, the Consume() of the 4 byte MessageSize field has to happen before the check to throw away this 0-payload packet and move forward along the byte stream.
andy31415
requested changes
Dec 13, 2024
There was a problem hiding this comment.
The Testing section here seems to contain a "manual" entry. Please add more details (how does one test with "malformed packets" to reproduce the test) and add a justification why automated testing of this code is impossible.
We are intentionally trying to make the "manually tested" threshold high to avoid PRs being marked as "manually tested" because it is easier to do so.
Since this is a "Processed buffer" call, it does seem that this could be unit tested.
BoB13-Matter
force-pushed
the
fix-tcp-nullptr-dereference
branch
from
6bfe4f5 to
f494cd9
Compare
Thanks. I have added a new test in TestTCP.cpp to validate the behavior with malformed packets. Let me know if further modifications are needed. |
|
PR #36751: Size comparison from 4e44586 to cca52cd Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
approved these changes
Dec 17, 2024
|
@BoB13-Matter this may need more fixes. Test fails on linux: |
|
PR #36751: Size comparison from 4e44586 to d9316a2 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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
This PR fixes #36750 a null pointer dereference in
TCPBase::ProcessSingleMessagecaused by malformed TCP packets. The issue could lead to a Denial of Service (DoS) attack.Changes
Added a null pointer validation for
state->mReceivedbefore callingProcessSingleMessage.This change prevents crash and print error log:
Testing
onoff on 1 1 --allow-large-payload 1)