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

Closed
wants to merge 7 commits into 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 :)

@evereq
Copy link
Member

evereq commented Jan 15, 2025

@samuelmbabhazi please work on this PR to improve following:

  1. Make sure the rate limit applies ONLY to that route authenticate, not globally.

I.e. remove that code

  // rate-limiting middleware
  app.use(
    rateLimit({
      windowMs: 15 * 60 * 1000, // 15 minutes
      max: 20, // limit each IP to 20 requests per windowMs
    }),
  );

and put code inside authenticate method that use rateLimit function from package express-rate-limit

  1. Check if express-rate-limit supports redis as a distributed storage. If yes, make sure it's possible to OPTIONALLY use redis (e.g. if env vars set, use redis, if not set, don't use redis). If library don't support redis / you can't find any other plugins, let me know, we will check other options (we use NestJs rate limiting in other projects, but this one not use NestJs yet, so have to find something for express please!)

  2. Put back code below, it's needed

 const timeThreshold = moment()
      .subtract(15, 'minutes')
      .toDate();

    // If lockout time has passed, reset counter
    if (user.lastLogin < timeThreshold) {
      user.loginAttempts = 0;
      // Otherwise abort request
    } else if (user.loginAttempts >= 3) {
      throw new TooManyRequestsException('too many login attempts');
    }

However make sure (very important!) that such code throw exactly same exception as in case if user not found in DB or any other errors. Our goal is to make sure that method/action "authenticate" always return SAME exception, no matter what happened so that attackers can't figure out more details etc.

@evereq evereq self-requested a review January 15, 2025 17:18
@evereq evereq marked this pull request as draft January 15, 2025 17:18
@evereq evereq added this to the Traduora 1.0 milestone Jan 15, 2025
@evereq
Copy link
Member

evereq commented Feb 23, 2025

We will cover this issue in #445, closing this PR

@evereq evereq closed this Feb 23, 2025
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.

9 participants