Can bypass mandatory sign up for MFA prompt by using password reset form

[emteknetnz]

Discovered while implementing a fix for https://github.com/silverstripe/silverstripe-framework/issues/10940

If MFA is required, when a new user is created by an administrator, the first time they attempt to login they can click 'forgot password', receive a password reset email and when they change their password they aren't prompted to signup for MFA, instead they go straight into the CMS.

Note - if the user previously signed up for MFA, the reset password link does NOT bypass mfa.

[maxime-rainville]

If the user logs out and logs back in, does it prompt him to register for MFA?

[emteknetnz]

Yeah they're prompted then. It's only the password reset email flow that's incorrect

[baukezwaan]

This is a pretty big security flaw, when an attacker only need to compromise 1 factor to complete the attack.

What would be a desired solution for this? We could either check if the MFA module is present (option A). Or introduce a config-setting, to make it configurable wheter a user should be logged in automatically after a password reset.

I can make a PR, but want to check which direction this should be heading.

Original code:

        if ($member->canLogin()) {
            /** @var IdentityStore $identityStore */
            $identityStore = Injector::inst()->get(IdentityStore::class);
            $identityStore->logIn($member, false, $this->getRequest());
        }

Option A:

        if ($member->canLogin() && !class_exists('SilverStripe\\TOTP\\RegisterHandler')) {
            /** @var IdentityStore $identityStore */
            $identityStore = Injector::inst()->get(IdentityStore::class);
            $identityStore->logIn($member, false, $this->getRequest());
        }

Option B:

       $autoLogin = Security::config()->get('autologin_after_password_reset');

        if ($member->canLogin() && $autoLogin) {
            /** @var IdentityStore $identityStore */
            $identityStore = Injector::inst()->get(IdentityStore::class);
            $identityStore->logIn($member, false, $this->getRequest());
        }

[emteknetnz]

Can't do Option A because we shouldn't be checking for optional modules within framework

Option B also seems wrong since we'd almost certainly to set it to true to maintain existing functionality, so it wouldn't fix the issue. Also projects shoudn't have to use config to fix bugs

Standard pattern when getting framework to interact with optional is to add an extension hook into framework and implement it in the optional module. I'm not sure if that's the correct thing to do here. If I was going to implement this I'd first spend sometime with a debugger to check the code path that gets to the prompt for MFA signup flow when not using the reset password flow