Nodebestpractices: 6.12 Recommended package express-brute does not do well on high traffic. Race conditions.

[animir]

express-brute package has an unpleasant vulnerability.
It allows to make thousands of password tries because of race conditions. Increment operation is not atomic there. It is true for Redis, Memcached, etc as well. Set/Get approach implemented there.
There is related issue https://github.com/AdamPflug/express-brute/issues/46

It results to an ability to brute-force passwords during small time window and then repeat it several times per day, depending on internal maxWait. If a hacker can instantly make 1000 requests, which is quite simple. And if maxWait is set to 1 hour as package readme example describes, a hacker can try 24000 passwords per day. It creates a possibility for slow brute force attacks.

[goldbergyoni]

@animir Always love to revise and improve our advice. Thanks for suggesting

Looping @lirantal and @js-kyle and myself. Will take a deeper look soon

[lirantal]

@i0natan I believe we were recently discussing an alternative that could fit for this case: rate-limiter-flexible

[goldbergyoni]

Yes @lirantal , @animir (PR opener) is the author and we are engaged here on multiple security issues.

@animir - I've kind of lost the context, did we recommend express-brute? where? how does your great package handle this better?

[animir]

@i0natan 6.12 recommends express-brute
rate-limiter-flexible makes atomic increments, but this is not about which is better. I was investigating why express-brute so popular, found that issue and shared it here, so you can improve 6.12

I think, there should be a notice about race conditions. as I see, developers don't care about race conditions, because it still makes a job, even if it allows to make 50 requests instead of 5.

I've recently created an issue https://github.com/TryGhost/Ghost/issues/10422. Ghost project uses express-brute and it is fine for them to have that package.

I would be happy to prepare PR with adding notice about that.

[animir]

I've prepared PR to express-brute package https://github.com/AdamPflug/express-brute/pull/80
I think, it is better if the issue is noted straight in readme, so advice on 6.12 can be kept as is.

[goldbergyoni]

@animir Can you explain to me, in layman terms, what is being incremented exactly, the difference between race conditions increments and atomic ones?

[animir]

@i0natan sure. It counts number of events. Depending on count, app behaviour is different.
Get/set approach:

1. get N
2. some app logic
3. set N + 1

As a result, if 2 or more requests get the same N, do some logic, then set N + 1, the second request overwrites the first set.

Atomic increment does it straight on DB/store and then return result:

1. set N + 1 and return new value
2. some app logic

[animir]

@i0natan @lirantal Hey.
I opened PR to add a note about possible race conditions to that package's readme https://github.com/AdamPflug/express-brute/pull/80 more than a month ago.

Then we had a short discussion with maintainer https://github.com/AdamPflug/express-brute/issues/81 and the PR is still there not merged.

I suggest to remove this package from recommended, as author doesn't care much about users.

Also, I suggest to consider this article https://medium.com/@animirr/brute-force-protection-node-js-examples-cd58e8bd9b8d as foundation for the future content on 6.12.

[goldbergyoni]

@animir It seems like the author admits that the package is not really maintained

@js-kyle What do you think: seems like a fair ask to remove our recommendation from a package that is not repsonding to PRs?

[js-kyle]

Going to pick up improvements to 6.12 in the next few days!

[goldbergyoni]

@js-kyle 💯 👍

[animir]

@js-kyle I've created a PR https://github.com/i0natan/nodebestpractices/pull/366, which should be helpful

1. remove non-maintained package examples and links.
2. provide the best practice example to prevent brute-force attack against authorization.

[js-kyle]

Looks good @animir, I just had a couple of minor comments

[js-kyle]

@animir thanks heaps for your work on this one, and making changes based on our feedback etc 👍

[animir]

@js-kyle btw, that express-brute bug is official on NPM now

Here is investigation described https://dev.to/animir/non-atomic-increments-in-nodejs-or-how-i-found-a-vulnerability-in-express-brute-package-1ncj