Merge pull request #7318 from zhongwencool/fix-bad-str

fix: Add name string legitimacy check.

CHANGES-4.3.md

@@ -22,7 +22,7 @@ File format:
 * CLI `emqx_ctl pem_cache clean` to force purge x509 certificate cache,
   to force an immediate reload of all certificates after the files are updated on disk.
 * Refactor the ExProto so that anonymous clients can also be displayed on the dashboard [#6983]
-* Force shutdown of processe that cannot answer takeover event [#7026]
+* Force shutdown of processes that cannot answer takeover event [#7026]
 * `topic` parameter in bridge configuration can have `${node}` substitution (just like in `clientid` parameter)
 * Add UTF-8 string validity check in `strict_mode` for MQTT packet.
   When set to true, invalid UTF-8 strings will cause the client to be disconnected. i.e. client ID, topic name. [#7261]
@@ -43,6 +43,7 @@ File format:
 * Fix false alert level log “cannot_find_plugins” caused by duplicate plugin names in `loaded_plugins` files.
 * Prompt user how to change the dashboard's initial default password when emqx start.
 * Fix errno=13 'Permission denied' Cannot create FIFO boot error in Amazon Linux 2022 (el8 package)
+* Fix user or appid created, name only allow `^[A-Za-z]+[A-Za-z0-9-_]*$`
 
 ## v4.3.12
 ### Important changes

apps/emqx_management/src/emqx_mgmt_api_apps.erl

@@ -68,7 +68,7 @@ add_app(_Bindings, Params) ->
     end.
 
 del_app(#{appid := AppId}, _Params) ->
-    case emqx_mgmt_auth:del_app(AppId) of
+    case emqx_mgmt_auth:del_app(emqx_mgmt_util:urldecode(AppId)) of
         ok -> minirest:return();
         {error, Reason} -> minirest:return({error, Reason})
     end.
@@ -77,7 +77,7 @@ list_apps(_Bindings, _Params) ->
     minirest:return({ok, [format(Apps)|| Apps <- emqx_mgmt_auth:list_apps()]}).
 
 lookup_app(#{appid := AppId}, _Params) ->
-    case emqx_mgmt_auth:lookup_app(AppId) of
+    case emqx_mgmt_auth:lookup_app(emqx_mgmt_util:urldecode(AppId)) of
         {AppId, AppSecret, Name, Desc, Status, Expired} ->
             minirest:return({ok, #{app_id => AppId,
                           secret => AppSecret,
@@ -94,7 +94,7 @@ update_app(#{appid := AppId}, Params) ->
     Desc = proplists:get_value(<<"desc">>, Params),
     Status = proplists:get_value(<<"status">>, Params),
     Expired = proplists:get_value(<<"expired">>, Params),
-    case emqx_mgmt_auth:update_app(AppId, Name, Desc, Status, Expired) of
+    case emqx_mgmt_auth:update_app(emqx_mgmt_util:urldecode(AppId), Name, Desc, Status, Expired) of
         ok -> minirest:return();
         {error, Reason} -> minirest:return({error, Reason})
     end.

apps/emqx_management/src/emqx_mgmt_auth.erl

@@ -16,6 +16,8 @@
 
 -module(emqx_mgmt_auth).
 
+-include_lib("emqx/include/logger.hrl").
+
 %% Mnesia Bootstrap
 -export([mnesia/1]).
 -boot_mnesia({mnesia, [boot]}).
@@ -89,6 +91,8 @@ add_app(AppId, Name, Desc, Status, Expired) when is_binary(AppId) ->
       -> {ok, appsecret()}
        | {error, term()}).
 add_app(AppId, Name, Secret, Desc, Status, Expired) when is_binary(AppId) ->
+    case emqx_misc:is_sane_id(AppId) of
+        ok ->
             Secret1 = generate_appsecret_if_need(Secret),
             App = #mqtt_app{id = AppId,
                             secret = Secret1,
@@ -99,17 +103,22 @@ add_app(AppId, Name, Secret, Desc, Status, Expired) when is_binary(AppId) ->
             AddFun = fun() ->
                 case mnesia:wread({mqtt_app, AppId}) of
                     [] -> mnesia:write(App);
-                     _  -> mnesia:abort(alread_existed)
+                    _  -> mnesia:abort(already_existed)
                 end
                      end,
             case mnesia:transaction(AddFun) of
                 {atomic, ok} -> {ok, Secret1};
                 {aborted, Reason} -> {error, Reason}
+            end;
+        {error, Reason} -> {error, Reason}
     end.
 
 force_add_app(AppId, Name, Secret, Desc, Status, Expired) ->
+    case emqx_misc:is_sane_id(AppId) of
+        ok ->
             AddFun = fun() ->
-                 mnesia:write(#mqtt_app{id = AppId,
+                mnesia:write(#mqtt_app{
+                    id = AppId,
                     secret = Secret,
                     name = Name,
                     desc = Desc,
@@ -119,6 +128,8 @@ force_add_app(AppId, Name, Secret, Desc, Status, Expired) ->
             case mnesia:transaction(AddFun) of
                 {atomic, ok} -> ok;
                 {aborted, Reason} -> {error, Reason}
+            end;
+        {error, Reason} -> {error, Reason}
     end.
 
 -spec(generate_appsecret_if_need(binary() | undefined) -> binary()).
@@ -207,4 +218,3 @@ is_authorized(AppId, AppSecret) ->
 
 is_expired(undefined) -> true;
 is_expired(Expired)   -> Expired >= erlang:system_time(second).
-

apps/emqx_management/src/emqx_mgmt_sup.erl

@@ -27,4 +27,3 @@ start_link() ->
 
 init([]) ->
 	{ok, {{one_for_one, 1, 5}, []}}.
-

lib-ce/emqx_dashboard/etc/emqx_dashboard.conf

@@ -3,12 +3,17 @@
 ##--------------------------------------------------------------------
 
 ## Default user's login name.
-##
+## For safety, it should be changed as soon as possible.
+## Please use the './bin/emqx_ctl admins' CLI to change it.
+## Then comment `dashboard.default_user.login/password` from here
 ## Value: String
 dashboard.default_user.login = admin
 
 ## Default user's password.
-##
+## The initial default password for 'dashboard.default_user.login'"
+## For safety, it should be changed as soon as possible.
+## Please use the './bin/emqx_ctl admins' CLI to change it.
+## Then comment `dashboard.default_user.login/password` from here
 ## Value: String
 dashboard.default_user.password = public
 
@@ -127,4 +132,3 @@ dashboard.listener.http.ipv6_v6only = false
 ##
 ## Value: on | off
 ## dashboard.listener.https.honor_cipher_order = on
-

lib-ce/emqx_dashboard/src/emqx_dashboard_admin.erl

@@ -78,18 +78,24 @@ start_link() ->
 
 -spec(add_user(binary(), binary(), binary()) -> ok | {error, any()}).
 add_user(Username, Password, Tags) when is_binary(Username), is_binary(Password) ->
+    case emqx_misc:is_sane_id(Username) of
+        ok ->
             Admin = #mqtt_admin{username = Username, password = hash(Password), tags = Tags},
-    return(mnesia:transaction(fun add_user_/1, [Admin])).
+            return(mnesia:transaction(fun add_user_/1, [Admin]));
+        {error, Reason} -> {error, Reason}
+    end.
 
 force_add_user(Username, Password, Tags) ->
+    case emqx_misc:is_sane_id(Username) of
+        ok ->
             AddFun = fun() ->
-                 mnesia:write(#mqtt_admin{username = Username,
+                mnesia:write(#mqtt_admin{username = Username, password = Password, tags = Tags})
-                                          password = Password,
-                                          tags = Tags})
                      end,
             case mnesia:transaction(AddFun) of
                 {atomic, ok} -> ok;
                 {aborted, Reason} -> {error, Reason}
+            end;
+        {error, Reason} -> {error, Reason}
     end.
 
 %% @private

lib-ce/emqx_dashboard/src/emqx_dashboard_api.erl

@@ -77,9 +77,10 @@ auth(_Bindings, Params) ->
     Password = proplists:get_value(<<"password">>, Params),
     return(emqx_dashboard_admin:check(Username, Password)).
 
-change_pwd(#{username := Username}, Params) ->
+change_pwd(#{username := Username0}, Params) ->
     OldPwd = proplists:get_value(<<"old_pwd">>, Params),
     NewPwd = proplists:get_value(<<"new_pwd">>, Params),
+    Username = emqx_mgmt_util:urldecode(Username0),
     return(emqx_dashboard_admin:change_password(Username, OldPwd, NewPwd)).
 
 create(_Bindings, Params) ->
@@ -96,14 +97,13 @@ list(_Bindings, _Params) ->
 
 update(#{name := Username}, Params) ->
     Tags = proplists:get_value(<<"tags">>, Params),
-    return(emqx_dashboard_admin:update_user(Username, Tags)).
+    return(emqx_dashboard_admin:update_user(emqx_mgmt_util:urldecode(Username), Tags)).
 
 delete(#{name := <<"admin">>}, _Params) ->
     return({error, <<"Cannot delete admin">>});
 
 delete(#{name := Username}, _Params) ->
-    return(emqx_dashboard_admin:remove_user(Username)).
+    return(emqx_dashboard_admin:remove_user(emqx_mgmt_util:urldecode(Username))).
 
 row(#mqtt_admin{username = Username, tags = Tags}) ->
     #{username => Username, tags => Tags}.
-

lib-ce/emqx_dashboard/test/emqx_dashboard_SUITE.erl

@@ -67,9 +67,13 @@ t_overview(_) ->
 t_admins_add_delete(_) ->
     ok = emqx_dashboard_admin:add_user(<<"username">>, <<"password">>, <<"tag">>),
     ok = emqx_dashboard_admin:add_user(<<"username1">>, <<"password1">>, <<"tag1">>),
+    ok = emqx_dashboard_admin:add_user(<<"1username1">>, <<"password1">>, <<"tag1">>),
+    {error, _} = emqx_dashboard_admin:add_user(<<"u/sername1">>, <<"password1">>, <<"tag1">>),
+    {error, _} = emqx_dashboard_admin:add_user(<<"/username1">>, <<"password1">>, <<"tag1">>),
     Admins = emqx_dashboard_admin:all_users(),
-    ?assertEqual(3, length(Admins)),
+    ?assertEqual(4, length(Admins)),
     ok = emqx_dashboard_admin:remove_user(<<"username1">>),
+    ok = emqx_dashboard_admin:remove_user(<<"1username1">>),
     Users = emqx_dashboard_admin:all_users(),
     ?assertEqual(2, length(Users)),
     ok = emqx_dashboard_admin:change_password(<<"username">>, <<"password">>, <<"pwd">>),
@@ -77,13 +81,14 @@ t_admins_add_delete(_) ->
     ?assert(request_dashboard(get, api_path("brokers"), auth_header_("username", "pwd"))),
 
     ok = emqx_dashboard_admin:remove_user(<<"username">>),
-    ?assertNotEqual(true, request_dashboard(get, api_path("brokers"), auth_header_("username", "pwd"))).
+    ?assertNotEqual(true, request_dashboard(get, api_path("brokers"),
+        auth_header_("username", "pwd"))).
 
 t_rest_api(_Config) ->
     {ok, Res0} = http_get("users"),
-
+    Users = get_http_data(Res0),
-    ?assertEqual([#{<<"username">> => <<"admin">>,
+    ?assert(lists:member(#{<<"username">> => <<"admin">>, <<"tags">> => <<"administrator">>},
-                    <<"tags">> => <<"administrator">>}], get_http_data(Res0)),
+        Users)),
 
     AssertSuccess = fun({ok, Res}) ->
                         ?assertEqual(#{<<"code">> => 0}, json(Res))
@@ -165,4 +170,3 @@ api_path(Path) ->
 
 json(Data) ->
     {ok, Jsx} = emqx_json:safe_decode(Data, [return_maps]), Jsx.
-

src/emqx.appup.src

@@ -13,10 +13,12 @@
      {load_module,emqx_app,brutal_purge,soft_purge,[]},
      {load_module,emqx_ctl,brutal_purge,soft_purge,[]},
      {load_module,emqx_cm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_connection,brutal_purge,soft_purge,[]}]},
    {"4.3.12",
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_frame,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys_mon,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
@@ -40,6 +42,7 @@
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys,brutal_purge,soft_purge,[]},
      {load_module,emqx_cm,brutal_purge,soft_purge,[]},
      {load_module,emqx_vm_mon,brutal_purge,soft_purge,[]},
@@ -63,6 +66,7 @@
    {"4.3.10",
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys,brutal_purge,soft_purge,[]},
      {load_module,emqx_cm,brutal_purge,soft_purge,[]},
@@ -87,6 +91,7 @@
    {"4.3.9",
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys,brutal_purge,soft_purge,[]},
      {load_module,emqx_vm_mon,brutal_purge,soft_purge,[]},
@@ -115,6 +120,7 @@
    {"4.3.8",
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys,brutal_purge,soft_purge,[]},
      {load_module,emqx_vm_mon,brutal_purge,soft_purge,[]},
@@ -410,6 +416,7 @@
      {load_module,emqx_frame,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys_mon,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_os_mon,brutal_purge,soft_purge,[]},
      {load_module,emqx_channel,brutal_purge,soft_purge,[]},
@@ -422,6 +429,7 @@
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_frame,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys_mon,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys,brutal_purge,soft_purge,[]},
@@ -442,6 +450,7 @@
    {"4.3.11",
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys,brutal_purge,soft_purge,[]},
      {load_module,emqx_cm,brutal_purge,soft_purge,[]},
@@ -465,6 +474,7 @@
    {"4.3.10",
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys,brutal_purge,soft_purge,[]},
      {load_module,emqx_cm,brutal_purge,soft_purge,[]},
@@ -488,6 +498,7 @@
    {"4.3.9",
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys,brutal_purge,soft_purge,[]},
      {load_module,emqx_vm_mon,brutal_purge,soft_purge,[]},
@@ -515,6 +526,7 @@
    {"4.3.8",
     [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_pmon,brutal_purge,soft_purge,[]},
+     {load_module,emqx_misc,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_sys,brutal_purge,soft_purge,[]},
      {load_module,emqx_vm_mon,brutal_purge,soft_purge,[]},

src/emqx_misc.erl

@@ -52,6 +52,26 @@
         , hexstr2bin/1
         ]).
 
+-export([ is_sane_id/1
+        ]).
+
+-define(VALID_STR_RE, "^[A-Za-z0-9]+[A-Za-z0-9-_]*$").
+
+-spec is_sane_id(list() | binary()) -> ok | {error, Reason::binary()}.
+is_sane_id(Str) ->
+    StrLen = len(Str),
+    case StrLen > 0 andalso StrLen =< 256 of
+        true ->
+            case re:run(Str, ?VALID_STR_RE) of
+                nomatch -> {error, <<"required: " ?VALID_STR_RE>>};
+                _ -> ok
+            end;
+        false -> {error, <<"0 < Length =< 256">>}
+    end.
+
+len(Bin) when is_binary(Bin) -> byte_size(Bin);
+len(Str) when is_list(Str) -> length(Str).
+
 -define(OOM_FACTOR, 1.25).
 
 %% @doc Parse v4 or v6 string format address to tuple.
@@ -309,4 +329,30 @@ hexchar2int(I) when I >= $a andalso I =< $f -> I - $a + 10.
 ipv6_probe_test() ->
     ?assertEqual([{ipv6_probe, true}], ipv6_probe([])).
 
+is_sane_id_test() ->
+    ?assertMatch({error, _}, is_sane_id("")),
+    ?assertMatch({error, _}, is_sane_id("_")),
+    ?assertMatch({error, _}, is_sane_id("_aaa")),
+    ?assertMatch({error, _}, is_sane_id("lkad/oddl")),
+    ?assertMatch({error, _}, is_sane_id("lkad*oddl")),
+    ?assertMatch({error, _}, is_sane_id("script>lkadoddl")),
+    ?assertMatch({error, _}, is_sane_id("<script>lkadoddl")),
+
+    ?assertMatch(ok, is_sane_id(<<"Abckdf_lkdfd_1222">>)),
+    ?assertMatch(ok, is_sane_id("Abckdf_lkdfd_1222")),
+    ?assertMatch(ok, is_sane_id("abckdf_lkdfd_1222")),
+    ?assertMatch(ok, is_sane_id("abckdflkdfd1222")),
+    ?assertMatch(ok, is_sane_id("abckdflkdf")),
+    ?assertMatch(ok, is_sane_id("a1122222")),
+    ?assertMatch(ok, is_sane_id("1223333434")),
+    ?assertMatch(ok, is_sane_id("1lkdfaldk")),
+
+    Ok = lists:flatten(lists:duplicate(256, "a")),
+    Bad = Ok ++ "a",
+    ?assertMatch(ok, is_sane_id(Ok)),
+    ?assertMatch(ok, is_sane_id(list_to_binary(Ok))),
+    ?assertMatch({error, _}, is_sane_id(Bad)),
+    ?assertMatch({error, _}, is_sane_id(list_to_binary(Bad))),
+    ok.
+
 -endif.