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 <security@wth-security.nl>
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
 [ TL: extend commit message ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
index 74b3910a3c7faed1dd4f9dd03c300d1b2612df93..c55a7b347be7a9466cdad328c2d6ce029644b8cd 100644 (file)
--- 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 50ab925ebaad365230accbae9f4efc6307384475..62ddd959378008ae3caf6ea92e04d43b489fc358 100644 (file)
--- 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,