Invalidating session in-between credentials and MFA challenge

[maxime-rainville]

When you log into the CMS, your existing session gets invalidated and you get a new session ID. This is a security feature to prevent session fixation attacks.

When using MFA, your session doesn't get invalidated in-between entering your credentials and entering the challenge code. It does get invalidated once you are logged-in proper.

There's a theoretical window of attack. The scenario where this could be helpful to an attacker looks something like this:

• The attacker has managed to compromised the victim's MFA code somehow, but NOT their password.
• The attacker has the ability to fix the victim's session.
• The victim enters their credentials.
• The attacker manages to answer the MFA challenge before the victim does (if the victim gets their first the session is invalidated and the attacker won't be allowed to complete the MFA challenge)

We reviewed this through our condifential security process and decided that because of the complexity required to pull this off, this should be treated as a security enhancement rather than a vulnerability.

This originally got flaged by a Qualys automated security scan on a third party project. Might be worth fixing this just to avoid more false positive.

[dhensby]

I think the question to consider is: when does a user become authenticated and when do their permissions get elevated?

I suppose as soon as we record in the session that a user is now associated with the session, that should probably count as an elevation of permissions. You've moved from anonymous to known and then you're being asked for an extra piece of information to continue the elevation but you have already presented a secret to allow us to associate your session with a user.

Perhaps the session should be regenerated at each step?

[ScopeyNZ]

I think this is relatively trivial to do, so I'll update the effort:

We already regenerate IDs in this module:

https://github.com/silverstripe/silverstripe-mfa/blob/4/src/Authenticator/LoginHandler.php#L126

This can just be moved outside of the else, as we want to regenerate on each successful verification. It should also be added to the finishVerification step, as this module was intended to actually allow the "M" of "MFA", giving the option for more than two factors.

I recall that we did consider this case, but thought it was incredibly unlikely that "The attacker has the ability to fix the victim's session" without "The attacker has not managed to compromise the victims password", as the session fixation requires some pretty intimate access to the users browser. Additionally, once they get a new session ID after entering their password, an attacker with "the ability to fix the victim's session" can just get it then.

In saying that, I can't recall any arguments against regenerating a session ID.

[robbieaverill]

If it's not going to hurt anything then I say let's do it - if nothing else it'll stop it coming up in future security audits.