Item14903: change password accepts "1" as an old password

Hotfix:

diff --git a/core/lib/Foswiki/Users/HtPasswdUser.pm b/core/lib/Foswiki/Users/HtPasswdUser.pm
index 916e8cc82..0a378fbee 100644
--- a/core/lib/Foswiki/Users/HtPasswdUser.pm
+++ b/core/lib/Foswiki/Users/HtPasswdUser.pm
@@ -741,9 +741,7 @@ sub setPassword {
     ASSERT($login) if DEBUG;
 
     if ( defined($oldUserPassword) ) {
-        unless ( $oldUserPassword eq '1' ) {
-            return 0 unless $this->checkPassword( $login, $oldUserPassword );
-        }
+        return 0 unless $this->checkPassword( $login, $oldUserPassword );
     }

Can anybody verify? I see that on some codepaths the new PasswordManagerPlugin on trunk/master calls setPassword() with an oldPassword set to "1" deliberately to circumvent the password check. Are there any other codepaths requiring this bypass? Is this really required? Presumably not when called via the ChangePassword userinterface.

Please comment.

-- MichaelDaum - 04 May 2020

Hotfixed on foswiki.org and blog.foswiki.org

-- MichaelDaum - 04 May 2020

I've checked the code. This bug goes back til TWiki times. Only other affected code is the new PasswordManagementPlugin: it should use undef instead of 1 to force password renewal. The rest is docu in Foswiki::Users and unit tests.

-- MichaelDaum - 04 May 2020

I took an initial look at the functionality. The 1 appears to be a quick way for the admin to reset the password of a user. Looking about the flows:

User (or attacker) needs to reset password doesn't know it -> foswiki sends the registered user an email link User logged in wants to Change own password -> 1 as old password not accepted must be an admin User logged in wants to Change someone else's password -> 1 as old password not accepted must be an admin Admin logged in wants to Change someone else's password -> 1 as old password accepted - password changed

I thought I saw that you need to be an admin but I have not been able to validate that.

I would be interested though on the impact on non htpasswd systems. I imagine LDAP servers would reject a password change with 1 as an old password and SAML does not use passwords from foswiki.

Really, the correct approach would be to force the admin to type in their current password in order to reset someones account and validate that it matches before changing the user's password. Or to have a specific check for IsAdmin there I suppose. I like the admin password though as there is less chance of a CSRF.

-- TimothyLegge - 13 May 2020

Commenting is disabled while not logged in