Merge pull request #9279 from lafirest/fix/binary_to_atom

fix: use binary_to_existing_atom to replace some risky binary_to_atom

apps/emqx/src/emqx_misc.erl

@@ -52,7 +52,9 @@
     explain_posix/1,
     pmap/2,
     pmap/3,
-    readable_error_msg/1
+    readable_error_msg/1,
+    safe_to_existing_atom/1,
+    safe_to_existing_atom/2
 ]).
 
 -export([
@@ -463,6 +465,18 @@ nolink_apply(Fun, Timeout) when is_function(Fun, 0) ->
         exit(timeout)
     end.
 
+safe_to_existing_atom(In) ->
+    safe_to_existing_atom(In, utf8).
+
+safe_to_existing_atom(Bin, Encoding) when is_binary(Bin) ->
+    try_to_existing_atom(fun erlang:binary_to_existing_atom/2, [Bin, Encoding]);
+safe_to_existing_atom(List, _Encoding) when is_list(List) ->
+    try_to_existing_atom(fun erlang:list_to_existing_atom/1, [List]);
+safe_to_existing_atom(Atom, _Encoding) when is_atom(Atom) ->
+    {ok, Atom};
+safe_to_existing_atom(_Any, _Encoding) ->
+    {error, invalid_type}.
+
 %%------------------------------------------------------------------------------
 %% Internal Functions
 %%------------------------------------------------------------------------------
@@ -533,6 +547,14 @@ readable_error_msg(Error) ->
             end
     end.
 
+try_to_existing_atom(Fun, Args) ->
+    try erlang:apply(Fun, Args) of
+        Atom ->
+            {ok, Atom}
+    catch
+        _:Reason -> {error, Reason}
+    end.
+
 -ifdef(TEST).
 -include_lib("eunit/include/eunit.hrl").
 

apps/emqx_bridge/src/emqx_bridge_api.erl

@@ -492,23 +492,16 @@ lookup_from_local_node(BridgeType, BridgeName) ->
             invalid ->
                 {400, error_msg('BAD_REQUEST', <<"invalid operation">>)};
             OperFunc ->
-                TargetNode = binary_to_atom(Node, utf8),
                 ConfMap = emqx:get_config([bridges, BridgeType, BridgeName]),
                 case maps:get(enable, ConfMap, false) of
                     false ->
                         {403,
                             error_msg(
-                                'FORBIDDEN_REQUEST', <<"forbidden operation: bridge disabled">>
+                                'FORBIDDEN_REQUEST',
+                                <<"forbidden operation: bridge disabled">>
                             )};
                     true ->
-                        case emqx_bridge_proto_v1:OperFunc(TargetNode, BridgeType, BridgeName) of
+                        call_operation(Node, OperFunc, BridgeType, BridgeName)
-                            ok ->
-                                {200};
-                            {error, timeout} ->
-                                {503, error_msg('SERVICE_UNAVAILABLE', <<"request timeout">>)};
-                            {error, Reason} ->
-                                {500, error_msg('INTERNAL_ERROR', Reason)}
-                        end
                 end
         end
     ).
@@ -707,3 +700,22 @@ bin(S) when is_atom(S) ->
     atom_to_binary(S, utf8);
 bin(S) when is_binary(S) ->
     S.
+
+call_operation(Node, OperFunc, BridgeType, BridgeName) ->
+    case emqx_misc:safe_to_existing_atom(Node, utf8) of
+        {ok, TargetNode} ->
+            case
+                emqx_bridge_proto_v1:OperFunc(
+                    TargetNode, BridgeType, BridgeName
+                )
+            of
+                ok ->
+                    {200};
+                {error, timeout} ->
+                    {503, error_msg('SERVICE_UNAVAILABLE', <<"request timeout">>)};
+                {error, Reason} ->
+                    {500, error_msg('INTERNAL_ERROR', Reason)}
+            end;
+        {error, _} ->
+            {400, error_msg('INVALID_NODE', <<"invalid node">>)}
+    end.

apps/emqx_bridge/src/emqx_bridge_resource.erl

@@ -59,9 +59,10 @@ bridge_id(BridgeType, BridgeName) ->
     Type = bin(BridgeType),
     <<Type/binary, ":", Name/binary>>.
 
+-spec parse_bridge_id(list() | binary() | atom()) -> {atom(), binary()}.
 parse_bridge_id(BridgeId) ->
     case string:split(bin(BridgeId), ":", all) of
-        [Type, Name] -> {binary_to_atom(Type, utf8), binary_to_atom(Name, utf8)};
+        [Type, Name] -> {binary_to_atom(Type, utf8), Name};
         _ -> error({invalid_bridge_id, BridgeId})
     end.
 

apps/emqx_connector/src/emqx_connector.erl

@@ -78,9 +78,10 @@ connector_id(Type0, Name0) ->
     Name = bin(Name0),
     <<Type/binary, ":", Name/binary>>.
 
+-spec parse_connector_id(binary() | list() | atom()) -> {atom(), binary()}.
 parse_connector_id(ConnectorId) ->
     case string:split(bin(ConnectorId), ":", all) of
-        [Type, Name] -> {binary_to_atom(Type, utf8), binary_to_atom(Name, utf8)};
+        [Type, Name] -> {binary_to_atom(Type, utf8), Name};
         _ -> error({invalid_connector_id, ConnectorId})
     end.
 

apps/emqx_dashboard/src/emqx_dashboard_monitor_api.erl

@@ -131,12 +131,20 @@ monitor(get, #{query_string := QS, bindings := Bindings}) ->
     end.
 
 monitor_current(get, #{bindings := Bindings}) ->
-    NodeOrCluster = binary_to_atom(maps:get(node, Bindings, <<"all">>), utf8),
+    RawNode = maps:get(node, Bindings, all),
-    case emqx_dashboard_monitor:current_rate(NodeOrCluster) of
+    case emqx_misc:safe_to_existing_atom(RawNode, utf8) of
-        {ok, CurrentRate} ->
+        {ok, NodeOrCluster} ->
-            {200, CurrentRate};
+            case emqx_dashboard_monitor:current_rate(NodeOrCluster) of
-        {badrpc, {Node, Reason}} ->
+                {ok, CurrentRate} ->
-            Message = list_to_binary(io_lib:format("Bad node ~p, rpc failed ~p", [Node, Reason])),
+                    {200, CurrentRate};
+                {badrpc, {Node, Reason}} ->
+                    Message = list_to_binary(
+                        io_lib:format("Bad node ~p, rpc failed ~p", [Node, Reason])
+                    ),
+                    {400, 'BAD_RPC', Message}
+            end;
+        {error, _} ->
+            Message = list_to_binary(io_lib:format("Bad node ~p", [RawNode])),
             {400, 'BAD_RPC', Message}
     end.
 

apps/emqx_gateway/src/coap/handler/emqx_coap_pubsub_handler.erl

@@ -121,7 +121,13 @@ apply_publish_opts(Msg, MQTTMsg) ->
             maps:fold(
                 fun
                     (<<"retain">>, V, Acc) ->
-                        Val = erlang:binary_to_atom(V),
+                        Val =
+                            case emqx_misc:safe_to_existing_atom(V) of
+                                {ok, true} ->
+                                    true;
+                                _ ->
+                                    false
+                            end,
                         emqx_message:set_flag(retain, Val, Acc);
                     (<<"expiry">>, V, Acc) ->
                         Val = erlang:binary_to_integer(V),

apps/emqx_gateway/src/emqx_gateway_api_clients.erl

@@ -115,15 +115,19 @@ clients(get, #{
                         ?QUERY_FUN
                     );
                 Node0 ->
-                    Node1 = binary_to_atom(Node0, utf8),
+                    case emqx_misc:safe_to_existing_atom(Node0) of
-                    QStringWithoutNode = maps:without([<<"node">>], QString),
+                        {ok, Node1} ->
-                    emqx_mgmt_api:node_query(
+                            QStringWithoutNode = maps:without([<<"node">>], QString),
-                        Node1,
+                            emqx_mgmt_api:node_query(
-                        QStringWithoutNode,
+                                Node1,
-                        TabName,
+                                QStringWithoutNode,
-                        ?CLIENT_QSCHEMA,
+                                TabName,
-                        ?QUERY_FUN
+                                ?CLIENT_QSCHEMA,
-                    )
+                                ?QUERY_FUN
+                            );
+                        {error, _} ->
+                            {error, Node0, {badrpc, <<"invalid node">>}}
+                    end
             end,
         case Result of
             {error, page_limit_invalid} ->

apps/emqx_management/src/emqx_mgmt_api_clients.erl

@@ -648,15 +648,19 @@ list_clients(QString) ->
                     ?QUERY_FUN
                 );
             Node0 ->
-                Node1 = binary_to_atom(Node0, utf8),
+                case emqx_misc:safe_to_existing_atom(Node0) of
-                QStringWithoutNode = maps:without([<<"node">>], QString),
+                    {ok, Node1} ->
-                emqx_mgmt_api:node_query(
+                        QStringWithoutNode = maps:without([<<"node">>], QString),
-                    Node1,
+                        emqx_mgmt_api:node_query(
-                    QStringWithoutNode,
+                            Node1,
-                    ?CLIENT_QTAB,
+                            QStringWithoutNode,
-                    ?CLIENT_QSCHEMA,
+                            ?CLIENT_QTAB,
-                    ?QUERY_FUN
+                            ?CLIENT_QSCHEMA,
-                )
+                            ?QUERY_FUN
+                        );
+                    {error, _} ->
+                        {error, Node0, {badrpc, <<"invalid node">>}}
+                end
         end,
     case Result of
         {error, page_limit_invalid} ->

apps/emqx_management/src/emqx_mgmt_api_subscriptions.erl

@@ -145,13 +145,18 @@ subscriptions(get, #{query_string := QString}) ->
                     ?QUERY_FUN
                 );
             Node0 ->
-                emqx_mgmt_api:node_query(
+                case emqx_misc:safe_to_existing_atom(Node0) of
-                    binary_to_atom(Node0, utf8),
+                    {ok, Node1} ->
-                    QString,
+                        emqx_mgmt_api:node_query(
-                    ?SUBS_QTABLE,
+                            Node1,
-                    ?SUBS_QSCHEMA,
+                            QString,
-                    ?QUERY_FUN
+                            ?SUBS_QTABLE,
-                )
+                            ?SUBS_QSCHEMA,
+                            ?QUERY_FUN
+                        );
+                    {error, _} ->
+                        {error, Node0, {badrpc, <<"invalid node">>}}
+                end
         end,
     case Response of
         {error, page_limit_invalid} ->

changes/v5.0.10-en.md

@@ -15,6 +15,7 @@
 - Now it is possible to opt out VM internal metrics in prometheus stats [#9222](https://github.com/emqx/emqx/pull/9222).
   When system load is high, reporting too much metrics data may cause the prometheus stats API timeout.
 
+- Improve security when converting types such as `binary` `lists` to `atom` types [#9279](https://github.com/emqx/emqx/pull/9279).
 
 ## Bug fixes
 

changes/v5.0.10-zh.md

@@ -14,6 +14,7 @@
 
 - 可通过配置关闭 prometheus 中的部分内部指标，如果遇到机器负载过高 prometheus 接口返回超时可考虑关闭部分不关心指标，以提高响应速度 [#9222](https://github.com/emqx/emqx/pull/9222）。
 
+- 提升 `binary` 、`list` 等类型转换为 `atom` 类型时的安全性 [#9279](https://github.com/emqx/emqx/pull/9279)。
 
 ## Bug fixes