Merge pull request #13334 from JimMoen/fix-mqtt-username-password-flag

fix(mqtt): check password flag to respect protocol spec

apps/emqx/src/emqx_frame.erl

@@ -284,28 +284,24 @@ parse_connect(FrameBin, StrictMode) ->
     end,
     parse_connect2(ProtoName, Rest, StrictMode).
 
-% Note: return malformed if reserved flag is not 0.
 parse_connect2(
     ProtoName,
-    <<BridgeTag:4, ProtoVer:4, UsernameFlag:1, PasswordFlag:1, WillRetainB:1, WillQoS:2,
+    <<BridgeTag:4, ProtoVer:4, UsernameFlagB:1, PasswordFlagB:1, WillRetainB:1, WillQoS:2,
         WillFlagB:1, CleanStart:1, Reserved:1, KeepAlive:16/big, Rest2/binary>>,
     StrictMode
 ) ->
-    case Reserved of
+    _ = validate_connect_reserved(Reserved),
-        0 -> ok;
+    _ = validate_connect_will(
-        1 -> ?PARSE_ERR(reserved_connect_flag)
-    end,
         WillFlag = bool(WillFlagB),
         WillRetain = bool(WillRetainB),
-    case WillFlag of
+        WillQoS
-        %% 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);
+    _ = validate_connect_password_flag(
-        %% MQTT-v3.1.1-[MQTT-3.1.2-14], MQTT-v5.0-[MQTT-3.1.2-12]
+        StrictMode,
-        true when WillQoS > 2 -> ?PARSE_ERR(invalid_will_qos);
+        ProtoVer,
-        %% MQTT-v3.1.1-[MQTT-3.1.2-15], MQTT-v5.0-[MQTT-3.1.2-13]
+        UsernameFlag = bool(UsernameFlagB),
-        false when WillRetain -> ?PARSE_ERR(invalid_will_retain);
+        PasswordFlag = bool(PasswordFlagB)
-        _ -> ok
+    ),
-    end,
     {Properties, Rest3} = parse_properties(Rest2, ProtoVer, StrictMode),
     {ClientId, Rest4} = parse_utf8_string_with_cause(Rest3, StrictMode, invalid_clientid),
     ConnPacket = #mqtt_packet_connect{
@@ -328,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
         <<>> ->
@@ -1133,6 +1129,32 @@ 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.
+
+%% 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.
 

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>>,
+    %% Username Flag = true
-    {ok, Packet, <<>>, {none, _}} = emqx_frame:parse(Payload),
+    %% 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 =

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.