Current 2FA solution can lock users out of their accounts

[LeoniePhiline]

• Version: v1.7.4
• Laravel Version: v8.22.1
• PHP Version: v7.4.13

Description:

The current two factor authentication solution sets 2fa to enabled without requiring a confirmation (via TOTP) that the authenticator app is actually set up.

Steps To Reproduce:

The current solution works like this:

1. POST /user/two-factor-authentication, the user's two_factor_secret is stored. (2fa is now enabled!)
2. GET /user/two-factor-qr-code, show QR code and ask user to scan the code with their app.
3. GET /user/two-factor-recovery-codes, show recovery codes and ask the user to save them.
4. User abandons the process before setting up their authenticator app or saving the recovery codes and is now locked out of their account.

They could abandon the process because they first need to choose one of the many TOTP generators in the app store, and get side tracked, or their session times out, or they click the back button, or close their tab, or their computer crashes, ... Definitely 2fa must not be enabled before it is confirmed by a generated OTP.

How To Fix It:

1. GET /user/two-factor-qr-code generates a QR code from a new two factor secret that is stored in the session.
2. GET /user/two-factor-recovery-codes, show the recovery codes to the user, ask them to save them.
3. The user is asked to set up their authenticator app with this QR code and enter a resulting TOTP code. This confirms that they have set up their authenticator (else they cannot generate a valid code), and can also (by written explanation) be used as confirmation that recovery codes were stored in a safe place.
4. POST /user/two-factor-authentication, receives a new parameter: code. The code is validated using the two factor secret stored in the session.
5. If the code is valid, the two factor secret from the session is written into the user table (= enabling 2fa, now for real). If not, then the response indicates that the user did not enter a valid code and must re-try.

[driesvints]

This issue basically ties into laravel/jetstream#74. While I agree it would be better if we required it, it's opinionated for sure. We could make this an opt-in feature for Fortify 1.x so we can implement it in Jetstream together with laravel/jetstream#74.

[LeoniePhiline]

Having it as an opt in feature for now to avoid the breaking change until the next major version sounds good to me.

I kindly disagree that this is opinionated, though. ;-)

——

By the way, is there a plan for a RequireSecondFactor confirmation middleware (with timeout) that works like the laravel core RequirePassword middleware?

A developer would want to force the “highest security level” confirmation for certain actions, while only asking for password confirmation in other cases.

A third possible middleware might use the “highest available”, which is OTP if 2fa is enabled, on the user record, else require password.

[driesvints]

By the way, is there a plan for a RequireSecondFactor confirmation middleware (with timeout) that works like the laravel core RequirePassword middleware?

Not at this time.

[LeoniePhiline]

Would you be interested in a PR or would that be trashed anyway?

[driesvints]

@LeoniePhiline Taylor handles the PR's so I can't really say sorry.

[markokeeffe]

@LeoniePhiline a lot of apps I use ask for confirmation of one-time password when the QR code is presented to the user. I'm handling this by adding a two_factor_confirmed boolean column to my users table. This will only be set to true when the user enters a valid one-time password in the 2FA setup process.

[LeoniePhiline]

@markokeeffe Definitely a valid approach to meet the same goal.

The discussion here, however, is a different one. I am claiming that this is a serious bug and must be part of Fortify. Dries on the other hand thinks of it as opinionated.

Most developers might just not notice the implementation is broken until the first users have been locked out.

The discussion is not about storing the key in the session until confirmed or using a boolean column on the User. Either approach is fine. It just needs to be fixed. ;)

[nicolus]

@markokeeffe : That sound like a decent solution, but the issue I have is that the part that check if a user had 2FA is in \Laravel\Fortify\Actions\RedirectIfTwoFactorAuthenticatable::handle() and directly checks if the two_factor_secret field is not null :

if (optional($user)->two_factor_secret &&
    in_array(TwoFactorAuthenticatable::class, class_uses_recursive($user))) {
    return $this->twoFactorChallengeResponse($request, $user);
}

So I have no idea how to cleanly modify this behavior since this action is not published but stays in the vendor directory.

edit : OK I got it working !

What I did was create a new Action in app/Actions/Fortify/RedirectIfTwoFactorAuthConfirmed.php that extends the one above but checks if the boolean field is true :

<?php

namespace App\Actions\Fortify;

use Laravel\Fortify\Actions\RedirectIfTwoFactorAuthenticatable;
use Laravel\Fortify\TwoFactorAuthenticatable;

class RedirectIfTwoFactorAuthConfirmed extends RedirectIfTwoFactorAuthenticatable {


    public function handle($request, $next)
    {
        $user = $this->validateCredentials($request);

        if (optional($user)->two_factor_confirmed &&
            in_array(TwoFactorAuthenticatable::class, class_uses_recursive($user))) {
            return $this->twoFactorChallengeResponse($request, $user);
        }

        return $next($request);
    }
}

