From 28b9f84eb7f5c266d36efe226801ec1fa4d80ade Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Fri, 15 Mar 2024 14:31:46 +0100 Subject: [PATCH] add 'confirmation-password' parameter to user password change API/UI Similar to a recent change in pve-access-control [0], add a new 'confirmation-password' parameter to the change-password endpoint and require non-root users to confirm their passwords. Doing so avoids that an attacker that has direct access to a computer where a user is logged in to the PVE interface can change the password of said user and thus either prolong their possibility to attack, and/or create a denial of service situation, where the original user cannot login into the PVE host using their old credentials. Note that this might sound worse than it is, as for this attack to work the attacker needs either: - physical access to an unlocked computer that is currently logged in to a PVE host - having taken over such a computer already through some unrelated vulnerability As these required pre-conditions are pretty big implications, which allow (temporary) access to all of the resources (including PVE ones) that the user can control, we see this as slight improvement that won't hurt, might protect one in some specific cases that is simply too cheap not to do. For now we avoid additional confirmation through a second factor, as that is a much higher complexity without that much gain, and some forms like (unauthenticated) button press on a WebAuthn token or the TOTP code would be easy to circumvent in the physical access case and in the local access case one might be able to MITM themselves too. [0]: https://git.proxmox.com/?p=pve-access-control.git;a=commit;h=5bcf553e3a193a537d92498f4fee3c23e22d1741 Reported-by: Wouter Arts Signed-off-by: Wolfgang Bumiller [ TL: Extend ocmmit message, squash in UI change ] Signed-off-by: Thomas Lamprecht --- src/api2/access/mod.rs | 62 ++++++++++++++++++++++++++++++++++++++--- src/api2/access/tfa.rs | 50 ++++----------------------------- src/api2/access/user.rs | 6 +++- www/config/UserView.js | 1 + 4 files changed, 69 insertions(+), 50 deletions(-) diff --git a/src/api2/access/mod.rs b/src/api2/access/mod.rs index 673e76bb..15509fd9 100644 --- a/src/api2/access/mod.rs +++ b/src/api2/access/mod.rs @@ -6,13 +6,15 @@ use serde_json::Value; use std::collections::HashMap; use std::collections::HashSet; -use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap}; +use proxmox_router::{ + http_bail, http_err, list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap, +}; use proxmox_schema::api; use proxmox_sortable_macro::sortable; use pbs_api_types::{ - Authid, Userid, ACL_PATH_SCHEMA, PASSWORD_SCHEMA, PRIVILEGES, PRIV_PERMISSIONS_MODIFY, - PRIV_SYS_AUDIT, + Authid, User, Userid, ACL_PATH_SCHEMA, PASSWORD_FORMAT, PASSWORD_SCHEMA, PRIVILEGES, + PRIV_PERMISSIONS_MODIFY, PRIV_SYS_AUDIT, }; use pbs_config::acl::AclTreeNode; use pbs_config::CachedUserInfo; @@ -24,6 +26,47 @@ pub mod role; pub mod tfa; pub mod user; +/// Perform first-factor (password) authentication only. Ignore password for the root user. +/// Otherwise check the current user's password. +/// +/// This means that user admins need to type in their own password while editing a user, and +/// regular users, which can only change their own settings (checked at the API level), can change +/// their own settings using their own password. +pub(self) async fn user_update_auth>( + rpcenv: &mut dyn RpcEnvironment, + userid: &Userid, + password: Option, + must_exist: bool, +) -> Result<(), Error> { + let authid: Authid = rpcenv.get_auth_id().unwrap().parse()?; + + if authid.user() != Userid::root_userid() { + let client_ip = rpcenv.get_client_ip().map(|sa| sa.ip()); + let password = password.ok_or_else(|| http_err!(UNAUTHORIZED, "missing password"))?; + #[allow(clippy::let_unit_value)] + { + let _: () = crate::auth::authenticate_user( + authid.user(), + password.as_ref(), + client_ip.as_ref(), + ) + .await + .map_err(|err| http_err!(UNAUTHORIZED, "{}", err))?; + } + } + + // After authentication, verify that the to-be-modified user actually exists: + if must_exist && authid.user() != userid { + let (config, _digest) = pbs_config::user::config()?; + + if config.lookup::("user", userid.as_str()).is_err() { + http_bail!(UNAUTHORIZED, "user '{}' does not exists.", userid); + } + } + + Ok(()) +} + #[api( protected: true, input: { @@ -34,6 +77,14 @@ pub mod user; password: { schema: PASSWORD_SCHEMA, }, + "confirmation-password": { + type: String, + description: "The current password for confirmation, unless logged in as root@pam", + min_length: 1, + max_length: 1024, + format: &PASSWORD_FORMAT, + optional: true, + }, }, }, access: { @@ -45,11 +96,14 @@ pub mod user; /// /// Each user is allowed to change his own password. Superuser /// can change all passwords. -pub fn change_password( +pub async fn change_password( userid: Userid, password: String, + confirmation_password: Option, rpcenv: &mut dyn RpcEnvironment, ) -> Result { + user_update_auth(rpcenv, &userid, confirmation_password, true).await?; + let current_auth: Authid = rpcenv .get_auth_id() .ok_or_else(|| format_err!("no authid available"))? diff --git a/src/api2/access/tfa.rs b/src/api2/access/tfa.rs index 589535a6..e6b26b33 100644 --- a/src/api2/access/tfa.rs +++ b/src/api2/access/tfa.rs @@ -2,55 +2,15 @@ use anyhow::Error; -use proxmox_router::{http_bail, http_err, Permission, Router, RpcEnvironment}; +use proxmox_router::{http_bail, Permission, Router, RpcEnvironment}; use proxmox_schema::api; use proxmox_tfa::api::methods; -use pbs_api_types::{ - Authid, User, Userid, PASSWORD_SCHEMA, PRIV_PERMISSIONS_MODIFY, PRIV_SYS_AUDIT, -}; +use pbs_api_types::{Authid, Userid, PASSWORD_SCHEMA, PRIV_PERMISSIONS_MODIFY, PRIV_SYS_AUDIT}; use pbs_config::CachedUserInfo; use crate::config::tfa::UserAccess; -/// Perform first-factor (password) authentication only. Ignore password for the root user. -/// Otherwise check the current user's password. -/// -/// This means that user admins need to type in their own password while editing a user, and -/// regular users, which can only change their own TFA settings (checked at the API level), can -/// change their own settings using their own password. -async fn tfa_update_auth( - rpcenv: &mut dyn RpcEnvironment, - userid: &Userid, - password: Option, - must_exist: bool, -) -> Result<(), Error> { - let authid: Authid = rpcenv.get_auth_id().unwrap().parse()?; - - if authid.user() != Userid::root_userid() { - let client_ip = rpcenv.get_client_ip().map(|sa| sa.ip()); - let password = password.ok_or_else(|| http_err!(UNAUTHORIZED, "missing password"))?; - #[allow(clippy::let_unit_value)] - { - let _: () = - crate::auth::authenticate_user(authid.user(), &password, client_ip.as_ref()) - .await - .map_err(|err| http_err!(UNAUTHORIZED, "{}", err))?; - } - } - - // After authentication, verify that the to-be-modified user actually exists: - if must_exist && authid.user() != userid { - let (config, _digest) = pbs_config::user::config()?; - - if config.lookup::("user", userid.as_str()).is_err() { - http_bail!(UNAUTHORIZED, "user '{}' does not exists.", userid); - } - } - - Ok(()) -} - #[api( protected: true, input: { @@ -128,7 +88,7 @@ pub async fn delete_tfa( password: Option, rpcenv: &mut dyn RpcEnvironment, ) -> Result<(), Error> { - tfa_update_auth(rpcenv, &userid, password, false).await?; + super::user_update_auth(rpcenv, &userid, password, false).await?; let _lock = crate::config::tfa::write_lock()?; @@ -225,7 +185,7 @@ async fn add_tfa_entry( r#type: methods::TfaType, rpcenv: &mut dyn RpcEnvironment, ) -> Result { - tfa_update_auth(rpcenv, &userid, password, true).await?; + super::user_update_auth(rpcenv, &userid, password, true).await?; let _lock = crate::config::tfa::write_lock()?; @@ -285,7 +245,7 @@ async fn update_tfa_entry( password: Option, rpcenv: &mut dyn RpcEnvironment, ) -> Result<(), Error> { - tfa_update_auth(rpcenv, &userid, password, true).await?; + super::user_update_auth(rpcenv, &userid, password, true).await?; let _lock = crate::config::tfa::write_lock()?; diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs index 118838ce..ad5752a0 100644 --- a/src/api2/access/user.rs +++ b/src/api2/access/user.rs @@ -253,7 +253,7 @@ pub enum DeletableProperty { )] /// Update user configuration. #[allow(clippy::too_many_arguments)] -pub fn update_user( +pub async fn update_user( userid: Userid, update: UserUpdater, password: Option, @@ -261,6 +261,10 @@ pub fn update_user( digest: Option, rpcenv: &mut dyn RpcEnvironment, ) -> Result<(), Error> { + if password.is_some() { + super::user_update_auth(rpcenv, &userid, password.as_deref(), false).await?; + } + let _lock = pbs_config::user::lock_config()?; let (mut config, expected_digest) = pbs_config::user::config()?; diff --git a/www/config/UserView.js b/www/config/UserView.js index 0ac6f252..19dce06f 100644 --- a/www/config/UserView.js +++ b/www/config/UserView.js @@ -60,6 +60,7 @@ Ext.define('PBS.config.UserView', { Ext.create('Proxmox.window.PasswordEdit', { userid: selection[0].data.userid, + confirmCurrentPassword: Proxmox.UserName !== 'root@pam', }).show(); },