Skip to content
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.

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

Add Rate Limiting to specific endpoints - huntr.dev #177

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

huntr-helper
Copy link

https://huntr.dev/users/arjunshibu has fixed the Lack of Rate Limiting vulnerability 🔨. arjunshibu has been awarded $25 for fixing the vulnerability through the huntr bug bounty program 💵. Think you could fix a vulnerability like this?

Get involved at https://huntr.dev/

Q | A
Version Affected | ALL
Bug Fix | YES
Original Pull Request | 418sec#2
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/other/traduora/3/README.md

User Comments:

📊 Metadata *

Traduora is a translation management platform for teams. Once you setup your project you can import and export your translations to various formats, work together with your team, instantly deliver translation updates over the air, and soon automatically translate your project via third-party integrations.

Bounty URL: https://www.huntr.dev/bounties/3-other-traduora

⚙️ Description *

Lack of Rate Limiting in the login page of traduora.

💻 Technical Description *

Traduora uses a weak algorithm to implement rate limiting, which only tried to protect against incoming requests issued to /api/v1/auth/token. This fix uses the express-rate-limit package. I've implemented the rate limiter as a global middleware so all the /api endpoints are protected.

🐛 Proof of Concept (PoC) *

  • clone the github repo
  • setted up traduora platform to reproduce the vulnerability
  • I used an intruder in BURP SUITE to test for rate limiting on the password field.
  • In normal intruder function it shows status code 429 that is ratelimit function is there
  • To bypass it use intruder with throttle above 700 and use thread 100+ , for wrong password it shows 401 errror if right password comes it shows 200 error.

Existing Protection

poc_protection

Bypass

poc_bypass

🔥 Proof of Fix (PoF) *

After fix, all the API endpoints are protected with rate limiting.

pof_fix

+1 User Acceptance Testing (UAT)

  • I've executed unit tests.
  • After fix the functionality is unaffected.

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2020

CLA assistant check
All committers have signed the CLA.

@JamieSlome
Copy link
Contributor

@arjunshibu - are you able to sign the CLA, thanks! 🍰

Let me know if you have any questions!

@arjunshibu
Copy link

@JamieSlome just did😊

@JamieSlome
Copy link
Contributor

@arjunshibu - awesome, thanks! 👍

Good job by the way!

@anthonynsimon
Copy link
Contributor

Hey thanks for the PR. A couple of notes:

Wouldn't this limit all API reqs per IP to 20 during a 15 minute window? If so, that might be way too low for most users :)

Based on my observed usage a single IP can generate 100s of requests during those 15 minutes to the various endpoints.

I'm more than happy to include rate limiting at the application level, instead of relying on users to set this at the Load Balancer level as it is currently.

But these limits should be set on a per endpoint basis.

Another thing to consider is that for multi-instance deployments the rate limits would need to be stored outside of the application, otherwise they won't be enforced consistently. Something like Redis might be suitable.

Lastly, could you please commit the dependency lock file in case you update the dependencies?

Once again, thanks for contributing!

@arjunshibu
Copy link

@anthonynsimon thank you for reviewing.

But these limits should be set on a per endpoint basis.

Will update soon😊

@anthonynsimon anthonynsimon changed the title Security Fix for Lack of Rate Limiting - huntr.dev Add Rate Limiting to specific endpoints - huntr.dev Feb 23, 2021
@anthonynsimon
Copy link
Contributor

It seems that the PR implementation still rate limits every endpoint with an unreasonably low quota.

As it is I cannot merge this PR, if I should keep this open let me know :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants