bug: mqtt-proxy stream plugin crashes on malformed MQTT v5 properties length
Current Behavior
The mqtt-proxy stream plugin can throw an unhandled Lua runtime error while parsing the CONNECT packet, instead of rejecting the connection with a 503 like it does for every other malformed-packet case.
The problem is in decode_variable_byte_int() in apisix/stream/plugins/mqtt-proxy.lua:
local function decode_variable_byte_int(data, offset)
local multiplier = 1
local len = 0
local pos
for i = offset, offset + 3 do
pos = i
local byte = str_byte(data, i, i)
len = len + bit.band(byte, 127) * multiplier
multiplier = multiplier * 128
if bit.band(byte, 128) == 0 then
break
end
end
return len, pos
endIt loops up to 4 bytes reading str_byte(data, i, i) but never checks that i is still inside data. This function is used to decode the MQTT v5 "Properties Length" field (parse_mqtt, line 103), and unlike the client-ID length handling a few lines below it (which correctly checks parsed_pos + client_id_len > #data and bails out), there's no equivalent bounds check here.
If the properties-length bytes have the continuation bit (0x80) set all the way to the end of the buffer that was peeked from the socket, str_byte returns nothing for the out-of-range index, so byte is nil, and bit.band(nil, 127) raises a runtime error. This happens inside the preread phase, so the connection doesn't get the plugin's normal core.log.error(...); return 503 treatment - it just errors out.
Expected Behavior
A malformed/truncated MQTT v5 properties length should be handled the same way the rest of this parser handles malformed input - log an error and return 503 - not raise an uncaught Lua error.
Error Logs
bad argument #1 to 'band' (number expected, got nil)(raised from bit.band(byte, 127) inside decode_variable_byte_int, apisix/stream/plugins/mqtt-proxy.lua:62)
Steps to Reproduce
- Configure a stream route with the
mqtt-proxyplugin (protocol_name = "MQTT",protocol_level = 5) in front of any upstream. - Open a raw TCP connection to the route and send this 14-byte MQTT v5 CONNECT packet (hex):
10 0c 00 04 4d 51 54 54 05 02 00 3c 80 80Breaking it down:
10- CONNECT packet type0c- Remaining Length = 1200 04 4d 51 54 54- protocol name length (4) + "MQTT"05- protocol version 5 (MQTT v5, so the properties field gets parsed)02 00 3c- connect flags + keepalive (skipped)80 80- properties length field, both bytes have the continuation bit set, and the buffer ends right there
decode_variable_byte_intwalks past the end of the 14-byte buffer trying to read a third properties-length byte and errors instead of returning 503.
I checked the two mqtt-proxy test files (t/stream-plugin/mqtt-proxy.t, t/stream-plugin/mqtt-proxy2.t) and none of the existing cases cover a properties length whose continuation bytes run past the end of the peeked data, so this isn't caught by CI right now.
Environment
- APISIX version: master (current HEAD around 3.18)
- Operating system: N/A (code-level issue, not environment specific)
- OpenResty / Nginx version: N/A
- etcd version, if relevant: N/A
- APISIX Dashboard version, if relevant: N/A
- Plugin runner version, for issues related to plugin runners: N/A
- LuaRocks version, for installation issues: N/A
Source: apache/apisix