Implement brute force protection

[LukasReschke]

Class Throttler implements the bruteforce protection for security actions in
Nextcloud.

It is working by logging invalid login attempts to the database and slowing
down all login attempts from the same subnet. The max delay is 30 seconds and
the starting delay are 200 milliseconds. (after the first failed login)

cc @rullzer Mind taking a look? As discussed :)

[mention-bot]

@LukasReschke, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ChristophWurst, @BernhardPosselt, @icewind1991 and @DeepDiver1975 to be potential reviewers

[rullzer]

private/protected?

[MorrisJobke]

Why not make this simpler? for 8 attempts it's 25,6 seconds and for 9 it's 51,2 seconds. Just say: if ($attempts > 8) ? ... this would make this code more readable.

[LukasReschke]

Mainly because L208 and L209 is something we may want to keep configurable and adjust in the future. Not hard-coding the value in L210 is one less thing to forget.

(magic here required for 32bit PHP 🙈 )

[MorrisJobke]

Why didn't you use PHP_INT_MAX? This is a lot more readable instead of this. Because I don't know exactly if this is with or without signed numbers 🙈

[MorrisJobke]

Shouldn't the timeout be reset on successful login? I also noticed that when I entered a lot of wrong passwords and then the correct one the CSRF token was already invalid for the login request and I got an error page. After a refresh I was logged in, but it feels a bit weird. ;)

[LukasReschke]

Shouldn't the timeout be reset on successful login?

No. Because then you could bruteforce other users. In a future incremential enhancement I'm thinking of adding something like trusted IPs which are trusted for a specific user. (once logged-in with an IP => it gets more login attempts for a specific user)

[MorrisJobke]

And I also noticed that the entry is written after waiting all the time: This means an attacker could send thousands of requests in parallel which then all have nearly the same timeout (a few seconds) and not the full 30 seconds after the 8 attempt is registered. When adding the entry after the password check is done (it is then a penalty that has instant effect).

[MorrisJobke]

No. Because then you could bruteforce other users. In a future incremential enhancement I'm thinking of adding something like trusted IPs which are trusted for a specific user. (once logged-in with an IP => it gets more login attempts for a specific user)

Of course only for the user you just authenticated with and the same IP address. Logging in with a different account would not reset the counter.

[MorrisJobke]

Beside the json_encode this works nicely 👍

[LukasReschke]

And I also noticed that the entry is written after waiting all the time: This means an attacker could send thousands of requests in parallel which then all have nearly the same timeout

Very good catch. I added a mitigation for this at c1589f1

[jknockaert]

Is this still "to review"?
As I and others have argued the throttling should ideally be done at the firewall level. It would not be too difficult to provide the nextcloud admin with the option to choose for a pure php implementation (this one) or an implementation that iteracts with a third party tool such as fail2ban or ossec that manages the kernel-level firewall. The implementation of the latter in nextcloud would be limited to issuing statements to a log file which is subsequently monitored by the third part tool. Supporting more efficient third party stuff is not new to nextcloud, think of the database support both for sqlite (inefficient but pure php) as well as postgres (efficient but third party).

[rullzer]

@jknockaert this is in. Please open a new issue if you have suggestions on how to improve for the next itteration.

[jknockaert]

Doesn't seem like it got two reviews. Did we drop that requirement?

[MorrisJobke]

Doesn't seem like it got two reviews. Did we drop that requirement?

😕 There was a second review by @rullzer ...somehow this is lost ...

Sorry for the confusion, but we didn't dropped the two reviews rule.

[rullzer]

Ah mmm... seems I was a little bit to agressive with deleting comments of still I falsely flagged... my bad.

[Wikinaut]

I am just testing the brute force protection, but I am unhappy with it because the oc_bruteforce_attempts column is not reset when a correct login (from the blocked IP) happened (see above #479 (comment) )

@MorrisJobke @LukasReschke
What is your current state of mind towards a solution of removing the throttling on a correct login ?

[staabm]

Couldnt this mechanics easily be used to DOS the webserver by force submitting bad requests which essentially could finally render all webserver processes sleeping?

[ChristophWurst]

Couldnt this mechanics easily be used to DOS the webserver by force submitting bad requests which essentially could finally render all webserver processes sleeping?

Hi,

please use the forum for this type of question: https://help.nextcloud.com/

Thanks