fix(mqtt-proxy): avoid crash on malformed v5 properties length - #13953
bhuvan-somisetty wants to merge 2 commits into
Conversation
decode_variable_byte_int() read up to 4 bytes without checking they were still inside the peeked buffer. A CONNECT packet whose MQTT v5 properties length field has its continuation bit set right up to the end of the buffer made string.byte() return nothing for the out-of-range index, and bit.band(nil, 127) then raised an unhandled Lua error instead of the usual 503 rejection. Bail out with an error once the loop runs past the data we have, and propagate that failure from parse_mqtt/parse_msg_hdr so preread() rejects the connection the same way it already does for every other malformed packet. Fixes apache#13952
|
A properties length that's a valid number (continuation bit clear) but larger than the packet still crashes, because 7f = properties length 127, so parsing jumps well past the 14-byte buffer. On this branch it gives the same uncaught error / connection reset instead of a 503: The root cause is that If you could alongside address this sibling issue that'd be great, thanks! |
A valid (continuation-bit-clear) v5 properties length that is still larger than the packet advances parsed_pos past the buffer, so the following client_id_len read does str_byte(...) * 256 on nil and crashes. Bounds-check the properties skip and the protocol name / client id length reads the same way the remaining-length parsing already does, so every malformed CONNECT returns 503 instead of aborting the request.
|
Good catch, thanks. Pushed a fix, bounds-checked the protocol name length and client id length reads (both did unchecked |
2 participants
Description
decode_variable_byte_int()in themqtt-proxystream plugin reads up to 4 bytes to decode an MQTT variable-byte-integer, but it never checked that the index it was reading stayed inside the buffer it was given. It's used to decode the MQTT v5 "Properties Length" field, and unlike the client-ID length handling a few lines below it (which correctly checksparsed_pos + client_id_len > #databefore reading), there was no equivalent guard here.If the properties-length bytes have the continuation bit (0x80) set all the way to the end of the peeked buffer,
string.byte()returns nothing for the out-of-range index,byteends upnil, andbit.band(nil, 127)raises an uncaught Lua error. That happens inside theprereadphase, so instead of getting the plugin's normalcore.log.error(...); return 503handling that every other malformed-packet case in this file gets, the connection just errors out.This PR adds the missing bounds check in
decode_variable_byte_int, and propagates the failure up throughparse_mqtt/parse_msg_hdrsopreread()now rejects a malformed/truncated properties length with a clean 503, same as it already does for a bad packet type or a truncated client ID.Which issue(s) this PR fixes:
Fixes #13952
Checklist