Make sure to only allow single usage of a 2FA code #277

heiglandreas · 2025-05-06T09:28:58Z

8000

Right now it is possible to use a 2FA code several times as long as the code itself isn't expired.

According to NIST though that should not be possible.

Time-based OTPs [RFC 6238] SHALL have a defined lifetime that is determined by the expected clock drift — in either direction — of the authenticator over its lifetime, plus allowance for network delay and user entry of the OTP. In order to provide replay resistance as described in Section 5.2.8, verifiers SHALL accept a given time-based OTP only once during the validity period. In the event a claimant’s authentication is denied due to duplicate use of an OTP, verifiers MAY warn the claimant in case an attacker has been able to authenticate in advance. Verifiers MAY also warn a subscriber in an existing session of the attempted duplicate use of an OTP. (emphasis mine)

If I see that correctly (please correct me!) there is currently no way of getting this behaviour out of the box from this package.

Would it not be an idea to provide a simple implementation of this behaviour using an existing caching method to implement this? I was thinking about providing a Psr\Cache\CacheItemPoolInterface or a Symfony\Contracts\Cache\CacheInterface in the package-config and when that is available to use that for ensuring single use of the code.

I will have to implement it either way, would it be of interest to do that in a way that it can be added to the package?

The text was updated successfully, but these errors were encountered:

scheb · 2025-05-06T10:05:02Z

This was discussed quite recently in #276.

Short version: There is currently no out-of-the-box mechanic to keep track of used authentication codes and prevent reuse. For custom implementations, it could be achieved by listening to the newly introduced "check" events and throwing an exception, when a reused code is detected.

I'd be open to make this part of the bundle, though I'd be reluctant to introduce a complicate storage mechanism or new dependency to the bundle. Relying on symfony/cache infrastructure seems like a good approach to me, since it's pulled in as a dependency anyways.

For a proper implementation I'd likely add another listener like CheckTwoFactorCodeListener, which checks codes for reuse and throws an exception of ReusedTwoFactorCodeException extends BadCredentialsException. That should make the 2fa authentication fail and make the respective error message show up in the UI.

heiglandreas · 2025-05-06T12:50:13Z

Yeah. My idea was to create a Listener for the scheb_two_factor.authentication.attempt-event that checks the code and - if it finds a non-unique usage - triggers a failure event (possibly a scheb_two_factor.authentication.failure one?) with information about the non-unique usage of the code.

And then also add a listener to that event that throws the ReusedTwoFactorCodeException extends BadCredentialsException.

That way users can implement their own way of handling the non-unique usage without having to catch the exception - like sending an email that the code was used twice or displaying that in the UI or whatever else they deem appropriate - but the exception will be thrown in the end (unless the users remove that listener deliberately)

Does that sound like a feasible way?

scheb · 2025-05-06T14:06:29Z

My idea was to create a Listener for the scheb_two_factor.authentication.attempt-event that checks the code

scheb_two_factor.authentication.attempt would theoretically work, but it has the disadvantage of not having direct access to the authentication code that was used. You'd need to fiddle the authentication code from the request and distinguish different firewalls. Better approach would be:

A listener class similar to CheckTwoFactorCodeListener, which is registered so that is has priority over CheckTwoFactorCodeListener and CheckBackupCodeListener. Such a listener can be injected dynamically based on configuration.
Alternatively, we add a new generic event for "code checking", which provides the code in the event. I've recently added such events for each authentication provider. Would be possible to add a generic event of that kind in CheckTwoFactorCodeListener right before the actual code checking. Though that approach wouldn't recognize when backup code are being used.

if it finds a non-unique usage - triggers a failure event (possibly a scheb_two_factor.authentication.failure one?) with information about the non-unique usage of the code.

And then also add a listener to that event that throws the ReusedTwoFactorCodeException extends BadCredentialsException.

That way users can implement their own way of handling the non-unique usage without having to catch the exception - like sending an email that the code was used twice or displaying that in the UI or whatever else they deem appropriate - but the exception will be thrown in the end (unless the users remove that listener deliberately)

I like the customization with that approach. But I'd introduce a new event to signal the reuse of a code, e.g. scheb_two_factor.authentication.code_reuse. The scheb_two_factor.authentication.failure event should only be dispatched, when there was an actual failure in the authentication process. A code reuse wouldn't be necessary a failure, since you mentioned it could also result in an email/alert being sent. So keeping the two events separate makes more sense to me.

Open questions:

What should the default behavior be in your opinion?
And how would you make the "used code storage" customizable?
How would you make the "handling" of a code reuse customizable?

heiglandreas · 2025-05-06T14:30:55Z

I'd prefer to go with the new generic event for "code checking" as it seems to be more specific. And not being triggered by the backup-codes sounds like a feature. Those should be one time usage anyhow. Or is that currently not implemented that way? 🙈

What should the default behavior be in your opinion?

I would throw a ReusedTwoFactorCodeException as default. As the default only triggers when a caching backend is configured that should be backwards compatible.

And how would you make the "used code storage" customizable?

I was thinking about adding a scheb_two_factor.totp.code_reuse_cache configuration value that can reference a caching-implementation to use. If that caching implementation is null or does not implement the Psr\Cache\CacheItemPoolInterface or the Symfony\Contracts\Cache\CacheInterface no caching is assumed and the Code is not cached and so no check is happening.

