bug: mqtt-proxy stream plugin crashes on malformed MQTT v5 properties length
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 86/100
- Issue type
- Bug
- Clarity
- Clearly specified
- Activity status
- Active
- Tech stack
- lua
- Domain
- backend, testing-qa
Research direction
Start in apisix/stream/plugins/mqtt-proxy.lua at decode_variable_byte_int() and its use in parse_mqtt. Review the malformed-input cases in t/stream-plugin/mqtt-proxy.t and t/stream-plugin/mqtt-proxy2.t, then exercise the supplied truncated MQTT v5 packet. Done means the case is covered by tests and returns 503 with error logging instead of an uncaught Lua runtime error.
Written by the indexing model from the issue text.
Description
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
end
It 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 80
Breaking 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
- Dominant language
- Lua
- Stars
- 17.1k
- Forks
- 2.9k
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 51
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from apache/apisix
-
Difficulty 1/5 1-3 hours Newbie friendliness 88/100
-
bug: syslog plugin crashes with nil-index error instead of returning an error when logger init fails Open
Difficulty 1/5 Under an hour Newbie friendliness 90/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
-
doc good first issue
Difficulty 1/5 1-3 hours Newbie friendliness 76/100
-
doc good first issue
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
api7/lua-resty-saml#62 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
nvim-tree/nvim-tree.lua#3357 · 1 comment ·
-
Difficulty 1/5 Under an hour Newbie friendliness 94/100
codymikol/multiverse.nvim#320 ·
-
Data Correction tbc
Difficulty 2/5 1-3 hours Newbie friendliness 68/100