From cb358ef1de7beb7bff873fabdc60185ef574595d Mon Sep 17 00:00:00 2001 From: Philippe Pittoli Date: Mon, 12 Jun 2023 14:40:03 +0200 Subject: [PATCH] More fine-grained authorizations and remove useless message GetUserByCredentials. --- src/requests/admin.cr | 12 +++++++----- src/requests/contact.cr | 24 ++++++++---------------- src/requests/delete.cr | 19 +++++++++---------- src/requests/list.cr | 7 ++----- src/requests/login.cr | 4 ++++ src/requests/password.cr | 16 ++++++++-------- src/requests/permissions.cr | 10 ++++------ src/requests/profile.cr | 20 +++++++++----------- src/requests/search.cr | 4 ++-- src/requests/users.cr | 32 +++++++------------------------- src/server.cr | 8 ++++++++ 11 files changed, 68 insertions(+), 88 deletions(-) diff --git a/src/requests/admin.cr b/src/requests/admin.cr index 4c4747e..469a0ca 100644 --- a/src/requests/admin.cr +++ b/src/requests/admin.cr @@ -11,10 +11,10 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd - + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - return Response::Error.new "unauthorized (not admin)" unless logged_user.admin + + logged_user.assert_permission("authd", "*", User::PermissionLevel::Admin) if authd.users_per_login.get? @login return Response::Error.new "login already used" @@ -60,7 +60,7 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? user = authd.user? @user @@ -68,7 +68,9 @@ class AuthD::Request # Only an admin can uprank someone. if @admin - return Response::Error.new "unauthorized (not admin)" unless logged_user.admin + logged_user.assert_permission("authd", "*", User::PermissionLevel::Admin) + else + logged_user.assert_permission("authd", "*", User::PermissionLevel::Edit) end @password.try do |s| diff --git a/src/requests/contact.cr b/src/requests/contact.cr index d659136..12497b1 100644 --- a/src/requests/contact.cr +++ b/src/requests/contact.cr @@ -7,26 +7,22 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - # Get the full AuthD::User instance, not just the public view. - user = authd.user? logged_user.uid - return Response::Error.new "unknown user" if user.nil? - if email = @email # FIXME: This *should* require checking the new mail, with # a new activation key and everything else. - user.contact.email = email + logged_user.contact.email = email end if phone = @phone - user.contact.phone = phone + logged_user.contact.phone = phone end - authd.users_per_uid.update user + authd.users_per_uid.update logged_user - Response::UserEdited.new user.uid + Response::UserEdited.new logged_user.uid end end AuthD.requests << EditContacts @@ -36,15 +32,11 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - # Get the full AuthD::User instance, not just the public view. - user = authd.user? logged_user.uid - return Response::Error.new "unknown user" if user.nil? - - _c = user.contact - Response::Contacts.new user.uid, _c.email, _c.phone + _c = logged_user.contact + Response::Contacts.new logged_user.uid, _c.email, _c.phone end end AuthD.requests << GetContacts diff --git a/src/requests/delete.cr b/src/requests/delete.cr index 2e7779e..f8bbf1f 100644 --- a/src/requests/delete.cr +++ b/src/requests/delete.cr @@ -1,23 +1,22 @@ class AuthD::Request IPC::JSON.message Delete, 17 do # Deletion can be triggered by either an admin or the related user. - property user : UserID + property user : UserID | Nil = nil - def initialize(@user) + def initialize(@user = nil) end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - # Get the full AuthD::User instance, not just the public view. - user_to_delete = authd.user? logged_user.uid - return Response::Error.new "unknown user" if user_to_delete.nil? - - unless logged_user.admin - # Is the logged user the target? - return Response::Error.new "invalid credentials" if logged_user.uid != user_to_delete.uid + user_to_delete = if u = @user + logged_user.assert_permission("authd", "*", User::PermissionLevel::Admin) + authd.user? u + else + logged_user end + return Response::Error.new "unknown user" if user_to_delete.nil? # User or admin is now verified: let's proceed with the user deletion. authd.users_per_login.delete user_to_delete.login diff --git a/src/requests/list.cr b/src/requests/list.cr index 3b83dc0..8744e72 100644 --- a/src/requests/list.cr +++ b/src/requests/list.cr @@ -4,14 +4,11 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - user = authd.user? logged_user.uid - return Response::Error.new "user not found" if user.nil? - # Test if the user is a moderator. - user.assert_permission("authd", "*", User::PermissionLevel::Read) + logged_user.assert_permission("authd", "*", User::PermissionLevel::Read) Response::UsersList.new authd.users.to_h.map &.[1].to_public end diff --git a/src/requests/login.cr b/src/requests/login.cr index 3ef8945..56bbb4f 100644 --- a/src/requests/login.cr +++ b/src/requests/login.cr @@ -10,9 +10,13 @@ class AuthD::Request begin user = authd.users_per_login.get @login rescue e : DODB::MissingEntry + # This lack of proper error message is intentional. + # Let attackers try to authenticate themselves with a wrong login. return Response::Error.new "invalid credentials" end + # This line is basically just to please the Crystal's type system. + # No user means DODB::MissingEntry, so it's already covered. return Response::Error.new "invalid credentials" if user.nil? if user.password_hash != authd.hash_password @password diff --git a/src/requests/password.cr b/src/requests/password.cr index c785684..08b4ed8 100644 --- a/src/requests/password.cr +++ b/src/requests/password.cr @@ -6,17 +6,13 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - user = authd.user? logged_user.uid - return Response::Error.new "user not found" if user.nil? + logged_user.password_hash = authd.hash_password @new_password + authd.users_per_uid.update logged_user.uid.to_s, logged_user - user.password_hash = authd.hash_password @new_password - - authd.users_per_uid.update user.uid.to_s, user - - Response::UserEdited.new user.uid + Response::UserEdited.new logged_user.uid end end AuthD.requests << UpdatePassword @@ -31,6 +27,8 @@ class AuthD::Request def handle(authd : AuthD::Service, fd : Int32) user = authd.user? @user + # This is a way for an attacker to know what are the valid logins. + # Not sure I care enough to fix this. return Response::Error.new "user not found" if user.nil? if user.password_renew_key == @password_renew_key @@ -56,6 +54,8 @@ class AuthD::Request def handle(authd : AuthD::Service, fd : Int32) user = authd.user? @user + # This is a way for an attacker to know what are the valid logins. + # Not sure I care enough to fix this. return Response::Error.new "user not found" if user.nil? # Create a new random key for password renewal. diff --git a/src/requests/permissions.cr b/src/requests/permissions.cr index a3ba9a9..1a35a9b 100644 --- a/src/requests/permissions.cr +++ b/src/requests/permissions.cr @@ -8,10 +8,9 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd - + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - return Response::Error.new "unauthorized (not admin)" unless logged_user.admin + logged_user.assert_permission("authd", "*", User::PermissionLevel::Admin) user = authd.user? @user return Response::Error.new "no such user" if user.nil? @@ -40,10 +39,9 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd - + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - return Response::Error.new "unauthorized (not admin)" unless logged_user.admin + logged_user.assert_permission("authd", "*", User::PermissionLevel::Admin) user = authd.user? @user return Response::Error.new "no such user" if user.nil? diff --git a/src/requests/profile.cr b/src/requests/profile.cr index ec38c53..ef12668 100644 --- a/src/requests/profile.cr +++ b/src/requests/profile.cr @@ -6,14 +6,12 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - user = authd.user? logged_user.uid - return Response::Error.new "user not found" if user.nil? - - profile = user.profile || Hash(String, JSON::Any).new + profile = logged_user.profile || Hash(String, JSON::Any).new + # Skip this verification for authd administrators. unless logged_user.admin authd.configuration.read_only_profile_keys.each do |key| if @new_profile[key]? != profile[key]? @@ -22,11 +20,11 @@ class AuthD::Request end end - user.profile = @new_profile + logged_user.profile = @new_profile - authd.users_per_uid.update user.uid.to_s, user + authd.users_per_uid.update logged_user.uid.to_s, logged_user - Response::User.new user.to_public + Response::User.new logged_user.to_public end end AuthD.requests << ReplaceProfile @@ -42,14 +40,14 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? user = if u = @user - raise AdminAuthorizationException.new unless logged_user.admin + logged_user.assert_permission("authd", "*", User::PermissionLevel::Edit) authd.user? u else - authd.user? logged_user.uid + logged_user end return Response::Error.new "user not found" if user.nil? diff --git a/src/requests/search.cr b/src/requests/search.cr index fbba15a..ebe06d5 100644 --- a/src/requests/search.cr +++ b/src/requests/search.cr @@ -6,9 +6,9 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) - logged_user = authd.get_logged_user? fd + logged_user = authd.get_logged_user_full? fd return Response::Error.new "you must be logged" if logged_user.nil? - return Response::Error.new "unauthorized (not admin)" unless logged_user.admin + logged_user.assert_permission("authd", "*", User::PermissionLevel::Read) pattern = Regex.new @user, Regex::Options::IGNORE_CASE diff --git a/src/requests/users.cr b/src/requests/users.cr index 78cff4b..45300a6 100644 --- a/src/requests/users.cr +++ b/src/requests/users.cr @@ -8,6 +8,8 @@ class AuthD::Request def handle(authd : AuthD::Service, fd : Int32) user = authd.user? @user + # This is a way for an attacker to know what are the valid logins. + # Not sure I care enough to fix this. return Response::Error.new "user not found" if user.nil? if user.contact.activation_key.nil? @@ -35,36 +37,16 @@ class AuthD::Request end def handle(authd : AuthD::Service, fd : Int32) + logged_user = authd.get_logged_user? fd + return Response::Error.new "you must be logged" if logged_user.nil? + user = authd.user? @user + # This is a way for an attacker to know what are the valid logins. + # Not sure I care enough to fix this. return Response::Error.new "user not found" if user.nil? Response::User.new user.to_public end end AuthD.requests << GetUser - - IPC::JSON.message GetUserByCredentials, 4 do - property login : String - property password : String - - def initialize(@login, @password) - end - - def handle(authd : AuthD::Service, fd : Int32) - user = authd.users_per_login.get? @login - return Response::Error.new "invalid credentials" unless user - - if authd.hash_password(@password) != user.password_hash - return Response::Error.new "invalid credentials" - end - - user.date_last_connection = Time.local - - # Change the date of the last connection. - authd.users_per_uid.update user.uid.to_s, user - - Response::User.new user.to_public - end - end - AuthD.requests << GetUserByCredentials end diff --git a/src/server.cr b/src/server.cr index 4895db8..544c472 100644 --- a/src/server.cr +++ b/src/server.cr @@ -83,6 +83,14 @@ class AuthD::Service < IPC @logged_users[fd]? end + # Instead of just getting the public view of a logged user, + # get the actual User instance. + def get_logged_user_full?(fd : Int32) + if u = @logged_users[fd]? + user? u.uid + end + end + def user?(uid_or_login : UserID) if uid_or_login.is_a? Int32 @users_per_uid.get? uid_or_login.to_s