How would you make the "handling" of a code reuse customizable?

By adding EventHandlers/-Subscribers that listen to the scheb_two_factor.authentication.code_reuse event people can add their own logic and so customize the behaviour of their application to a code-reuse. If they do not remove the ThrowExceptionOnMfaCodeReuse-EventHandler from the queue within their code, that one is triggered at a very late priority and then throws the ReusedTwoFactorCodeException.

Does that sound plausible?

Yes! the documentation needs to be amended as well then

heiglandreas · 2025-05-06T14:47:58Z

code-organization-wise I'd see that all within the TwoFactor folder.

TwoFactor\Event\TwoFactorCodeCheckEvent
Triggered in CheckTwoFactorCodeListener::isValidCode - This would mean injecting the Dispatcher into the constructor
TwoFactor\Http\EventListener\CheckTwoFactorCodeReuseListener
This would do the "hard work" of checking whether the injected caching implementation is usable and if so whether the provided code is already used. If not: Do nothing.
TwoFactor\Event\TwoFactorCodeReusedEvent
TwoFactor\Http\EventListener\ThrowExceptionOnTwoFactorCodeReuseListener
This would listen to the above event with a priority of -256 (should be low enough)

Only the exception I'D see in Authentication\Exception\ReusedTwoFactorCodeException

scheb · 2025-05-06T15:16:21Z

I'd prefer to go with the new generic event for "code checking" as it seems to be more specific. And not being triggered by the backup-codes sounds like a feature. Those should be one time usage anyhow. Or is that currently not implemented that way? 🙈

👍 Yes, it is implemented, so that backup codes are invalidated, once they're used. But people do all kinds of stuff 🤷‍♂. But I agree, backup codes don't need to be covered by this feature, as the nature of backup codes is different than regular authentication codes.

What should the default behavior be in your opinion?

I would throw a ReusedTwoFactorCodeException as default. As the default only triggers when a caching backend is configured that should be backwards compatible.

👍

And how would you make the "used code storage" customizable?

I was thinking about adding a scheb_two_factor.totp.code_reuse_cache configuration value that can reference a caching-implementation to use. If that caching implementation is null or does not implement the Psr\Cache\CacheItemPoolInterface or the Symfony\Contracts\Cache\CacheInterface no caching is assumed and the Code is not cached and so no check is happening.

👍 Sounds good. Though I think this should become option scheb_two_factor.code_reuse_cache, instead of being something specific to TOTP, since we're implementing it to a generic "code check" event, which would be dispatched for all kinds of authentication providers. Effectively making this a generic bundle-level feature, independent of the authentication provider.

How would you make the "handling" of a code reuse customizable?

By adding EventHandlers/-Subscribers that listen to the scheb_two_factor.authentication.code_reuse event people can add their own logic and so customize the behaviour of their application to a code-reuse. If they do not remove the ThrowExceptionOnMfaCodeReuse-EventHandler from the queue within their code, that one is triggered at a very late priority and then throws the ReusedTwoFactorCodeException.

Would work for me. Though I'm wondering if we could provide an easier way to replace the default listener with a custom implementation. Some configuration option.

Yes! the documentation needs to be amended as well then

Absolutely :)

TwoFactor\Event\TwoFactorCodeCheckEvent
Triggered in CheckTwoFactorCodeListener::isValidCode - This would mean injecting the Dispatcher into the constructor

You can use the existing class Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorCodeEvent, which is also used by the authentication providers to dispatch their "code check" events.

The new event would likely need to go here, btw.:

2fa/src/bundle/Security/Http/EventListener/CheckTwoFactorCodeListener.php

Line 38 in 6cd1824

TwoFactor\Http\EventListener\CheckTwoFactorCodeReuseListener
This would do the "hard work" of checking whether the injected caching implementation is usable and if so whether the provided code is already used. If not: Do nothing.

Agreed. Though should be next to the other event listener in the Scheb\TwoFactorBundle\Security\Http\EventListener namespace.

TwoFactor\Event\TwoFactorCodeReusedEvent

Also in the Scheb\TwoFactorBundle\Security\TwoFactor\Event namespace. Potentially you can also use Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorCodeEvent for that.

TwoFactor\Http\EventListener\ThrowExceptionOnTwoFactorCodeReuseListener
This would listen to the above event with a priority of -256 (should be low enough)

Yes, makes sense.

Only the exception I'D see in Authentication\Exception\ReusedTwoFactorCodeException

Should be next to InvalidTwoFactorCodeException in the Scheb\TwoFactorBundle\Security\Authentication\Exception namespace.

heiglandreas · 2025-05-06T15:23:26Z

I better get going then...

scheb · 2025-05-06T15:24:31Z

No need to rush :)

heiglandreas added the Enhancement label May 6, 2025

heiglandreas mentioned this issue May 8, 2025

Handle multiple usages of an MFA-code #279

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure to only allow single usage of a 2FA code #277

Make sure to only allow single usage of a 2FA code #277

Make sure to only allow single usage of a 2FA code #277

Make sure to only allow single usage of a 2FA code #277

Comments