bhuvan-somisetty opened a new issue, #13952:
URL: https://github.com/apache/apisix/issues/13952
### 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`:
```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
1. Configure a stream route with the `mqtt-proxy` plugin (`protocol_name =
"MQTT"`, `protocol_level = 5`) in front of any upstream.
2. 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 type
- `0c` - Remaining Length = 12
- `00 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
3. `decode_variable_byte_int` walks 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