And in app/Fortify/FortifyServiceProvider.php I used the Fortify::authenticateThrough() to change the Pipeline to use my action instead :

    Fortify::authenticateThrough(function(){
        return array_filter([
            config('fortify.limiters.login') ? null : EnsureLoginIsNotThrottled::class,
            Features::enabled(Features::twoFactorAuthentication()) ? RedirectIfTwoFactorAuthConfirmed::class : null,
            AttemptToAuthenticate::class,
            PrepareAuthenticatedSession::class,
        ]);
    });

And voilà !

btw @driesvints, @taylorotwell , the paragraph "Customizing the Authentication Pipeline" only exists in the Jetstream documentation but not in the Laravel documentation. I'm not sure if it is intentional, but it seems strange that I'd have to look at the Jetstream docs to learn about a Laravel Fortify feature.

[driesvints]

@nicolus appreciating PRs to the docs if you think something can be explained better 👍

[radudiaconu0]

@nicolus so where u make that field true if it's confirmed. I don't see that part in your code

[nicolus]

@radudiaconu0 : If you're interested I've done a full write up here :
https://dev.to/nicolus/laravel-fortify-implement-2fa-in-a-way-that-won-t-let-users-lock-themselves-out-2ejk

[radudiaconu0]

@nicolus Thanks

[James4645]

If I could put in my 2 cents. I would strongly disagree that its opinionated and instead say its best practice to confirm the user has correctly enabled and configured their authentication app before enabling 2FA. I'm glad there is a workaround but I think it should be core to the implementation.

[dmandrade]

I have a fork with such an implementation, I currently use it as a patch in fortify and jetstream.
https://gist.github.com/dmandrade/092818de02c7987ba2615448f0428382

I could do a PR with an opt-in feature

[dragonfly4]

@radudiaconu0 : If you're interested I've done a full write up here : https://dev.to/nicolus/laravel-fortify-implement-2fa-in-a-way-that-won-t-let-users-lock-themselves-out-2ejk

@nicolus Thank you for this. I faced the same problem as Ebe with binding the DisableTwoFactorAuthentication class. Running Laravel 8.

https://github.com/nicolus/laravel-2fa/blob/master/app/Providers/FortifyServiceProvider.php#L57-L59

Needs a backslash before Laravel to work.

    $this->app->bind(\Laravel\Fortify\Actions\DisableTwoFactorAuthentication::class, function(){
        return new ReallyDisableTwoFactorAuthentication();
    });

[patrickomeara]

@taylorotwell @driesvints Are either of you able to confirm that whatever solution the Laravel team implements will be backward compatible with the current 2fa codes that users have? I assume the answer is yes.

We have a looming regulatory cut off date for 2fa, and I'm trying to gauge whether to keep holding out on the official fix for this or not. I'm happy to implement a workaround in the meantime if users don't need to enable 2fa again once the official fix is out.

[driesvints]

There's no upcoming date for a fix sorry. Best that you roll your own for now.

[patrickomeara]

Thanks @driesvints

Am I correct in saying that whatever the confirmation solution looks like the stored OTP algorithm for each user will be unchanged?

[driesvints]

@patrickomeara I can't say anything about that unless I dig in and I don't have the time for that right now.

[securit]

My 2c, using a session key/cache approach might be a way to resolve the man-in-the-middle vulnerability of the token.

The current implementation of 2FA allows a generated token to be re-used any time within the time-step window . The RFC talks about the implications of the time-step window and says:

Second, the next different OTP must be generated in the next time-step window. A user must wait until the clock moves to the next time-step window from the last submission.

The Laravel implementation does not conform to this part of the standard, thus allowing a man-in-the-middle replay.

Simply put, store the code and the timestamp in the session. If the code is the same as the one in the session and the timestamp is within the time-step window, then the authentication should be rejected.

[LeoniePhiline]

Very good point. May be better in a new issue?

[driesvints]

@securit doesn't the rate limiting of the Two factor auth challenge already prevent this?

[securit]

I don’t believe so.

…

On 5/02/2022, at 3:50 AM, Dries Vints ***@***.***> wrote:  @securit doesn't the rate limiting of the Two factor auth challenge already prevent this? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[driesvints]

@securit why? The current rate limiting already prevents entering the 2FA more than 5 times during a minute: https://github.com/laravel/fortify/blob/1.x/stubs/FortifyServiceProvider.php#L46. It's based on the login ID in session from the user that entered a correct username/password. So I don't see how that's insecure? Happy to be proven otherwise if you can explain.

[securit]

