@tsteur opened this Pull Request on September 24th 2018 Member

As discussed in #2888 a quite simple implementation that should do the job. Heaps more could be done as mentioned in the comments but might not be needed.

It took me couple hours to understand all the authentication/login flows as there are lots of different components now such as Auth, SessionAuth, SessionInitializer, Access, etc. and it wasn't clear where to put things to make it work universally while still being clear about when it happens etc.

Also had to add couple of events eg for failed logins. The most important part is really to register a failed login attempt when there is one (we often authenticate eg API requests etc even though the user didn't, or we always authenticate first against session then regular auth adapter etc which makes things complicated). And it is important to check whether IP is blocked when trying to attempt to log in. All those parts should be covered.

When an IP is blocked, I did not want to block any already logged in users in the UI. This could be annoying when a company shares the same IP and it would otherwise even block the super user.

The superuser has a diagnostic page to unblock all users and a console command. Can also document in a FAQ how to do it through the SQL database directly.

For system settings and diagnostics page screens see screenshot tests.

Functionality wise you specify two values: X amount of login retries allowed in Y amount of minutes. If you have more than X amount of logins within Y amount of minutes, you are blocked and cannot log in. Even when successful token provided for example. You can only log in again, when some time has passed and you had less than X amount of login retries within the last Y amount of minutes. There is no specific lock out time that you can configure so if people lock themselves out they can use UI again after eg 30 min or 60min. By default we allow 20 logins within 60 minutes. When sharing an IP within an office those 20 logins could be reached within an hour but should be maybe fine. Hard to find the right balance here.

You can also whitelist and blacklist specific IPs.

fixes #2888

I'll try to write some more tests for the different login failures once the PR is reviewed.

@sgiehl commented on September 24th 2018 Member

Haven't tested, but wondering if using "Webcron" (calling the archiver with token_auth in url) will still work if the according account got locked down.

@tsteur commented on September 24th 2018 Member

Just tested it, but it works as it is only an API test.

But realizing now that the UI is sending API requests so there is pretty much no way to keep the UI working for a logged in user when the IP got blocked by a different user. Will need to block the entire UI as it current behaviour it would only block partial UI and look broken.

@tsteur commented on November 6th 2018 Member

Todo: We need to update either this PR or https://github.com/matomo-org/matomo/pull/13670 and
https://github.com/matomo-org/matomo/pull/13683 depending on which one gets merged first to apply brute force / lock down changes to #13670

@diosmosis commented on December 2nd 2018 Member

Reviewed code, haven't tested locally though

@Findus23 commented on December 3rd 2018 Member

@tsteur In case this isn't already included (I didn't check), I think it would be worth it to extend this feature to the password reset feature.

Currently an attacker can try out unlimited password resets, which will send unlimited emails to the Matomo user and if they accidently click on the link in any email, the attacker has immediate full access to the instance (which is another security issue in itself, that maybe should be fixed in one of the next releases (https://github.com/matomo-org/matomo/issues/11071))

@tsteur commented on December 6th 2018 Member

@tsteur In case this isn't already included (I didn't check), I think it would be worth it to extend this feature to the password reset feature.

Currently an attacker can try out unlimited password resets, which will send unlimited emails to the Matomo user and if they accidentaly click on the link in any email, the attacker has immediate full access to the instance (which is another security issue in itself, that maybe should be fixed in one of the next releases (#11071))

Not sure @Findus23 I can add a simple block to password reset page if user is already blocked because of too many failed logins. I reckon in case you locked yourself out it may be good to reach the password reset page though. Once the lock is expired, you can simply log in with the new password... Maybe create a new issue for this, but don't think it has too much of a priority if we should also count the number of resets etc. In theory, just one reset is enough though to possibly trick a user, or one reset every hour... or something. I don't think it adds much to security and rather #11071 can be looked at to be improved eventually.

@Findus23 commented on December 8th 2018 Member

@tsteur I think the main reason why we should limit the password resets is that attackers can create a lot of chaos by requesting hundreds of password resets per minute an spamming the users mailbox, overloading the mailserver or even damaging the spam ranking.
You are right that this is less or a security issue, but more limiting the damage an attacker can create.
I like the position of discourse on having rate limits on everything and it really helps keeping spam out of the Matomo forum as most spammers get caught in some limit before posting.
https://blog.codinghorror.com/rate-limiting-and-velocity-checking/

@tsteur commented on December 8th 2018 Member

Can you create a new issue for this?

@tsteur commented on December 9th 2018 Member

@diosmosis fyi fixed the tests

This Pull Request was closed on December 10th 2018
Powered by GitHub Issue Mirror