fix(authn): fix pbkdf2 option validation

apps/emqx/src/emqx_passwd.erl

@@ -83,7 +83,7 @@ do_check_pass({_SimpleHash, _Salt, _SaltPosition} = HashParams, PasswordHash, Pa
     compare_secure(Hash, PasswordHash).
 
 -spec hash(hash_params(), password()) -> password_hash().
-hash({pbkdf2, MacFun, Salt, Iterations, DKLength}, Password) ->
+hash({pbkdf2, MacFun, Salt, Iterations, DKLength}, Password) when Iterations > 0 ->
     case pbkdf2(MacFun, Password, Salt, Iterations, DKLength) of
         {ok, HashPasswd} ->
             hex(HashPasswd);

apps/emqx_auth/src/emqx_authn/emqx_authn_password_hashing.erl

@@ -92,7 +92,7 @@ fields(pbkdf2) ->
             )},
         {iterations,
             sc(
-                integer(),
+                pos_integer(),
                 #{required => true, desc => "Iteration count for PBKDF2 hashing algorithm."}
             )},
         {dk_length, fun dk_length/1}

apps/emqx_auth/test/emqx_authn/emqx_authn_password_hashing_SUITE.erl

@@ -185,3 +185,29 @@ hash_examples() ->
             }
         }
     ].
