fix: prevent UAF in MQTT packet processing loop#5
Open
goyalpalak18 wants to merge 1 commit intohoneynet:mainfrom
Open
fix: prevent UAF in MQTT packet processing loop#5goyalpalak18 wants to merge 1 commit intohoneynet:mainfrom
goyalpalak18 wants to merge 1 commit intohoneynet:mainfrom
Conversation
Set clientFreed flag before each disconnectClient() call and break out of the for loop immediately after, preventing subsequent iterations from accessing the freed client struct. Guard the post-loop buffer cleanup with the same flag. Signed-off-by: goyalpalak18 <goyalpalak1806@gmail.com>
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
I ran into a critical Use-After-Free (UAF) bug in the MQTT packet processing loop (
servers/mqtt_pit.c) that was causing the tarpit process to crash when handling multi-packet TCP segments.I noticed that automated IoT scanners (like Mirai derivatives or Masscan) routinely concatenate packets like CONNECT+DISCONNECT or DISCONNECT+PING in a single segment. When the tarpit receives these, it crashes, resulting in the silent termination of all active tarpitted connections and the loss of session duration data and packet captures without triggering any metrics alarms.
Root Cause
While debugging the
main()epoll loop, I saw thatdisconnectClient()is called inside several case branches of theswitch(request)statement. This function fully deallocates theclientstruct viafree(). However, the subsequentbreakstatement only exits theswitch, not the enclosingforloop iterating overpacketCount.If a disconnect occurs on iteration
i < packetCount - 1, the next loop iteration executes against the freedclientpointer, causing UAF reads and writes. Additionally, I found that once the loop exits, a post-loop block unconditionally calculates leftover bytes and writes toclient->bytesWrittenToBuffer, overwriting glibc's heap freelist metadata. This corrupts the heap state and crashes the process on the nextmalloc()call (usually the nextaccept()).Changes Proposed
I implemented state tracking for the client pointer's lifetime within the processing loop to fix this:
bool clientFreed = false;flag before the packet processing loop.clientFreed = trueimmediately before the 5disconnectClient()calls inside the switch branches (CONNECT failures, PUBCOMP, PING, DISCONNECT).if (clientFreed) break;check after the switch statement to exit theforloop before the freed pointer is accessed again.if (!clientFreed)to prevent the final heap overwrite.