From 744ec314269ed9707a9862173da9cfde1855b2d0 Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Thu, 8 Feb 2024 17:31:04 +0100 Subject: [PATCH 01/16] api: user: limit email to 254 characters and user comment to 2048 For email the reasoning is: > In addition to restrictions on syntax, there is a length limit on > email addresses. That limit is a maximum of 64 characters (octets) > in the "local part" (before the "@") and a maximum of 255 > characters (octets) in the domain part (after the "@") for a total > length of 320 characters. However, there is a restriction in RFC > 2821 on the length of an address in MAIL and RCPT commands of 254 > characters. Since addresses that do not fit in those fields are > not normally useful, the upper limit on address lengths should > normally be considered to be 254. -- https://www.rfc-editor.org/errata_search.php?rfc=3696&eid=1690 And for user-comments, we normally show those as single line and using 2048 bytes as maximum, while also a rather arbitrary number it allows for about 2.5 times more users on a system (full name + comment can be up to 4 KiB vs 10 KiB), and we can re-raise this relatively easily again if there are somewhat reasonable complaints. Signed-off-by: Thomas Lamprecht --- src/PVE/API2/User.pm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/PVE/API2/User.pm b/src/PVE/API2/User.pm index 8e0f440..489d34f 100644 --- a/src/PVE/API2/User.pm +++ b/src/PVE/API2/User.pm @@ -35,12 +35,12 @@ register_standard_option('user-email', { type => 'string', optional => 1, format => 'email-opt', - maxLength => 4096, + maxLength => 254, # 256 including punctuation and separator is the max path as per RFC 5321 }); register_standard_option('user-comment', { type => 'string', optional => 1, - maxLength => 8192, + maxLength => 2048, }); register_standard_option('user-keys', { description => "Keys for two factor auth (yubico).", -- 2.39.2 From b543394c937049265bac20c2ddb1fde0662f6a73 Mon Sep 17 00:00:00 2001 From: Gabriel Goller Date: Tue, 6 Feb 2024 11:11:01 +0100 Subject: [PATCH 02/16] oidc: enforce generic URI regex for the ACR value MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Restrict the acr-value regex a little bit so as to align the behavior with PBS. The openid documentation says that the acr-value *should* be an URI [0]. Added a regex that loosely disallows some of the reserved URI characters specified in the RFC [1]. Values like: * "urn:mace:incommon:iap:silver" * "urn:comsolve.nl:idp:contract:rba:location" SHOULD work, but values like: * "urn:#ace:incommon:iap:silver" * "urn:"omsolve.nl:idp:contract:rba:location" should NOT work. This is related to the fix [2] for bug #5190 in PBS, but different as there we had to make the verifier more flexible, whereas here we make it stricter – mostly to have both projects aligned to avoid confusion. [0]: https://openid.net/specs/openid-connect-core-1_0.html [1]: https://www.rfc-editor.org/rfc/rfc2396.txt [2]: https://git.proxmox.com/?p=proxmox-backup.git;a=commit;h=e0222ce83c28397d493c70825e873943c1223c67 Signed-off-by: Gabriel Goller --- src/PVE/Auth/OpenId.pm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm index 56904e6..c8e4db9 100755 --- a/src/PVE/Auth/OpenId.pm +++ b/src/PVE/Auth/OpenId.pm @@ -59,7 +59,8 @@ sub properties { 'acr-values' => { description => "Specifies the Authentication Context Class Reference values that the" ."Authorization Server is being requested to use for the Auth Request.", - type => 'string', # format => 'some-safe-id-list', # FIXME: TODO + type => 'string', + pattern => '^[^\x00-\x1F\x7F <>#"]*$', # Prohibit characters not allowed in URI RFC 2396. optional => 1, }, }; -- 2.39.2 From 6324cbb39c8388371861e4d02580334bd232328e Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Thu, 8 Feb 2024 17:58:05 +0100 Subject: [PATCH 03/16] bump version to 8.1.0 Signed-off-by: Thomas Lamprecht --- debian/changelog | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/debian/changelog b/debian/changelog index eaff591..8c84341 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,27 @@ +libpve-access-control (8.1.0) bookworm; urgency=medium + + * api: user: limit the legacy user-keys option to the depreacated values + that could be set in the first limited TFA system, like e.g., 'x!yubico' + or base32 encoded secrets. + + * oidc: enforce generic URI regex for the ACR value to align with OIDC + specifications and with Proxmox Backup Server, which was recently changed + to actually be less strict. + + * LDAP sync: improve validation of synced attributes, closely limit the + mapped attributes names and their values to avoid glitches through odd + LDIF entries. + + * api: user: limit maximum length for first & last name to 1024 characters, + email to 254 characters (the maximum actually useable in practice) and + comment properties to 2048 characters. This avoid that a few single users + bloat the user.cfg to much by mistake, reducing the total amount of users + and ACLs that can be set up. Note that only users with User.Modify and + realm syncs (setup by admins) can change these in the first place, so this + is mostly to avoid mishaps and just to be sure. + + -- Proxmox Support Team Thu, 08 Feb 2024 17:50:59 +0100 + libpve-access-control (8.0.7) bookworm; urgency=medium * fix #1148: allow up to three levels of pool nesting -- 2.39.2 From 8c3bf4124c216abb00b2d99b153c0039246d50aa Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Thu, 8 Feb 2024 19:03:10 +0100 Subject: [PATCH 04/16] LDAP sync: fix-up assembling valid attribute set Signed-off-by: Thomas Lamprecht --- src/PVE/Auth/LDAP.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm index 0f9403d..bf7e968 100755 --- a/src/PVE/Auth/LDAP.pm +++ b/src/PVE/Auth/LDAP.pm @@ -289,7 +289,7 @@ sub get_users { # NOTE: also ensure verify_sync_attribute_value can handle any new/changed attribute name }; # build on the fly as this is small and only called once per realm in a ldap-sync anyway - my $valid_sync_attributes = map { $_ => 1 } values $ldap_attribute_map->%*; + my $valid_sync_attributes = { map { $_ => 1 } values $ldap_attribute_map->%* }; foreach my $attr (PVE::Tools::split_list($config->{sync_attributes})) { my ($ours, $ldap) = ($attr =~ m/^\s*(\w+)=(.*)\s*$/); -- 2.39.2 From 588927f14a634bd92ea251dcd4178a29c991ac8b Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Thu, 8 Feb 2024 19:03:31 +0100 Subject: [PATCH 05/16] bump version to 8.1.1 Signed-off-by: Thomas Lamprecht --- debian/changelog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/debian/changelog b/debian/changelog index 8c84341..53bd71c 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +libpve-access-control (8.1.1) bookworm; urgency=medium + + * LDAP sync: fix-up assembling valid attribute set + + -- Proxmox Support Team Thu, 08 Feb 2024 19:03:26 +0100 + libpve-access-control (8.1.0) bookworm; urgency=medium * api: user: limit the legacy user-keys option to the depreacated values -- 2.39.2 From 0aa00d13a44664919ad18c8c379154e49bd845a1 Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Mon, 19 Feb 2024 14:40:10 +0100 Subject: [PATCH 06/16] special roles: code-style improvements for generation of special roles no semantic change intended Signed-off-by: Thomas Lamprecht --- src/PVE/AccessControl.pm | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm index 461a64e..25fc0d9 100644 --- a/src/PVE/AccessControl.pm +++ b/src/PVE/AccessControl.pm @@ -1150,24 +1150,25 @@ my $special_roles = { sub create_roles { - foreach my $cat (keys %$privgroups) { + for my $cat (keys %$privgroups) { my $cd = $privgroups->{$cat}; - foreach my $p (@{$cd->{root}}, @{$cd->{admin}}, - @{$cd->{user}}, @{$cd->{audit}}) { - $valid_privs->{$p} = 1; + # create map to easily check if a privilege is valid + for my $priv (@{$cd->{root}}, @{$cd->{admin}}, @{$cd->{user}}, @{$cd->{audit}}) { + $valid_privs->{$priv} = 1; } - foreach my $p (@{$cd->{admin}}, @{$cd->{user}}, @{$cd->{audit}}) { - - $special_roles->{"PVE${cat}Admin"}->{$p} = 1; - $special_roles->{"PVEAdmin"}->{$p} = 1; + # create grouped admin roles and PVEAdmin + for my $priv (@{$cd->{admin}}, @{$cd->{user}}, @{$cd->{audit}}) { + $special_roles->{"PVE${cat}Admin"}->{$priv} = 1; + $special_roles->{"PVEAdmin"}->{$priv} = 1; } + # create grouped user and audit roles if (scalar(@{$cd->{user}})) { - foreach my $p (@{$cd->{user}}, @{$cd->{audit}}) { - $special_roles->{"PVE${cat}User"}->{$p} = 1; + for my $priv (@{$cd->{user}}, @{$cd->{audit}}) { + $special_roles->{"PVE${cat}User"}->{$priv} = 1; } } - foreach my $p (@{$cd->{audit}}) { - $special_roles->{"PVEAuditor"}->{$p} = 1; + for my $priv (@{$cd->{audit}}) { + $special_roles->{"PVEAuditor"}->{$priv} = 1; } } -- 2.39.2 From 742a7b6cbd2f1052ca5f5f1fc2a6a62a7d95310e Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Mon, 19 Feb 2024 15:12:22 +0100 Subject: [PATCH 07/16] tests: split long expected-permission list over multiple lines for a better overview and to allow slightly easier tracking of any change, like adding a new privilege. Signed-off-by: Thomas Lamprecht --- src/test/perm-test1.pl | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/test/perm-test1.pl b/src/test/perm-test1.pl index a9dc502..27aadeb 100755 --- a/src/test/perm-test1.pl +++ b/src/test/perm-test1.pl @@ -59,9 +59,32 @@ check_permission('alex@pve', '/vms', ''); check_permission('alex@pve', '/vms/100', 'VM.Audit,VM.PowerMgmt'); # PVEVMAdmin -> no Permissions.Modify! -check_permission('alex@pve', '/vms/300', 'VM.Allocate,VM.Audit,VM.Backup,VM.Clone,VM.Config.CDROM,VM.Config.CPU,VM.Config.Cloudinit,VM.Config.Disk,VM.Config.HWType,VM.Config.Memory,VM.Config.Network,VM.Config.Options,VM.Console,VM.Migrate,VM.Monitor,VM.PowerMgmt,VM.Snapshot,VM.Snapshot.Rollback'); +check_permission( + 'alex@pve', + '/vms/300', + '' # sorted, comma-separated expected privilege string + . 'VM.Allocate,VM.Audit,VM.Backup,VM.Clone,VM.Config.CDROM,VM.Config.CPU,VM.Config.Cloudinit,' + . 'VM.Config.Disk,VM.Config.HWType,VM.Config.Memory,VM.Config.Network,VM.Config.Options,' + . 'VM.Console,VM.Migrate,VM.Monitor,VM.PowerMgmt,VM.Snapshot,VM.Snapshot.Rollback' +); # Administrator -> Permissions.Modify! -check_permission('alex@pve', '/vms/400', 'Datastore.Allocate,Datastore.AllocateSpace,Datastore.AllocateTemplate,Datastore.Audit,Group.Allocate,Mapping.Audit,Mapping.Modify,Mapping.Use,Permissions.Modify,Pool.Allocate,Pool.Audit,Realm.Allocate,Realm.AllocateUser,SDN.Allocate,SDN.Audit,SDN.Use,Sys.Audit,Sys.Console,Sys.Incoming,Sys.Modify,Sys.PowerMgmt,Sys.Syslog,User.Modify,VM.Allocate,VM.Audit,VM.Backup,VM.Clone,VM.Config.CDROM,VM.Config.CPU,VM.Config.Cloudinit,VM.Config.Disk,VM.Config.HWType,VM.Config.Memory,VM.Config.Network,VM.Config.Options,VM.Console,VM.Migrate,VM.Monitor,VM.PowerMgmt,VM.Snapshot,VM.Snapshot.Rollback'); +check_permission( + 'alex@pve', + '/vms/400', + '' # sorted, comma-separated expected privilege string, loosely grouped by prefix + . 'Datastore.Allocate,Datastore.AllocateSpace,Datastore.AllocateTemplate,Datastore.Audit,' + . 'Group.Allocate,' + . 'Mapping.Audit,Mapping.Modify,Mapping.Use,' + . 'Permissions.Modify,' + . 'Pool.Allocate,Pool.Audit,' + . 'Realm.Allocate,Realm.AllocateUser,' + . 'SDN.Allocate,SDN.Audit,SDN.Use,' + . 'Sys.Audit,Sys.Console,Sys.Incoming,Sys.Modify,Sys.PowerMgmt,Sys.Syslog,' + . 'User.Modify,' + . 'VM.Allocate,VM.Audit,VM.Backup,VM.Clone,VM.Config.CDROM,VM.Config.CPU,VM.Config.Cloudinit,' + . 'VM.Config.Disk,VM.Config.HWType,VM.Config.Memory,VM.Config.Network,VM.Config.Options,' + . 'VM.Console,VM.Migrate,VM.Monitor,VM.PowerMgmt,VM.Snapshot,VM.Snapshot.Rollback', +); check_roles('max@pve', '/vms/200', 'storage_manager'); check_roles('joe@pve', '/vms/200', 'vm_admin'); -- 2.39.2 From 36c18144def351b0f1b60780dc841ea8970ca1c4 Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Mon, 19 Feb 2024 16:25:51 +0100 Subject: [PATCH 08/16] add Sys.AccessNetwork privilege MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit We have some API endpoints that can access the network from the POV of a Proxmox VE node, like e.g., the one for downloading a template/ISO image directly to a PVE storage from an HTTP URL, and the matching query-url-metadata that makes this functionality much more convenient to use in the UI. But the downside of such calls is naturally that they basically allow to scan the whole network via HTTP URLs, and potentially even download some image that the user should not have access to and adding to a VM that the user controls. Due to that we limited the exposure of those API endpoints to Sys.Modify on / (in addition to e.g. basic storage privs) for the initial addition of the feature, as we were not sure about user adoption and if a separate privilege could be justified. Since we got a handful requests like #5254 this justification is now met, so add a 'Sys.AccessNetwork' privilege. That name should make it clear that having that privilege will allow access to the network and the sys(tem) prefix should underline that it's about the host systems network. Add it such, that it will only be available for the most powerful of our built-in special roles, namely the Administration one, besides naturally the all-powerful root@pam special user. Admins can then e.g. create new roles that include Sys.AccessNetwork and Datastore.AllocateTemplate which can then be used for allowing automation to download images while adhering to the Least Privilege Principle. Buglink: https://bugzilla.proxmox.com/show_bug.cgi?id=5254 Signed-off-by: Thomas Lamprecht Reviewed-by: Fabian Grünbichler --- src/PVE/AccessControl.pm | 1 + src/test/perm-test1.pl | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm index 25fc0d9..faea70d 100644 --- a/src/PVE/AccessControl.pm +++ b/src/PVE/AccessControl.pm @@ -1065,6 +1065,7 @@ my $privgroups = { 'Sys.PowerMgmt', 'Sys.Modify', # edit/change node settings 'Sys.Incoming', # incoming storage/guest migrations + 'Sys.AccessNetwork', # for, e.g., downloading ISOs from any URL ], admin => [ 'Sys.Console', diff --git a/src/test/perm-test1.pl b/src/test/perm-test1.pl index 27aadeb..df9fe90 100755 --- a/src/test/perm-test1.pl +++ b/src/test/perm-test1.pl @@ -79,7 +79,7 @@ check_permission( . 'Pool.Allocate,Pool.Audit,' . 'Realm.Allocate,Realm.AllocateUser,' . 'SDN.Allocate,SDN.Audit,SDN.Use,' - . 'Sys.Audit,Sys.Console,Sys.Incoming,Sys.Modify,Sys.PowerMgmt,Sys.Syslog,' + . 'Sys.AccessNetwork,Sys.Audit,Sys.Console,Sys.Incoming,Sys.Modify,Sys.PowerMgmt,Sys.Syslog,' . 'User.Modify,' . 'VM.Allocate,VM.Audit,VM.Backup,VM.Clone,VM.Config.CDROM,VM.Config.CPU,VM.Config.Cloudinit,' . 'VM.Config.Disk,VM.Config.HWType,VM.Config.Memory,VM.Config.Network,VM.Config.Options,' -- 2.39.2 From 85f61297734b74591a51f6ca36f303885c03fd5e Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Wed, 28 Feb 2024 15:42:20 +0100 Subject: [PATCH 09/16] bump version to 8.1.2 Signed-off-by: Thomas Lamprecht --- debian/changelog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/debian/changelog b/debian/changelog index 53bd71c..6ef88e4 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +libpve-access-control (8.1.2) bookworm; urgency=medium + + * add Sys.AccessNetwork privilege + + -- Proxmox Support Team Wed, 28 Feb 2024 15:42:12 +0100 + libpve-access-control (8.1.1) bookworm; urgency=medium * LDAP sync: fix-up assembling valid attribute set -- 2.39.2 From 184a499e8a7963eefa7c5d99fb97660c9712b17e Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Mon, 18 Mar 2024 09:20:23 +0100 Subject: [PATCH 10/16] tfa: prototype fixup Signed-off-by: Wolfgang Bumiller --- src/PVE/API2/TFA.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm index 13ffc59..9fc6bcb 100644 --- a/src/PVE/API2/TFA.pm +++ b/src/PVE/API2/TFA.pm @@ -409,7 +409,7 @@ __PACKAGE__->register_method ({ }); }}); -sub validate_yubico_otp : prototype($$) { +sub validate_yubico_otp : prototype($$$) { my ($userid, $realm, $value) = @_; my $domain_cfg = cfs_read_file('domains.cfg'); -- 2.39.2 From 060941d467693a97f6b005c387957d2c3196b046 Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Fri, 15 Mar 2024 13:40:48 +0100 Subject: [PATCH 11/16] move/rename root_permission_check to RPCEnvironment now called "reauth_user_for_user_modification" since we re-authenticate the current user while they are trying to modify their (or others') password/tfa settings Signed-off-by: Wolfgang Bumiller --- src/PVE/API2/TFA.pm | 51 ++++++++++++--------------------------- src/PVE/AccessControl.pm | 1 + src/PVE/RPCEnvironment.pm | 32 +++++++++++++++++++++++- 3 files changed, 47 insertions(+), 37 deletions(-) diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm index 9fc6bcb..e178e97 100644 --- a/src/PVE/API2/TFA.pm +++ b/src/PVE/API2/TFA.pm @@ -95,36 +95,6 @@ my $TFA_UPDATE_INFO_SCHEMA = { }, }; -# Only root may modify root, regular users need to specify their password. -# -# Returns the userid returned from `verify_username`. -# Or ($userid, $realm) in list context. -my sub root_permission_check : prototype($$$$) { - my ($rpcenv, $authuser, $userid, $password) = @_; - - ($userid, undef, my $realm) = PVE::AccessControl::verify_username($userid); - $rpcenv->check_user_exist($userid); - - raise_perm_exc() if $userid eq 'root@pam' && $authuser ne 'root@pam'; - - # Regular users need to confirm their password to change TFA settings. - if ($authuser ne 'root@pam') { - raise_param_exc({ 'password' => 'password is required to modify TFA data' }) - if !defined($password); - - ($authuser, my $auth_username, my $auth_realm) = - PVE::AccessControl::verify_username($authuser); - - my $domain_cfg = cfs_read_file('domains.cfg'); - my $cfg = $domain_cfg->{ids}->{$auth_realm}; - die "auth domain '$auth_realm' does not exist\n" if !$cfg; - my $plugin = PVE::Auth::Plugin->lookup($cfg->{type}); - $plugin->authenticate_user($cfg, $auth_realm, $auth_username, $password); - } - - return wantarray ? ($userid, $realm) : $userid; -} - # Set TFA to enabled if $tfa_cfg is passed, or to disabled if $tfa_cfg is undef, # When enabling we also merge the old user.cfg keys into the $tfa_cfg. my sub set_user_tfa_enabled : prototype($$$) { @@ -244,8 +214,11 @@ __PACKAGE__->register_method ({ my $rpcenv = PVE::RPCEnvironment::get(); my $authuser = $rpcenv->get_user(); - my $userid = - root_permission_check($rpcenv, $authuser, $param->{userid}, $param->{password}); + my $userid = $rpcenv->reauth_user_for_user_modification( + $authuser, + $param->{userid}, + $param->{password}, + ); my $has_entries_left = PVE::AccessControl::lock_tfa_config(sub { my $tfa_cfg = cfs_read_file('priv/tfa.cfg'); @@ -378,8 +351,11 @@ __PACKAGE__->register_method ({ my $rpcenv = PVE::RPCEnvironment::get(); my $authuser = $rpcenv->get_user(); - my ($userid, $realm) = - root_permission_check($rpcenv, $authuser, $param->{userid}, $param->{password}); + my ($userid, $realm) = $rpcenv->reauth_user_for_user_modification( + $authuser, + $param->{userid}, + $param->{password}, + ); my $type = delete $param->{type}; my $value = delete $param->{value}; @@ -471,8 +447,11 @@ __PACKAGE__->register_method ({ my $rpcenv = PVE::RPCEnvironment::get(); my $authuser = $rpcenv->get_user(); - my $userid = - root_permission_check($rpcenv, $authuser, $param->{userid}, $param->{password}); + my $userid = $rpcenv->reauth_user_for_user_modification( + $authuser, + $param->{userid}, + $param->{password}, + ); PVE::AccessControl::lock_tfa_config(sub { my $tfa_cfg = cfs_read_file('priv/tfa.cfg'); diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm index faea70d..21f93ff 100644 --- a/src/PVE/AccessControl.pm +++ b/src/PVE/AccessControl.pm @@ -16,6 +16,7 @@ use JSON; use Scalar::Util 'weaken'; use URI::Escape; +use PVE::Exception qw(raise_perm_exc raise_param_exc); use PVE::OTP; use PVE::Ticket; use PVE::Tools qw(run_command lock_file file_get_contents split_list safe_print); diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm index 646a9b9..db33cbb 100644 --- a/src/PVE/RPCEnvironment.pm +++ b/src/PVE/RPCEnvironment.pm @@ -5,7 +5,7 @@ use warnings; use PVE::AccessControl; use PVE::Cluster; -use PVE::Exception qw(raise raise_perm_exc); +use PVE::Exception qw(raise raise_param_exc raise_perm_exc); use PVE::INotify; use PVE::ProcFSTools; use PVE::RESTEnvironment; @@ -637,4 +637,34 @@ sub is_worker { return PVE::RESTEnvironment->is_worker(); } +# Only root may modify root, regular users need to specify their password. +# +# Returns the userid returned from `verify_username`. +# Or ($userid, $realm) in list context. +sub reauth_user_for_user_modification : prototype($$$$) { + my ($rpcenv, $authuser, $userid, $password) = @_; + + ($userid, undef, my $realm) = PVE::AccessControl::verify_username($userid); + $rpcenv->check_user_exist($userid); + + raise_perm_exc() if $userid eq 'root@pam' && $authuser ne 'root@pam'; + + # Regular users need to confirm their password to change TFA settings. + if ($authuser ne 'root@pam') { + raise_param_exc({ 'password' => 'password is required to modify TFA data' }) + if !defined($password); + + ($authuser, my $auth_username, my $auth_realm) = + PVE::AccessControl::verify_username($authuser); + + my $domain_cfg = PVE::Cluster::cfs_read_file('domains.cfg'); + my $cfg = $domain_cfg->{ids}->{$auth_realm}; + die "auth domain '$auth_realm' does not exist\n" if !$cfg; + my $plugin = PVE::Auth::Plugin->lookup($cfg->{type}); + $plugin->authenticate_user($cfg, $auth_realm, $auth_username, $password); + } + + return wantarray ? ($userid, $realm) : $userid; +} + 1; -- 2.39.2 From 90faf488db9496aeb9f6e3ddb6677af1e6b41659 Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Fri, 15 Mar 2024 13:44:27 +0100 Subject: [PATCH 12/16] return ruid in reauth_user_for_user_modification, add param name since the upcoming use case in change_password uses the returned $ruid and the parameter is called 'confirmation-password' there also generalize the error so it does not mention TFA Signed-off-by: Wolfgang Bumiller --- src/PVE/API2/TFA.pm | 2 +- src/PVE/RPCEnvironment.pm | 17 ++++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm index e178e97..50ab925 100644 --- a/src/PVE/API2/TFA.pm +++ b/src/PVE/API2/TFA.pm @@ -351,7 +351,7 @@ __PACKAGE__->register_method ({ my $rpcenv = PVE::RPCEnvironment::get(); my $authuser = $rpcenv->get_user(); - my ($userid, $realm) = $rpcenv->reauth_user_for_user_modification( + my ($userid, undef, $realm) = $rpcenv->reauth_user_for_user_modification( $authuser, $param->{userid}, $param->{password}, diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm index db33cbb..e668353 100644 --- a/src/PVE/RPCEnvironment.pm +++ b/src/PVE/RPCEnvironment.pm @@ -637,21 +637,24 @@ sub is_worker { return PVE::RESTEnvironment->is_worker(); } +# Permission helper for TFA and password API endpoints modifying users. # Only root may modify root, regular users need to specify their password. # -# Returns the userid returned from `verify_username`. -# Or ($userid, $realm) in list context. -sub reauth_user_for_user_modification : prototype($$$$) { - my ($rpcenv, $authuser, $userid, $password) = @_; +# Returns the same as `verify_username` in list context (userid, ruid, realm), +# or just the userid in scalar context. +sub reauth_user_for_user_modification : prototype($$$$;$) { + my ($rpcenv, $authuser, $userid, $password, $param_name) = @_; - ($userid, undef, my $realm) = PVE::AccessControl::verify_username($userid); + $param_name //= 'password'; + + ($userid, my $ruid, my $realm) = PVE::AccessControl::verify_username($userid); $rpcenv->check_user_exist($userid); raise_perm_exc() if $userid eq 'root@pam' && $authuser ne 'root@pam'; # Regular users need to confirm their password to change TFA settings. if ($authuser ne 'root@pam') { - raise_param_exc({ 'password' => 'password is required to modify TFA data' }) + raise_param_exc({ $param_name => 'password is required to modify user' }) if !defined($password); ($authuser, my $auth_username, my $auth_realm) = @@ -664,7 +667,7 @@ sub reauth_user_for_user_modification : prototype($$$$) { $plugin->authenticate_user($cfg, $auth_realm, $auth_username, $password); } - return wantarray ? ($userid, $realm) : $userid; + return wantarray ? ($userid, $ruid, $realm) : $userid; } 1; -- 2.39.2 From 5bcf553e3a193a537d92498f4fee3c23e22d1741 Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Fri, 15 Mar 2024 13:41:29 +0100 Subject: [PATCH 13/16] user: password change: require confirmation-password parameter 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. Reported-by: Wouter Arts Signed-off-by: Wolfgang Bumiller [ TL: extend commit message ] Signed-off-by: Thomas Lamprecht --- src/PVE/API2/AccessControl.pm | 10 +++++++--- src/PVE/API2/TFA.pm | 4 ++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm index 74b3910..c55a7b3 100644 --- a/src/PVE/API2/AccessControl.pm +++ b/src/PVE/API2/AccessControl.pm @@ -344,6 +344,7 @@ __PACKAGE__->register_method ({ minLength => 5, maxLength => 64, }, + 'confirmation-password' => $PVE::API2::TFA::OPTIONAL_PASSWORD_SCHEMA, } }, returns => { type => "null" }, @@ -353,9 +354,12 @@ __PACKAGE__->register_method ({ my $rpcenv = PVE::RPCEnvironment::get(); my $authuser = $rpcenv->get_user(); - my ($userid, $ruid, $realm) = PVE::AccessControl::verify_username($param->{userid}); - - $rpcenv->check_user_exist($userid); + my ($userid, $ruid, $realm) = $rpcenv->reauth_user_for_user_modification( + $authuser, + $param->{userid}, + $param->{'confirmation-password'}, + 'confirmation-password', + ); if ($authuser eq 'root@pam') { # OK - root can change anything diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm index 50ab925..62ddd95 100644 --- a/src/PVE/API2/TFA.pm +++ b/src/PVE/API2/TFA.pm @@ -18,8 +18,8 @@ use PVE::RESTHandler; use base qw(PVE::RESTHandler); -my $OPTIONAL_PASSWORD_SCHEMA = { - description => "The current password.", +our $OPTIONAL_PASSWORD_SCHEMA = { + description => "The current password of the user performing the change.", type => 'string', optional => 1, # Only required if not root@pam minLength => 5, -- 2.39.2 From bb34ca534e44b3cf91b8ae34a204aba499dcfe90 Mon Sep 17 00:00:00 2001 From: Christoph Heiss Date: Tue, 19 Mar 2024 14:48:43 +0100 Subject: [PATCH 14/16] jobs: realm sync: fix scheduled LDAP syncs not applying attributes correctly This was reported by a user in the forum [0]. The cause was that the user-* standard options were not registered when the sync was called from the scheduler, resulting in the following error: pvescheduler[2849]: skipping attribute mapping 'cn'->'comment' for user 'test@samba0' - no such standard option 'user-comment' Fix this by simply importing the PVE::API2::User module, thus ensuring the options get registered. [0] https://forum.proxmox.com/threads/ldap-integration-comment-email-first-name-lastname.143490/ Fixes: cb93636 ("LDAP sync: improve validation of synced attributes") Signed-off-by: Christoph Heiss Reviewed-by: Fiona Ebner Tested-by: Fiona Ebner --- src/PVE/Jobs/RealmSync.pm | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/PVE/Jobs/RealmSync.pm b/src/PVE/Jobs/RealmSync.pm index 91235d5..4c77e55 100644 --- a/src/PVE/Jobs/RealmSync.pm +++ b/src/PVE/Jobs/RealmSync.pm @@ -13,6 +13,9 @@ use PVE::Tools (); use PVE::API2::Domains (); +# load user-* standard options +use PVE::API2::User (); + use base qw(PVE::Job::Registry); sub type { -- 2.39.2 From 787e4c06e39faad5de94f02d6012504412cba6b9 Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Fri, 22 Mar 2024 14:14:39 +0100 Subject: [PATCH 15/16] bump version to 8.1.3 Signed-off-by: Thomas Lamprecht --- debian/changelog | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/debian/changelog b/debian/changelog index 6ef88e4..ad18bb7 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,22 @@ +libpve-access-control (8.1.3) bookworm; urgency=medium + + * user: password change: require confirmation-password parameter so that + anybody gaining local or physical access to a device where a user is + logged in on a Proxmox VE web-interface cannot give them more permanent + access or deny the actual user accessing their account by changing the + password. Note that such an attack scenario means that the attacker + already has high privileges and can already control the resource + completely through another attack. + Such initial attacks (like stealing an unlocked device) are almost always + are outside of the control of our projects. Still, hardening the API a bit + by requiring a confirmation of the original password is to cheap to + implement to not do so. + + * jobs: realm sync: fix scheduled LDAP syncs not applying all attributes, + like comments, correctly + + -- Proxmox Support Team Fri, 22 Mar 2024 14:14:36 +0100 + libpve-access-control (8.1.2) bookworm; urgency=medium * add Sys.AccessNetwork privilege -- 2.39.2 From cca4c0009e5f2826c4994bb6b1706e59141431d5 Mon Sep 17 00:00:00 2001 From: Daniel Krambrock via pve-devel Date: Thu, 11 Apr 2024 10:09:09 +0200 Subject: [PATCH 16/16] fix #5335: sort ACL entries in user.cfg Stable sorting in user.cfg config file allows tracking changes by checking into git or when using automation like ansible. Signed-off-by: Daniel Krambrock Tested-by: Folge Gleumes --- src/PVE/AccessControl.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm index 21f93ff..47f2d38 100644 --- a/src/PVE/AccessControl.pm +++ b/src/PVE/AccessControl.pm @@ -951,7 +951,7 @@ sub iterate_acl_tree { my $children = $node->{children}; - foreach my $child (keys %$children) { + foreach my $child (sort keys %$children) { iterate_acl_tree("$path/$child", $children->{$child}, $code); } } -- 2.39.2