Add optional ThrottlingListener to throttle two factor attempts

[tostiheld]

Description
We recently had our platform pentested and one of the findings was that two factor codes were able to be brute forced by an attacker. In our case, quite easily in fact, because we are using TOTP with 6 digit codes. I added this listener to our core library, and now all our applications are protected from brute forcing the two factor codes. I thought it might be nice to (optionally) add this to this bundle. Let me know what you think. If you think it's a good idea, I will add a unit test.

[wouterj]

Given this is a colleague of mine, I think this is a good idea :)

Just a little heads-up: Symfony 6.2 introduced peekable rate limiters in order to reduce load on the limiter/lock systems: symfony/symfony#46110 It would be a good idea to use them in this listener for Symfony 6.2 applications (this can be checked using class_exists(\Symfony\Component\HttpFoundation\RateLimiter\PeekableRequestRateLimiterInterface::class)).

[scheb]

Could make sense to add this to the bundle. I have this page in the documentation for ages, but I don't know how many people actually read and do it. I think it makes sense to encourage people to adopt good security practices by adding an out-of-the-box solution.

Two things that I'm currently concerned about:

• I'd make this optional, per default disabled. I don't want to introduce new hidden behavior, when people are upgrading to the new minor version. Could be default enabled with the next major release.
• I don't want to force another dependency onto people with symfony/rate-limiter, one that they might not even be using. So I'm wondering how to do that. Either make it another sub-package of the bundle to be installed, or not making it a hard dependency and checking on container compilation - when the feature is enabled - if the component is present.

[tostiheld]

I see I made a mistake (I always register the listener, even though I intended for that to be optional). [Edit: upon further inspection I think I did it right the first time? Please let me know, I'm not super familiar with bundle configuration]

I agree with the 2 points you raised @scheb, and I tried to implement it like that. I added symfony/rate-limiter to dev-dependencies for that reason as well.

[scheb]

Necessary, so that I can consider this for merge:

• Fix code style issues
• Documentation for the new configuration setting
• Test coverage

[scheb]

Please remove the final keyword and add PHPDoc @final (check other classes for reference). That's how I do it in the library to keep myself the ability to mock things if necessary.

[hdk-pd]

I'm reading up on Symfony a bit and looking at some code to dig into the concepts and ecosystem and came across this, so I'm sorry if I am a bit off in some technical details.

The throttle addresses TwoFactorAuthenticationEvents::ATTEMPT events, in plain words: a successful login attempt with correct account credentials and the rate limit key is the request IP, right? So I think the threat model here is someone trying to gain access to a specific account or a specific set of accounts. I don't think the usual automated credential sprayers that are doing this at scale will continue when faced with a 2FA authentication - generally speaking.
My suggestion would be to use the users identifier instead of the request IP as rate limit key. Checking the event and the TokenInterface, I think this should be what I'm suggesting:

$rateLimiter = $this->rateLimiterFactory->create($event->getToken()->getUserIdentifier());

This highly depends on the rate limiter and code generator configuration and implementation. Given a relatively short 2FA code and simple code (i.e. 6 digit) and some attempts available, access to accounts could possibly be gained with several cloud providers, proxy services etc. (Example bug bounty writeup for an Instagram 2FA brute force rate limit issue)
With my suggestion, an attacker with a single IP might have more attempts in total, but all on different accounts with different 2FA codes to brute force. However, an attacker looking for a specific account can not get around the rate limit key with different IPs. Every user will have a specific set of attempts. The worst scenario I could imagine is an attacker creating a DoS scenario for a victims account by blocking the 2FA functionality with ongoing attempts. Given this requiring the attacker to know the users credentials, this might be an acceptable risk and can be fixed with a password reset that is overdue anyways. Privacy enthusiasts behind a VPN or Tor might be blocked by the IP key, too.
Please let me know what you think.

[scheb]

Closing due to inactivity.