fix: prevent UAF after disconnectClient in mqtt event loop#2
Closed
goyalpalak18 wants to merge 1 commit intohoneynet:mainfrom
Closed
fix: prevent UAF after disconnectClient in mqtt event loop#2goyalpalak18 wants to merge 1 commit intohoneynet:mainfrom
goyalpalak18 wants to merge 1 commit intohoneynet:mainfrom
Conversation
Guard all post-disconnect code paths with a clientDisconnected flag so freed client memory is never accessed after teardown. Signed-off-by: goyalpalak18 <goyalpalak1806@gmail.com>
Author
|
Hi @regulartim , please have a look at this. |
|
I have see the #5 pr replicated the corrected version of this pr , so i appreciate this work and requested you to just close this pr to prevent misconception |
Author
|
Got it, no problem. Closing this to avoid any confusion. |
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
While reviewing the main MQTT event loop (
servers/mqtt_pit.c), I found and fixed a guaranteed Heap Use-After-Free (UAF) bug.During the connection teardown, I noticed that
disconnectClient()completely frees themqttClientstruct. The core problem is that it gets called from inside aswitchstatement. When it hitsbreak, it only escapes the switch block, but the outer packet-processingforloop keeps right on running.Because of this, right after the client is freed, the code blindly marches forward and tries to update
client->bytesWrittenToBufferand run amemmoveon the now-freed pointer.Since normal botnets and standard tools (like
mosquitto_pub) cleanly send aDISCONNECTpacket before dropping, this UAF gets triggered reliably during normal honeypot operation. Under load, this causes silent heap corruption or eventual SIGSEGV crashes, which kills the threat intel collection.How I fixed it
I kept the patch straightforward without messing with the core tarpit logic or protocol behavior:
bool clientDisconnected = false;flag scoped to the epoll event.trueright before anydisconnectClient()call inside the switch.if (clientDisconnected) break;after the switch to bail out of the packet loop early.memmove) in anif (!clientDisconnected)check to ensure I only touch memory for live clients.How to test
It's super easy to reproduce the before and after:
mosquitto_pub -t test -m "hello"(this sends a cleanDISCONNECTon exit). Alternatively, just send the raw bytes0xE0 0x00.memmoveorHASH_DEL) or silently corrupt the heap if you hit it with concurrent connections.