Skip to content

fix: crash in readPublish when payload is 1 byte#4

Open
goyalpalak18 wants to merge 1 commit intohoneynet:mainfrom
goyalpalak18:fix/mqtt-publish-payload-underflow
Open

fix: crash in readPublish when payload is 1 byte#4
goyalpalak18 wants to merge 1 commit intohoneynet:mainfrom
goyalpalak18:fix/mqtt-publish-payload-underflow

Conversation

@goyalpalak18
Copy link
Copy Markdown

I found a pretty nasty crash in servers/mqtt_pit.c inside readPublish(). Right now, if the honeypot receives a structurally valid MQTT PUBLISH packet with exactly a 1-byte payload, the entire process dies instantly.

What's happening

I noticed that when payloadLen is 1, the payloadLen - 2 calculation for copyLen underflows because it's a uint32_t, wrapping around to 0xFFFFFFFF.

The memcpy right below it survives because it uses a separate inline clamp, but the null-termination step (payload[copyLen] = '\0';) uses the underflowed index. This attempts to write a null byte about 4 GiB past our 512-byte stack buffer, hitting unmapped memory and triggering a SIGSEGV.

Because it crashes instantly, we aren't logging the attacker's IP, and all concurrently tarpitted connections are immediately dropped.

How I fixed it

I removed the - 2 subtraction to prevent the underflow entirely and unified the length logic. Now, both the copy operation and the null-termination use the exact same safely bounded copyLen variable.

Before:

uint32_t copyLen = payloadLen < sizeof(payload) - 1 ? payloadLen - 2 : sizeof(payload) - 1;
memcpy(payload, &buffer[offset], payloadLen < 511 ? payloadLen : 511);
payload[copyLen] = '\0';

After:

uint32_t copyLen = payloadLen < sizeof(payload) - 1 ? payloadLen : sizeof(payload) - 1;
memcpy(payload, &buffer[offset], copyLen);
payload[copyLen] = '\0';

With this patch, I've made sure 1-byte payloads are safely processed, properly null-terminated, and fully logged without taking down the honeypot.

When payloadLen is 1, subtracting 2 from a uint32_t wraps around
to 0xFFFFFFFF and writes a null byte way out of bounds, crashing
the process. Removed the -2 and unified copyLen for both the
memcpy and the null terminator.

Signed-off-by: goyalpalak18 <goyalpalak1806@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant