akaunting / laravel-firewall

Web Application Firewall (WAF) package for Laravel
https://akaunting.com
MIT License
943 stars 106 forks source link

addressing attempts count issue #66

Closed Carnicero90 closed 1 year ago

Carnicero90 commented 1 year ago

Hey,

This commit tackles a crucial issue within the firewall's handle function, rectifying a potential vulnerability that could enable an infinite number of attacks to bypass the auto-blocking mechanism. The bug stemmed from inadequate handling of middleware-specific conditions and incorrect verification of the attempt count.

Changes made:

Added middleware-specific filtering to accurately assess logs. Rectified the verification condition to ensure the count remains below the threshold for the specified middleware. This fix is paramount to fortify our defense against attacks. By eliminating this loophole, we thwart the possibility of unchecked and persistent attacks.

Appreciate your time in reviewing and considering this commit.

Best regards, Filippo Montani

denisdulici commented 1 year ago

The middleware condition looks good but the count sign is not. Once the IP is recorded, it won't be able to hit again.

Carnicero90 commented 1 year ago

Not sure I get what you mean: I am checking if count is less than the logs cos otherwise if the "attacker" exceeds the number of allowed attempts (i.e.: loading a more restrictive configuration, with a lower number of attempts) he kinda gets a free pass (since its hits wont be == to the configured number of attempts. My apologies for not making myself clear in my previous pr message (and for sending a nonsensical clearly a.i. generated message).

denisdulici commented 1 year ago

The attacker can not exceed the number of allowed attempts because they'll be blocked once they reach the number of attempts.

Carnicero90 commented 1 year ago

Well it was happening, which is the main reason why I made this pr. And I think I gave a reasonable example of how it could still happen in my previous message, let me try to be more clear:

  1. firewall.middleware.url.auto_block.attempts set to 5;
  2. I notice some attempts to hit domain.com/.env, I update my firewall.middleware.url.auto_block.attempts to 1
  3. config clear
  4. whoever ip had from 2 to 4 logs in the database, now won't ever be banned, since his number of attempts would exceed the number stored in firewall.middleware.url.auto_block.attempts;

having said that, my main issue was with the missing filtering condition (for similar reasons: it was causing some scrapers to get an "infinite" number of attempts in an application), so if you still are not convinced with the less than instead of not equal, I/you can safely remove it from my commit, and I would eventually submit another pull request with more robust testing/explanation in a future.

denisdulici commented 1 year ago

Feel free to remove the count change, and I'll merge the PR.

Carnicero90 commented 1 year ago

okok, removed