From 983f02ea1bbbbe9fc85b9971e054d2a1e6391342 Mon Sep 17 00:00:00 2001 From: JimMoen Date: Tue, 25 Jun 2024 20:37:54 +0800 Subject: [PATCH 1/3] refactor: separate CONNECT flags validation funcs --- apps/emqx/src/emqx_frame.erl | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/apps/emqx/src/emqx_frame.erl b/apps/emqx/src/emqx_frame.erl index 4b4a2d5cf..873311ed4 100644 --- a/apps/emqx/src/emqx_frame.erl +++ b/apps/emqx/src/emqx_frame.erl @@ -284,28 +284,18 @@ parse_connect(FrameBin, StrictMode) -> end, parse_connect2(ProtoName, Rest, 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, + _ = validate_connect_reserved(Reserved), + _ = validate_connect_will( + WillFlag = bool(WillFlagB), + WillRetain = bool(WillRetainB), + WillQoS + ), {Properties, Rest3} = parse_properties(Rest2, ProtoVer, StrictMode), {ClientId, Rest4} = parse_utf8_string_with_cause(Rest3, StrictMode, invalid_clientid), ConnPacket = #mqtt_packet_connect{ @@ -1133,6 +1123,18 @@ validate_subqos([3 | _]) -> ?PARSE_ERR(bad_subqos); validate_subqos([_ | T]) -> validate_subqos(T); validate_subqos([]) -> ok. +%% MQTT-v3.1.1-[MQTT-3.1.2-3], MQTT-v5.0-[MQTT-3.1.2-3] +validate_connect_reserved(0) -> ok; +validate_connect_reserved(1) -> ?PARSE_ERR(reserved_connect_flag). + +%% MQTT-v3.1.1-[MQTT-3.1.2-13], MQTT-v5.0-[MQTT-3.1.2-11] +validate_connect_will(false, _, WillQos) 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] +validate_connect_will(true, _, WillQoS) 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] +validate_connect_will(false, WillRetain, _) when WillRetain -> ?PARSE_ERR(invalid_will_retain); +validate_connect_will(_, _, _) -> ok. + bool(0) -> false; bool(1) -> true. From 02a9885aa5bac909a4253138422276f8be6790e1 Mon Sep 17 00:00:00 2001 From: JimMoen Date: Tue, 25 Jun 2024 23:13:15 +0800 Subject: [PATCH 2/3] fix(mqtt): check password flag to respect protocol spec --- apps/emqx/src/emqx_frame.erl | 26 +++++++++++++++++++++++--- changes/ce/fix-13334.en.md | 4 ++++ 2 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 changes/ce/fix-13334.en.md diff --git a/apps/emqx/src/emqx_frame.erl b/apps/emqx/src/emqx_frame.erl index 873311ed4..a1c9084dd 100644 --- a/apps/emqx/src/emqx_frame.erl +++ b/apps/emqx/src/emqx_frame.erl @@ -286,7 +286,7 @@ parse_connect(FrameBin, StrictMode) -> parse_connect2( ProtoName, - <>, StrictMode ) -> @@ -296,6 +296,12 @@ parse_connect2( WillRetain = bool(WillRetainB), WillQoS ), + _ = validate_connect_password_flag( + StrictMode, + ProtoVer, + UsernameFlag = bool(UsernameFlagB), + PasswordFlag = bool(PasswordFlagB) + ), {Properties, Rest3} = parse_properties(Rest2, ProtoVer, StrictMode), {ClientId, Rest4} = parse_utf8_string_with_cause(Rest3, StrictMode, invalid_clientid), ConnPacket = #mqtt_packet_connect{ @@ -318,14 +324,14 @@ parse_connect2( fun(Bin) -> parse_utf8_string_with_cause(Bin, StrictMode, invalid_username) end, - bool(UsernameFlag) + UsernameFlag ), {Password, Rest7} = parse_optional( Rest6, fun(Bin) -> parse_utf8_string_with_cause(Bin, StrictMode, invalid_password) end, - bool(PasswordFlag) + PasswordFlag ), case Rest7 of <<>> -> @@ -1135,6 +1141,20 @@ validate_connect_will(true, _, WillQoS) when WillQoS > 2 -> ?PARSE_ERR(invalid_w validate_connect_will(false, WillRetain, _) when WillRetain -> ?PARSE_ERR(invalid_will_retain); validate_connect_will(_, _, _) -> ok. +%% MQTT-v3.1 +%% Username flag and password flag are not strongly related +%% https://public.dhe.ibm.com/software/dw/webservices/ws-mqtt/mqtt-v3r1.html#connect +validate_connect_password_flag(true, ?MQTT_PROTO_V3, _, _) -> + ok; +%% MQTT-v3.1.1-[MQTT-3.1.2-22] +validate_connect_password_flag(true, ?MQTT_PROTO_V4, UsernameFlag, PasswordFlag) -> + %% BUG-FOR-BUG compatible, only check when `strict-mode` + UsernameFlag orelse PasswordFlag andalso ?PARSE_ERR(invalid_password_flag); +validate_connect_password_flag(true, ?MQTT_PROTO_V5, _, _) -> + ok; +validate_connect_password_flag(_, _, _, _) -> + ok. + bool(0) -> false; bool(1) -> true. diff --git a/changes/ce/fix-13334.en.md b/changes/ce/fix-13334.en.md new file mode 100644 index 000000000..e638be9ce --- /dev/null +++ b/changes/ce/fix-13334.en.md @@ -0,0 +1,4 @@ +Check the `PasswordFlag` of the MQTT v3.1.1 CONNECT packet in strict mode to comply with the protocol. + +> [!NOTE] +> To ensure BUG-TO-BUG compatibility, this check is performed only in strict mode. From ed130fdc57a21af8ee687eed55571418b82d2f3f Mon Sep 17 00:00:00 2001 From: JimMoen Date: Tue, 25 Jun 2024 23:14:23 +0800 Subject: [PATCH 3/3] test: MQTT CONNECT flags check --- apps/emqx/test/emqx_frame_SUITE.erl | 31 ++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/apps/emqx/test/emqx_frame_SUITE.erl b/apps/emqx/test/emqx_frame_SUITE.erl index 2457c3faf..9c8a99547 100644 --- a/apps/emqx/test/emqx_frame_SUITE.erl +++ b/apps/emqx/test/emqx_frame_SUITE.erl @@ -706,9 +706,15 @@ t_invalid_clientid(_) -> ). %% for regression: `password` must be `undefined` +%% BUG-FOR-BUG compatible t_undefined_password(_) -> - Payload = <<16, 19, 0, 4, 77, 81, 84, 84, 4, 130, 0, 60, 0, 2, 97, 49, 0, 3, 97, 97, 97>>, - {ok, Packet, <<>>, {none, _}} = emqx_frame:parse(Payload), + %% Username Flag = true + %% Password Flag = false + %% Clean Session = true + ConnectFlags = <<2#1000:4, 2#0010:4>>, + ConnBin = + <<16, 17, 0, 4, 77, 81, 84, 84, 4, ConnectFlags/binary, 0, 60, 0, 2, 97, 49, 0, 1, 97>>, + {ok, Packet, <<>>, {none, _}} = emqx_frame:parse(ConnBin), Password = undefined, ?assertEqual( #mqtt_packet{ @@ -732,7 +738,7 @@ t_undefined_password(_) -> will_props = #{}, will_topic = undefined, will_payload = undefined, - username = <<"aaa">>, + username = <<"a">>, password = Password }, payload = undefined @@ -741,6 +747,25 @@ t_undefined_password(_) -> ), ok. +t_invalid_password_flag(_) -> + %% Username Flag = false + %% Password Flag = true + %% Clean Session = true + ConnectFlags = <<2#0100:4, 2#0010:4>>, + ConnectBin = + <<16, 17, 0, 4, 77, 81, 84, 84, 4, ConnectFlags/binary, 0, 60, 0, 2, 97, 49, 0, 1, 97>>, + ?assertMatch( + {ok, _, _, _}, + emqx_frame:parse(ConnectBin) + ), + + StrictModeParseState = emqx_frame:initial_parse_state(#{strict_mode => true}), + ?assertException( + throw, + {frame_parse_error, invalid_password_flag}, + emqx_frame:parse(ConnectBin, StrictModeParseState) + ). + t_invalid_will_retain(_) -> ConnectFlags = <<2#01100000>>, ConnectBin =