That gives me 4 more attempts to re-use of the current QR code during that minute. The RFC says that once a QR code has been used. I should not be able to be used again. That is where the “ONE” in OTP (One Time Passcode) comes from. Kind regards Greg Munro ***@***.*** +64 22 623 0406

[driesvints]

@securit 5 attempts during a minute hardly sound like a MITM vulnerability to me. I think you're referring to a brute force attack instead? That also, doesn't seems like a vulnerability with rate limiting in play.

I don't want to downplay your concerns because you are right that things can be done slightly better here. But I don't see any major security risks with the current way things work. We'd appreciate PR's to improve this though.

Also, let's keep this discussion out of this issue so we can focus on the topic at hand. If you want to address this feel free to send in a PR, thanks.

[x7ryan]

@driesvints FWIW I agree that is not desired. It wouldn't be MITM I believe the correct categorization would be a Replay Attack. Because if an attacker were to see a user enter a valid code, and also knows or saw the Username and Password there is a short window of time where they could login with it as well. The code is only supposed to be valid for a single use. Even just a short lived cache entry noting the user and code used to check attempts against to ensure it wasn't used already should be sufficient.

[driesvints]

Thanks @x7ryan. I'll note that internally to check into at some point.

[driesvints]

Taylor has been working on this yesterday: #357

[securit]

Yup, saw that and was very pleased :) Thankyou

…

On 18/02/2022, at 9:33 PM, Dries Vints ***@***.***> wrote: Taylor has been working on this yesterday: #357 <#357> — Reply to this email directly, view it on GitHub <#201 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABJE5OMWR27MX4GNWWB5DTU3X74XANCNFSM4WGLDIDA>. You are receiving this because you were mentioned.

[LeoniePhiline]

@driesvints
As fortify 2fa is vulnerable to replay attacks, a security advisory should be published.

[driesvints]

@LeoniePhiline what replay attack?

[LeoniePhiline]

@driesvints As detailed in #201 (comment) and multiple comments above that.

[taylorotwell]

@LeoniePhiline will consider adding that in the future - however I'm not sure I would consider that a super serious vulnerability? A person is literally standing there watching you type, knows your username and password already, and then runs to another computer to login as you before the code expires?

[LeoniePhiline]

@taylorotwell Security vulnerabilities, and thus security advisories, come in all severities.

If you check the Snyk vulnerability database, you find multiple dimensions and an overall severity score:

• Attack Complexity
• Availability
• Attack Vector
• Privileges Required
• User Interaction
• Scope
• Confidentiality
• Integrity

No matter your subjectively perceived severity: Never hold back knowledge about a vulnerability, and never refrain from fixing it asap.

Even more, I would advise talking to security specialists about all of the dimensions above. Experts in the field will have a much better understanding of the actual severity.

Exploits commonly use chains of vulnerabilities. Therefore, any vulnerability is serious, albeit some have a much higher severity than others.

Replay attack vulnerabilities in authentication routines of widely used software are not to be taken lightly.

The 2nd authentication factor exists to protect Confidentiality, Integrity and Availability of data in (sadly very common) situations where the password is already compromised.

Reminder why we do 2FA at all: The password is "something you know" while the 2FA generator is "something you have".

With Laravel Fortify being vulnerable to replay attacks, the "something you have" is no longer necessary. The vulnerability makes it so that the attacker only needs two "something you know"s. They need no "something you have" any more:

By definition, the ONE time password (OTP, see also #201 (comment)) must be guarded from re-use by explicitly disallowing the code to be used more than once. Otherwise, the code can be fished off the wire by an attacker, and used together with the compromised password to authenticate and gain access to protected data and permissions.

These kinds of attacks might be performed similar to as you detailed, or the vulnerability would be used to gain privilege escalation in a situation where the attacker has already gained access to parts of the system.

Imagine - just as an example - someone gaining access to incoming requests of an internal network, behind a TLS-terminating reverse proxy. In this internal network, HTTP traffic is unencrypted.

If Laravel was not vulnerable, then 2FA would protect users, as the attacker could read the password and OTP being sent.
Laravel would then only authenticate the original user, and not the attacker. Because when the OTP is being re-sent (the authentication request replayed) then Laravel would deny an authentication attempt using an OTP that has already been used.

However, since Laravel is vulnerable to this attack, it would - in violation of the TOTP spec - accept the one time password a second time, which severely weakens the chain of protective measures and makes 2fa "almost useless" as a security feature.

[taylorotwell]

If you want to contribute to this open source library feel free to do so.

[driesvints]

This one will be released today.

[driesvints]

A Jetstream update is still pending btw.

[driesvints]

@x7ryan @LeoniePhiline @securit I attempted at fixing that issue you described here: #366. Would appreciate your thoughts if that looked good or not.