From edde661da3f31c1868e8ccd5f52865eb9b15b3ed Mon Sep 17 00:00:00 2001 From: Ilya Averyanov Date: Tue, 17 Oct 2023 19:45:56 +0300 Subject: [PATCH] fix(authn): fix pbkdf2 option validation --- apps/emqx/src/emqx_passwd.erl | 2 +- .../emqx_authn_password_hashing.erl | 2 +- .../emqx_authn_password_hashing_SUITE.erl | 26 ++++ .../src/emqx_authn_mnesia.erl | 118 ++++++++++++------ .../src/emqx_authn_scram_mnesia.erl | 105 ++++++++++------ .../test/emqx_authn_scram_mnesia_SUITE.erl | 68 ++++++++++ changes/ce/fix-11780.en.md | 1 + 7 files changed, 239 insertions(+), 83 deletions(-) create mode 100644 changes/ce/fix-11780.en.md diff --git a/apps/emqx/src/emqx_passwd.erl b/apps/emqx/src/emqx_passwd.erl index c68a146ed..1232dfcb4 100644 --- a/apps/emqx/src/emqx_passwd.erl +++ b/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); diff --git a/apps/emqx_auth/src/emqx_authn/emqx_authn_password_hashing.erl b/apps/emqx_auth/src/emqx_authn/emqx_authn_password_hashing.erl index 40e96ce6f..756f39d06 100644 --- a/apps/emqx_auth/src/emqx_authn/emqx_authn_password_hashing.erl +++ b/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} diff --git a/apps/emqx_auth/test/emqx_authn/emqx_authn_password_hashing_SUITE.erl b/apps/emqx_auth/test/emqx_authn/emqx_authn_password_hashing_SUITE.erl index 83b923d0e..ac3186bea 100644 --- a/apps/emqx_auth/test/emqx_authn/emqx_authn_password_hashing_SUITE.erl +++ b/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]) + ). diff --git a/apps/emqx_auth_mnesia/src/emqx_authn_mnesia.erl b/apps/emqx_auth_mnesia/src/emqx_authn_mnesia.erl index 8e59d94e7..bbbaeddb1 100644 --- a/apps/emqx_auth_mnesia/src/emqx_authn_mnesia.erl +++ b/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) -> - trans(fun ?MODULE:do_add_user/2, [UserInfo, State]). +add_user( + UserInfo, + State +) -> + UserInfoRecord = user_info_record(UserInfo, State), + trans(fun ?MODULE:do_add_user/1, [UserInfoRecord]). do_add_user( - #{ - user_id := UserID, - password := Password - } = UserInfo, - #{ - user_group := UserGroup, - password_hash_algorithm := Algorithm - } + #user_info{ + user_id = {_UserGroup, UserID} = DBUserID, + is_superuser = IsSuperuser + } = UserInfoRecord ) -> - case mnesia:read(?TAB, {UserGroup, UserID}, write) of + case mnesia:read(?TAB, DBUserID, write) of [] -> - {PasswordHash, Salt} = emqx_authn_password_hashing:hash(Algorithm, Password), - IsSuperuser = maps:get(is_superuser, UserInfo, false), - insert_user(UserGroup, UserID, PasswordHash, Salt, IsSuperuser), + insert_user(UserInfoRecord), {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, - password_hash_algorithm := Algorithm + user_group := UserGroup } ) -> case mnesia:read(?TAB, {UserGroup, UserID}, write) of [] -> {error, not_found}; - [ - #user_info{ - password_hash = PasswordHash, - salt = Salt, - 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}} + [#user_info{} = UserInfoRecord] -> + NUserInfoRecord = update_user_record(UserInfoRecord, FieldsToUpdate), + insert_user(NUserInfoRecord), + {ok, #{user_id => UserID, is_superuser => NUserInfoRecord#user_info.is_superuser}} 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) -> diff --git a/apps/emqx_auth_mnesia/src/emqx_authn_scram_mnesia.erl b/apps/emqx_auth_mnesia/src/emqx_authn_scram_mnesia.erl index 641efcf74..a66ae5786 100644 --- a/apps/emqx_auth_mnesia/src/emqx_authn_scram_mnesia.erl +++ b/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_id := UserID, - password := Password - } = UserInfo, - #{user_group := UserGroup} = State + #user_info{user_id = {UserID, _} = DBUserID, is_superuser = IsSuperuser} = UserInfoRecord ) -> - case mnesia:read(?TAB, {UserGroup, UserID}, write) of + case mnesia:read(?TAB, DBUserID, write) of [] -> - IsSuperuser = maps:get(is_superuser, UserInfo, false), - add_user(UserGroup, UserID, Password, IsSuperuser, State), + mnesia:write(?TAB, UserInfoRecord, write), {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, - #{user_group := UserGroup} = State + FieldsToUpdate, + #{user_group := UserGroup} = _State ) -> case mnesia:read(?TAB, {UserGroup, UserID}, write) of [] -> {error, not_found}; - [#user_info{is_superuser = IsSuperuser} = UserInfo] -> - UserInfo1 = UserInfo#user_info{ - is_superuser = maps:get(is_superuser, User, IsSuperuser) - }, - 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)} + [#user_info{} = UserInfo0] -> + UserInfo1 = update_user_record(UserInfo0, FieldsToUpdate), + mnesia:write(?TAB, UserInfo1, write), + {ok, format_user_info(UserInfo1)} 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) -> - {StoredKey, ServerKey, Salt} = esasl_scram:generate_authentication_info(Password, State), - write_user(UserGroup, UserID, StoredKey, ServerKey, Salt, IsSuperuser). +user_info_record( + #{ + 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) -> - UserInfo = #user_info{ +user_info_record(UserGroup, UserID, Password, IsSuperuser, State) -> + {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 diff --git a/apps/emqx_auth_mnesia/test/emqx_authn_scram_mnesia_SUITE.erl b/apps/emqx_auth_mnesia/test/emqx_authn_scram_mnesia_SUITE.erl index abd5518a6..39350e4b9 100644 --- a/apps/emqx_auth_mnesia/test/emqx_authn_scram_mnesia_SUITE.erl +++ b/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), diff --git a/changes/ce/fix-11780.en.md b/changes/ce/fix-11780.en.md new file mode 100644 index 000000000..549707ffb --- /dev/null +++ b/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.