From cf9d6943d52a71a7d8beaf8bb414cb6eeb7ae30b Mon Sep 17 00:00:00 2001 From: JimMoen Date: Tue, 11 Jun 2024 03:06:25 +0800 Subject: [PATCH 1/2] fix: check willretain and willqos when WillFlag set to `true` --- apps/emqx/src/emqx_frame.erl | 19 +++++++++++++++---- apps/emqx/src/emqx_schema.erl | 1 + changes/ce/fix-13222.en.md | 5 +++++ 3 files changed, 21 insertions(+), 4 deletions(-) create mode 100644 changes/ce/fix-13222.en.md diff --git a/apps/emqx/src/emqx_frame.erl b/apps/emqx/src/emqx_frame.erl index 0b02ad1f5..4b4a2d5cf 100644 --- a/apps/emqx/src/emqx_frame.erl +++ b/apps/emqx/src/emqx_frame.erl @@ -287,14 +287,25 @@ parse_connect(FrameBin, StrictMode) -> % Note: return malformed if reserved flag is not 0. parse_connect2( ProtoName, - <>, + <>, StrictMode ) -> case Reserved of 0 -> ok; 1 -> ?PARSE_ERR(reserved_connect_flag) end, + WillFlag = bool(WillFlagB), + WillRetain = bool(WillRetainB), + case WillFlag of + %% MQTT-v3.1.1-[MQTT-3.1.2-13], MQTT-v5.0-[MQTT-3.1.2-11] + false when WillQoS > 0 -> ?PARSE_ERR(invalid_will_qos); + %% MQTT-v3.1.1-[MQTT-3.1.2-14], MQTT-v5.0-[MQTT-3.1.2-12] + true when WillQoS > 2 -> ?PARSE_ERR(invalid_will_qos); + %% MQTT-v3.1.1-[MQTT-3.1.2-15], MQTT-v5.0-[MQTT-3.1.2-13] + false when WillRetain -> ?PARSE_ERR(invalid_will_retain); + _ -> ok + end, {Properties, Rest3} = parse_properties(Rest2, ProtoVer, StrictMode), {ClientId, Rest4} = parse_utf8_string_with_cause(Rest3, StrictMode, invalid_clientid), ConnPacket = #mqtt_packet_connect{ @@ -304,9 +315,9 @@ parse_connect2( %% Invented by mosquitto, named 'try_private': https://mosquitto.org/man/mosquitto-conf-5.html is_bridge = (BridgeTag =:= 8), clean_start = bool(CleanStart), - will_flag = bool(WillFlag), + will_flag = WillFlag, will_qos = WillQoS, - will_retain = bool(WillRetain), + will_retain = WillRetain, keepalive = KeepAlive, properties = Properties, clientid = ClientId diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index 63b77f8d2..9b96ad20a 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -3491,6 +3491,7 @@ mqtt_general() -> )}, {"max_clientid_len", sc( + %% MQTT-v3.1.1-[MQTT-3.1.3-5], MQTT-v5.0-[MQTT-3.1.3-5] range(23, 65535), #{ default => 65535, diff --git a/changes/ce/fix-13222.en.md b/changes/ce/fix-13222.en.md new file mode 100644 index 000000000..0fc7a40ac --- /dev/null +++ b/changes/ce/fix-13222.en.md @@ -0,0 +1,5 @@ +Fix the flags check and error handling related to the Will message in the `CONNECT` packet. +See also: +- MQTT-v3.1.1-[MQTT-3.1.2-13], MQTT-v5.0-[MQTT-3.1.2-11] +- MQTT-v3.1.1-[MQTT-3.1.2-14], MQTT-v5.0-[MQTT-3.1.2-12] +- MQTT-v3.1.1-[MQTT-3.1.2-15], MQTT-v5.0-[MQTT-3.1.2-13] From 675abd7512493abb795c784a074737d93ed5d977 Mon Sep 17 00:00:00 2001 From: JimMoen Date: Tue, 11 Jun 2024 03:26:08 +0800 Subject: [PATCH 2/2] test: will retain and willqos in connect flags --- apps/emqx/test/emqx_frame_SUITE.erl | 55 ++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/apps/emqx/test/emqx_frame_SUITE.erl b/apps/emqx/test/emqx_frame_SUITE.erl index 8193f9c31..2457c3faf 100644 --- a/apps/emqx/test/emqx_frame_SUITE.erl +++ b/apps/emqx/test/emqx_frame_SUITE.erl @@ -64,7 +64,10 @@ groups() -> t_malformed_connect_header, t_malformed_connect_data, t_reserved_connect_flag, - t_invalid_clientid + t_invalid_clientid, + t_undefined_password, + t_invalid_will_retain, + t_invalid_will_qos ]}, {connack, [parallel], [ t_serialize_parse_connack, @@ -738,6 +741,56 @@ t_undefined_password(_) -> ), ok. +t_invalid_will_retain(_) -> + ConnectFlags = <<2#01100000>>, + ConnectBin = + <<16, 51, 0, 4, 77, 81, 84, 84, 5, ConnectFlags/binary, 174, 157, 24, 38, 0, 14, 98, 55, + 122, 51, 83, 73, 89, 50, 54, 79, 77, 73, 65, 86, 0, 5, 66, 117, 53, 57, 66, 0, 6, 84, + 54, 75, 78, 112, 57, 0, 6, 68, 103, 55, 87, 87, 87>>, + ?assertException( + throw, + {frame_parse_error, invalid_will_retain}, + emqx_frame:parse(ConnectBin) + ), + ok. + +t_invalid_will_qos(_) -> + Will_F_WillQoS0 = <<2#010:3, 2#00:2, 2#000:3>>, + Will_F_WillQoS1 = <<2#010:3, 2#01:2, 2#000:3>>, + Will_F_WillQoS2 = <<2#010:3, 2#10:2, 2#000:3>>, + Will_F_WillQoS3 = <<2#010:3, 2#11:2, 2#000:3>>, + Will_T_WillQoS3 = <<2#011:3, 2#11:2, 2#000:3>>, + ConnectBinFun = fun(ConnectFlags) -> + <<16, 51, 0, 4, 77, 81, 84, 84, 5, ConnectFlags/binary, 174, 157, 24, 38, 0, 14, 98, 55, + 122, 51, 83, 73, 89, 50, 54, 79, 77, 73, 65, 86, 0, 5, 66, 117, 53, 57, 66, 0, 6, 84, + 54, 75, 78, 112, 57, 0, 6, 68, 103, 55, 87, 87, 87>> + end, + ?assertMatch( + {ok, _, _, _}, + emqx_frame:parse(ConnectBinFun(Will_F_WillQoS0)) + ), + ?assertException( + throw, + {frame_parse_error, invalid_will_qos}, + emqx_frame:parse(ConnectBinFun(Will_F_WillQoS1)) + ), + ?assertException( + throw, + {frame_parse_error, invalid_will_qos}, + emqx_frame:parse(ConnectBinFun(Will_F_WillQoS2)) + ), + ?assertException( + throw, + {frame_parse_error, invalid_will_qos}, + emqx_frame:parse(ConnectBinFun(Will_F_WillQoS3)) + ), + ?assertException( + throw, + {frame_parse_error, invalid_will_qos}, + emqx_frame:parse(ConnectBinFun(Will_T_WillQoS3)) + ), + ok. + parse_serialize(Packet) -> parse_serialize(Packet, #{strict_mode => true}).