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

Lock down accounts by IP after N failed attemps at logging in #13472

Merged
merged 28 commits into from Dec 10, 2018
Merged

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Sep 24, 2018

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.

@tsteur tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Sep 24, 2018
@tsteur tsteur added this to the 3.7.0 milestone Sep 24, 2018
@sgiehl
Copy link
Member

sgiehl commented Sep 24, 2018

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
Copy link
Member Author

tsteur commented Sep 24, 2018

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
Copy link
Member Author

tsteur commented Nov 6, 2018

Todo: We need to update either this PR or #13670 and
#13683 depending on which one gets merged first to apply brute force / lock down changes to #13670

"CurrentlyBlockedIPs": "Currently blocked IPs",
"IPsAlwaysBlocked": "These IPs are always blocked",
"UnblockAllIPs": "Unblock all currently blocked IPs",
"CurrentlyBlockedIPsUnblockInfo": "You can unblock IPs that are currently blocked so they can log in again in case they were falsely flagged and need to be able to log in again.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think the last part could be changed to just "in case they were incorrectly flagged"

expect.screenshot("bruteforcelog_blockedapi").to.be.capture(function (page) {
page.load(apiAuthUrl);
}, done);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one could be tested in a system test w/ Http::sendRequest(), in case you don't want to keep the screenshot around.

@diosmosis
Copy link
Member

Reviewed code, haven't tested locally though

@Findus23
Copy link
Member

Findus23 commented Dec 3, 2018

@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 (#11071))

@tsteur
Copy link
Member Author

tsteur commented Dec 6, 2018

@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
Copy link
Member

Findus23 commented Dec 8, 2018

@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
Copy link
Member Author

tsteur commented Dec 8, 2018

Can you create a new issue for this?

@tsteur
Copy link
Member Author

tsteur commented Dec 9, 2018

@diosmosis fyi fixed the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lock down accounts by IP after N failed attemps at logging
4 participants