Vetoing password could disclose information

[lekoala]

I recently worked on some upgrades for password auditing on a website and I discovered it's possible to veto password reset.

Doing so send you back to the form instead of the success screen. If the veto procedure is predictable, it could disclose information. Instead, it's probably much better if any forgotPassword hook returns false to set the member to null instead and proceed quietly to success screen, this will lead to always the same results regardless of what password is entered.

https://github.com/silverstripe/silverstripe-framework/blob/fe45655a2b7b49c01c69178baaddb4ab20cefa5e/src/Security/MemberAuthenticator/LostPasswordHandler.php#L173

[emteknetnz]

I've had a look through the code around this, though I'm not too sure what we're trying to achieve with this change?

proceed quietly to success screen

That seems like a pretty bad UX experience for regular users on sites that have enabled password reset vetoing. They'll won't get any feedback on the action they just took, or that password resetting is actually disabled for them

I'm also not entirely sure what information could be disclosed here? If the password reset is vetoed, then nothing is actually done with $member? Instead the code just proceeds to:

$lostPasswordLink = Security::singleton()->Link('lostpassword');
return $this->redirect($this->addBackURLParam($lostPasswordLink));

Please let me know if I've missed something

[lekoala]

@emteknetnz so here is what happens to me and maybe this will make things cleared

I had two ideas:

• prevent password reset for admins
• prevent hammering with passwords resets by implementing some kind of interval between resets

In both cases, if you veto the reset, it allows attackers to determine if an email exists or not in the database because instead of proceeding to the password reset screen, you would get a different result based on the entered value;

For example:

• I enter some.undefined.user@example.com => i go to the success screen. I enter : my.super.admin@example.com => i get vetoed
• Same thing for my other idea about hammering : i enter : my.user@example.com once => i go to the success screen. I enter again my.user@example.com => i go back to the lost password screen (due to hammering prevention), meaning my.user@example.com exists in the database

I understand it's probably bad to change how things are handled for previous project. But I would at least add some kind of warning to make developers aware that vetoing password reset can lead to that situation, because for my part, what I thought to be smart was in reality a bad idea^^

[emteknetnz]

OK that clarifies things. I do like the idea of not disclosing whether or not an account for an email exists.

Would it work if we updated the extension hook so that you can dynamically set where it redirects to on veto?

        // Allow vetoing forgot password requests
        $doRedirectToLostPassword = true; // default to existing behaviour
        $results = $this->extend('forgotPassword', $member, $doRedirectToLostPassword); // add new param to extension hook
        if ($results && is_array($results) && in_array(false, $results, true)) {
            if ($doRedirectToLostPassword) {
                return $this->redirectToLostPassword();
            }
            $member = null; // new code
        }

[lekoala]

@emteknetnz i'm not sure adding a new parameter is useful. you can already set the member to null in the extension and not return anything (which effectively leads to a veto, but without disclosing information), it's more about making the developers aware that vetoing can disclose information. They don't know anyway about available parameters unless they check the code or read the docs, so probably a warning in a comment and a mention in the docs are enough, with the strong suggestion to set member to null instead of returning false :-)

[JamesDPC]

Hi folks, I hit this today with a requirement to disallow password resets for Members without a permission(s). Eg. only members with a permission can request a password reset.

Having a different behaviour when requesting a password for an account can give clues to whether or not an email address/account exists in a system. This is something that gets flagged in pentests.

Any forgot password attempt whether it succeeds or fails should "redirectToSuccess", either by setting the member record to null as @lekoala suggests in 40364da or calling redirectToSuccess rather than redirectToLostPassword regardless of the results of a forgotPassword veto.

The latter would be preferable as then the core LostPasswordHandler could enforce it:

// LostPasswordHandler
        if ($results && is_array($results) && in_array(false, $results, true)) {
            return $this->redirectToSuccess();
        }

I've planned something similar to 40364da to avoid exposing account existence:

<?php
// LostPasswordHandler extension

    public function forgotPassword(Member &$member = null) : bool {
        if ($member && !Permission::checkMember($member, 'SOME_PERMISSION')) {
            // trigger redirectToSuccess
            $member = null;
        }
        // always return true
        return true;
    }

Results are:

• non-existent member -> redirectToSuccess()
• existing member allowed to change password -> redirectToSuccess()
• existing member not allowed to change password -> redirectToSuccess()

As for UX, we'd prefer people not have their accounts phished. If they don't get the password reset email they can escalate to a support desk or their site admin.