Skip to content

[Security][RateLimiter] Added request rate limiter to prevent breadth-first attacks #38308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 1, 2020
Merged

[Security][RateLimiter] Added request rate limiter to prevent breadth-first attacks #38308

merged 1 commit into from
Oct 1, 2020

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Sep 26, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? yes
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

This allows limiting on different elements of a request. The normal CompoundLimiter requires the same key for all its limiters.

This request limiter is useful to e.g. prevent breadth-first attacks, by allowing to enforce a limit on both IP and IP+username. It can also be useful for applications using some sort of API request limiting (or e.g. file upload limiting).

The default login throttling limiter will allow max_attempts (default: 5) attempts per minute for username + IP and 5 * max_attempts for IP. Customizing this will require creating a new service that extends AbstractRequestRateLimiter and implementing getLimiters(Request $request): LimiterInterface[].

@wouterj wouterj changed the title [Security][RateLimiter] Added compound request rate limiters to prevent breadth-first attacks [Security][RateLimiter] Added request rate limiter to prevent breadth-first attacks Sep 26, 2020
@wouterj wouterj force-pushed the security/breadth-first-attacks branch from 4d9b8be to 36b5f98 Compare September 26, 2020 18:21
@nicolas-grekas nicolas-grekas added this to the next milestone Sep 27, 2020
@wouterj wouterj force-pushed the security/breadth-first-attacks branch 3 times, most recently from 61f02fb to 21e97c2 Compare September 27, 2020 21:13
@wouterj wouterj force-pushed the security/breadth-first-attacks branch 2 times, most recently from cddd5ef to 209cda6 Compare September 28, 2020 11:57
@stof
Copy link
Member

stof commented Sep 28, 2020

@wouterj why is the reset method part of the interface while the previous discussion agreed on removing it ?

@wouterj
Copy link
Member Author

wouterj commented Sep 28, 2020

@wouterj why is the reset method part of the interface while the previous discussion agreed on removing it ?

I'm thinking about the interface/abstract class as a generic "request rate limiter" - not necessarily limited to login throttling. The normal LimiterInterface also has a reset() method and when looking at reference implementations (Laravel's and Graham Campbell's limiters), they have a clear/reset functionality as well.

Instead of duplicating the LimiterInterface, we could also remove the string typehint from that interface - and allow any type of data to be passed. That would make the component even more flexible, but we're loosing the benefits of more strictly typed contracts.

@wouterj wouterj force-pushed the security/breadth-first-attacks branch from 209cda6 to 857a989 Compare September 29, 2020 11:16
@fabpot
Copy link
Member

fabpot commented Sep 30, 2020

Can you rebase to take changes from #38257 into account?

@wouterj wouterj force-pushed the security/breadth-first-attacks branch from 857a989 to 6edfae3 Compare September 30, 2020 10:05
@wouterj
Copy link
Member Author

wouterj commented Sep 30, 2020

Rebased and added the Limit logic to the request rate limiter (and also updated login throttling failure message to indicate wait time).

@wouterj wouterj force-pushed the security/breadth-first-attacks branch 3 times, most recently from 9eaab61 to ccf7c33 Compare September 30, 2020 13:11
@chalasr
Copy link
Member

chalasr commented Sep 30, 2020

Rebase needed again :)

@wouterj
Copy link
Member Author

wouterj commented Sep 30, 2020

Yes, and Fabien discovered a bug: the login throttling feature doesn't yet work if the username is invalid. I'll investigate later this day.

@wouterj wouterj force-pushed the security/breadth-first-attacks branch from ccf7c33 to 95fc76a Compare September 30, 2020 19:17
@wouterj
Added request rate limiters and improved login throttling
5d03afe 
This allows limiting on different elements of a request. This is usefull to
e.g. prevent breadth-first attacks, by allowing to enforce a limit on both IP
and IP+username.
@wouterj wouterj force-pushed the security/breadth-first-attacks branch from 95fc76a to 5d03afe Compare September 30, 2020 19:18
@wouterj
Copy link
Member Author

wouterj commented Sep 30, 2020

Rebased and fixed the last bug + added tests

@fabpot
Copy link
Member

fabpot commented Oct 1, 2020

Thank you @wouterj.

@fabpot fabpot mentioned this pull request Oct 1, 2020
Closed
@fabpot fabpot merged commit 127724d into symfony:master Oct 1, 2020
@wouterj wouterj deleted the security/breadth-first-attacks branch October 1, 2020 08:06
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@MatTheCat MatTheCat mentioned this pull request Apr 22, 2021
Closed
@tostiheld tostiheld mentioned this pull request Dec 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof requested changes

@fabpot fabpot fabpot approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Feature Security Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
7 participants
@wouterj @stof @fabpot @chalasr @COil @nicolas-grekas @carsonbot