+
+t_pbkdf2_schema(_Config) ->
+    Config = fun(Iterations) ->
+        #{
+            <<"pbkdf2">> => #{
+                <<"name">> => <<"pbkdf2">>,
+                <<"mac_fun">> => <<"sha">>,
+                <<"iterations">> => Iterations
+            }
+        }
+    end,
+
+    ?assertException(
+        throw,
+        {emqx_authn_password_hashing, _},
+        hocon_tconf:check_plain(emqx_authn_password_hashing, Config(0), #{}, [pbkdf2])
+    ),
+    ?assertException(
+        throw,
+        {emqx_authn_password_hashing, _},
+        hocon_tconf:check_plain(emqx_authn_password_hashing, Config(-1), #{}, [pbkdf2])
+    ),
+    ?assertMatch(
+        #{<<"pbkdf2">> := _},
+        hocon_tconf:check_plain(emqx_authn_password_hashing, Config(1), #{}, [pbkdf2])
+    ).

apps/emqx_auth_mnesia/src/emqx_authn_mnesia.erl

@@ -50,7 +50,7 @@
 %% Internal exports (RPC)
 -export([
     do_destroy/1,
-    do_add_user/2,
+    do_add_user/1,
     do_delete_user/2,
     do_update_user/3,
     import/2,
@@ -187,24 +187,22 @@ import_users({Filename0, FileData}, State) ->
             {error, {unsupported_file_format, Extension}}
     end.
 
-add_user(UserInfo, State) ->
+add_user(
-    trans(fun ?MODULE:do_add_user/2, [UserInfo, State]).
+    UserInfo,
+    State
+) ->
+    UserInfoRecord = user_info_record(UserInfo, State),
+    trans(fun ?MODULE:do_add_user/1, [UserInfoRecord]).
 
 do_add_user(
-    #{
+    #user_info{
-        user_id := UserID,
+        user_id = {_UserGroup, UserID} = DBUserID,
-        password := Password
+        is_superuser = IsSuperuser
-    } = UserInfo,
+    } = UserInfoRecord
-    #{
-        user_group := UserGroup,
-        password_hash_algorithm := Algorithm
-    }
 ) ->
-    case mnesia:read(?TAB, {UserGroup, UserID}, write) of
+    case mnesia:read(?TAB, DBUserID, write) of
         [] ->
-            {PasswordHash, Salt} = emqx_authn_password_hashing:hash(Algorithm, Password),
+            insert_user(UserInfoRecord),
-            IsSuperuser = maps:get(is_superuser, UserInfo, false),
-            insert_user(UserGroup, UserID, PasswordHash, Salt, IsSuperuser),
             {ok, #{user_id => UserID, is_superuser => IsSuperuser}};
         [_] ->
             {error, already_exist}
@@ -222,38 +220,30 @@ do_delete_user(UserID, #{user_group := UserGroup}) ->
     end.
 
 update_user(UserID, UserInfo, State) ->
-    trans(fun ?MODULE:do_update_user/3, [UserID, UserInfo, State]).
+    FieldsToUpdate = fields_to_update(
+        UserInfo,
+        [
+            hash_and_salt,
+            is_superuser
+        ],
+        State
+    ),
+    trans(fun ?MODULE:do_update_user/3, [UserID, FieldsToUpdate, State]).
 
 do_update_user(
     UserID,
-    UserInfo,
+    FieldsToUpdate,
     #{
-        user_group := UserGroup,
+        user_group := UserGroup
-        password_hash_algorithm := Algorithm
     }
 ) ->
     case mnesia:read(?TAB, {UserGroup, UserID}, write) of
         [] ->
             {error, not_found};
-        [
+        [#user_info{} = UserInfoRecord] ->
-            #user_info{
+            NUserInfoRecord = update_user_record(UserInfoRecord, FieldsToUpdate),
-                password_hash = PasswordHash,
+            insert_user(NUserInfoRecord),
-                salt = Salt,
+            {ok, #{user_id => UserID, is_superuser => NUserInfoRecord#user_info.is_superuser}}
-                is_superuser = IsSuperuser
-            }
-        ] ->
-            NSuperuser = maps:get(is_superuser, UserInfo, IsSuperuser),
-            {NPasswordHash, NSalt} =
-                case UserInfo of
-                    #{password := Password} ->
-                        emqx_authn_password_hashing:hash(
-                            Algorithm, Password
-                        );
-                    #{} ->
-                        {PasswordHash, Salt}
-                end,
-            insert_user(UserGroup, UserID, NPasswordHash, NSalt, NSuperuser),
-            {ok, #{user_id => UserID, is_superuser => NSuperuser}}
     end.
 
 lookup_user(UserID, #{user_group := UserGroup}) ->
@@ -391,13 +381,59 @@ get_user_info_by_seq(_, _, _) ->
     {error, bad_format}.
 
 insert_user(UserGroup, UserID, PasswordHash, Salt, IsSuperuser) ->
-    UserInfo = #user_info{
+    UserInfoRecord = user_info_record(UserGroup, UserID, PasswordHash, Salt, IsSuperuser),
+    insert_user(UserInfoRecord).
+
+insert_user(#user_info{} = UserInfoRecord) ->
+    mnesia:write(?TAB, UserInfoRecord, write).
+
+user_info_record(UserGroup, UserID, PasswordHash, Salt, IsSuperuser) ->
+    #user_info{
         user_id = {UserGroup, UserID},
         password_hash = PasswordHash,
         salt = Salt,
         is_superuser = IsSuperuser
-    },
+    }.
-    mnesia:write(?TAB, UserInfo, write).
+
+user_info_record(
+    #{
+        user_id := UserID,
+        password := Password
+    } = UserInfo,
+    #{
+        password_hash_algorithm := Algorithm,
+        user_group := UserGroup
+    } = _State
+) ->
+    IsSuperuser = maps:get(is_superuser, UserInfo, false),
+    {PasswordHash, Salt} = emqx_authn_password_hashing:hash(Algorithm, Password),
+    user_info_record(UserGroup, UserID, PasswordHash, Salt, IsSuperuser).
+
+fields_to_update(
+    #{password := Password} = UserInfo,
+    [hash_and_salt | Rest],
+    #{password_hash_algorithm := Algorithm} = State
+) ->
+    [
+        {hash_and_salt,
+            emqx_authn_password_hashing:hash(
+                Algorithm, Password
+            )}
+        | fields_to_update(UserInfo, Rest, State)
+    ];
+fields_to_update(#{is_superuser := IsSuperuser} = UserInfo, [is_superuser | Rest], State) ->
+    [{is_superuser, IsSuperuser} | fields_to_update(UserInfo, Rest, State)];
+fields_to_update(UserInfo, [_ | Rest], State) ->
+    fields_to_update(UserInfo, Rest, State);
+fields_to_update(_UserInfo, [], _State) ->
+    [].
+
+update_user_record(UserInfoRecord, []) ->
+    UserInfoRecord;
+update_user_record(UserInfoRecord, [{hash_and_salt, {PasswordHash, Salt}} | Rest]) ->
+    update_user_record(UserInfoRecord#user_info{password_hash = PasswordHash, salt = Salt}, Rest);
+update_user_record(UserInfoRecord, [{is_superuser, IsSuperuser} | Rest]) ->
+    update_user_record(UserInfoRecord#user_info{is_superuser = IsSuperuser}, Rest).
 
 %% TODO: Support other type
 get_user_identity(#{username := Username}, username) ->

apps/emqx_auth_mnesia/src/emqx_authn_scram_mnesia.erl

@@ -51,7 +51,7 @@
 %% Internal exports (RPC)
 -export([
     do_destroy/1,
-    do_add_user/2,
+    do_add_user/1,
     do_delete_user/2,
     do_update_user/3
 ]).
@@ -157,19 +157,15 @@ do_destroy(UserGroup) ->
     ).
 
 add_user(UserInfo, State) ->
-    trans(fun ?MODULE:do_add_user/2, [UserInfo, State]).
+    UserInfoRecord = user_info_record(UserInfo, State),
+    trans(fun ?MODULE:do_add_user/1, [UserInfoRecord]).
 
 do_add_user(
-    #{
+    #user_info{user_id = {UserID, _} = DBUserID, is_superuser = IsSuperuser} = UserInfoRecord
-        user_id := UserID,
-        password := Password
-    } = UserInfo,
-    #{user_group := UserGroup} = State
 ) ->
-    case mnesia:read(?TAB, {UserGroup, UserID}, write) of
+    case mnesia:read(?TAB, DBUserID, write) of
         [] ->
-            IsSuperuser = maps:get(is_superuser, UserInfo, false),
+            mnesia:write(?TAB, UserInfoRecord, write),
-            add_user(UserGroup, UserID, Password, IsSuperuser, State),
             {ok, #{user_id => UserID, is_superuser => IsSuperuser}};
         [_] ->
             {error, already_exist}
@@ -187,36 +183,28 @@ do_delete_user(UserID, #{user_group := UserGroup}) ->
     end.
 
 update_user(UserID, User, State) ->
-    trans(fun ?MODULE:do_update_user/3, [UserID, User, State]).
+    FieldsToUpdate = fields_to_update(
+        User,
+        [
+            keys_and_salt,
+            is_superuser
+        ],
+        State
+    ),
+    trans(fun ?MODULE:do_update_user/3, [UserID, FieldsToUpdate, State]).
 
 do_update_user(
     UserID,
-    User,
+    FieldsToUpdate,
-    #{user_group := UserGroup} = State
+    #{user_group := UserGroup} = _State
 ) ->
     case mnesia:read(?TAB, {UserGroup, UserID}, write) of
         [] ->
             {error, not_found};
-        [#user_info{is_superuser = IsSuperuser} = UserInfo] ->
+        [#user_info{} = UserInfo0] ->
-            UserInfo1 = UserInfo#user_info{
+            UserInfo1 = update_user_record(UserInfo0, FieldsToUpdate),
-                is_superuser = maps:get(is_superuser, User, IsSuperuser)
+            mnesia:write(?TAB, UserInfo1, write),
-            },
+            {ok, format_user_info(UserInfo1)}
-            UserInfo2 =
-                case maps:get(password, User, undefined) of
-                    undefined ->
-                        UserInfo1;
-                    Password ->
-                        {StoredKey, ServerKey, Salt} = esasl_scram:generate_authentication_info(
-                            Password, State
-                        ),
-                        UserInfo1#user_info{
-                            stored_key = StoredKey,
-                            server_key = ServerKey,
-                            salt = Salt
-                        }
-                end,
-            mnesia:write(?TAB, UserInfo2, write),
-            {ok, format_user_info(UserInfo2)}
     end.
 
 lookup_user(UserID, #{user_group := UserGroup}) ->
@@ -315,19 +303,56 @@ check_client_final_message(Bin, #{is_superuser := IsSuperuser} = Cache, #{algori
             {error, not_authorized}
     end.
 
-add_user(UserGroup, UserID, Password, IsSuperuser, State) ->
+user_info_record(
-    {StoredKey, ServerKey, Salt} = esasl_scram:generate_authentication_info(Password, State),
+    #{
-    write_user(UserGroup, UserID, StoredKey, ServerKey, Salt, IsSuperuser).
+        user_id := UserID,
+        password := Password
+    } = UserInfo,
+    #{user_group := UserGroup} = State
+) ->
+    IsSuperuser = maps:get(is_superuser, UserInfo, false),
+    user_info_record(UserGroup, UserID, Password, IsSuperuser, State).
 
-write_user(UserGroup, UserID, StoredKey, ServerKey, Salt, IsSuperuser) ->
+user_info_record(UserGroup, UserID, Password, IsSuperuser, State) ->
-    UserInfo = #user_info{
+    {StoredKey, ServerKey, Salt} = esasl_scram:generate_authentication_info(Password, State),
+    #user_info{
         user_id = {UserGroup, UserID},
         stored_key = StoredKey,
         server_key = ServerKey,
         salt = Salt,
         is_superuser = IsSuperuser
-    },
+    }.
-    mnesia:write(?TAB, UserInfo, write).
+
+fields_to_update(
+    #{password := Password} = UserInfo,
+    [keys_and_salt | Rest],
+    State
+) ->
+    {StoredKey, ServerKey, Salt} = esasl_scram:generate_authentication_info(Password, State),
+    [
+        {keys_and_salt, {StoredKey, ServerKey, Salt}}
+        | fields_to_update(UserInfo, Rest, State)
+    ];
+fields_to_update(#{is_superuser := IsSuperuser} = UserInfo, [is_superuser | Rest], State) ->
+    [{is_superuser, IsSuperuser} | fields_to_update(UserInfo, Rest, State)];
+fields_to_update(UserInfo, [_ | Rest], State) ->
+    fields_to_update(UserInfo, Rest, State);
+fields_to_update(_UserInfo, [], _State) ->
+    [].
+
+update_user_record(UserInfoRecord, []) ->
+    UserInfoRecord;
+update_user_record(UserInfoRecord, [{keys_and_salt, {StoredKey, ServerKey, Salt}} | Rest]) ->
+    update_user_record(
+        UserInfoRecord#user_info{
+            stored_key = StoredKey,
+            server_key = ServerKey,
+            salt = Salt
+        },
+        Rest
+    );
+update_user_record(UserInfoRecord, [{is_superuser, IsSuperuser} | Rest]) ->
+    update_user_record(UserInfoRecord#user_info{is_superuser = IsSuperuser}, Rest).
 
 retrieve(UserID, #{user_group := UserGroup}) ->
     case mnesia:dirty_read(?TAB, {UserGroup, UserID}) of

apps/emqx_auth_mnesia/test/emqx_authn_scram_mnesia_SUITE.erl

@@ -314,6 +314,74 @@ t_update_user(_) ->
 
     {ok, #{is_superuser := true}} = emqx_authn_scram_mnesia:lookup_user(<<"u">>, State).
 
+t_update_user_keys(_Config) ->
+    Algorithm = sha512,
+    Username = <<"u">>,
+    Password = <<"p">>,
+
+    init_auth(Username, <<"badpass">>, Algorithm),
+
+    {ok, [#{state := State}]} = emqx_authn_chains:list_authenticators(?GLOBAL),
+
+    emqx_authn_scram_mnesia:update_user(
+        Username,
+        #{password => Password},
+        State
+    ),
+
+    ok = emqx_config:put([mqtt, idle_timeout], 500),
+
+    {ok, Pid} = emqx_authn_mqtt_test_client:start_link("127.0.0.1", 1883),
+
+    ClientFirstMessage = esasl_scram:client_first_message(Username),
+
+    ConnectPacket = ?CONNECT_PACKET(
+        #mqtt_packet_connect{
+            proto_ver = ?MQTT_PROTO_V5,
+            properties = #{
+                'Authentication-Method' => <<"SCRAM-SHA-512">>,
+                'Authentication-Data' => ClientFirstMessage
+            }
+        }
+    ),
+
+    ok = emqx_authn_mqtt_test_client:send(Pid, ConnectPacket),
+
+    ?AUTH_PACKET(
+        ?RC_CONTINUE_AUTHENTICATION,
+        #{'Authentication-Data' := ServerFirstMessage}
+    ) = receive_packet(),
+
+    {continue, ClientFinalMessage, ClientCache} =
+        esasl_scram:check_server_first_message(
+            ServerFirstMessage,
+            #{
+                client_first_message => ClientFirstMessage,
+                password => Password,
+                algorithm => Algorithm
+            }
+        ),
+
+    AuthContinuePacket = ?AUTH_PACKET(
+        ?RC_CONTINUE_AUTHENTICATION,
+        #{
+            'Authentication-Method' => <<"SCRAM-SHA-512">>,
+            'Authentication-Data' => ClientFinalMessage
+        }
+    ),
+
+    ok = emqx_authn_mqtt_test_client:send(Pid, AuthContinuePacket),
+
+    ?CONNACK_PACKET(
+        ?RC_SUCCESS,
+        _,
+        #{'Authentication-Data' := ServerFinalMessage}
+    ) = receive_packet(),
+
+    ok = esasl_scram:check_server_final_message(
+        ServerFinalMessage, ClientCache#{algorithm => Algorithm}
+    ).
+
 t_list_users(_) ->
     Config = config(),
     {ok, State} = emqx_authn_scram_mnesia:create(<<"id">>, Config),

changes/ce/fix-11780.en.md

@@ -0,0 +1 @@
+Fixed validation of the `iterations` field of the `pbkdf2` password hashing algorithm. Now, `iterations` must be strictly positive. Previously, it could be set to 0, which led to a nonfunctional authenticator.