Page MenuHomePhabricator

Log unsuccessful login attempts in CheckUser
Closed, ResolvedPublic

Description

Problem:
LoginNotify does not leave a trace in the CheckUser logs when there are unsuccessful attempts. Lacking integration with local CheckUser tables prevents this feature from being used by community processes to investigate this point of abuse.

Solution:
Send login attempt information to local CheckUser tables using CU extension hooks from LoginNotify.

Benefit:
This will let checkusers to be able to keep tabs on and if necessary, block IPs or IP ranges from which such login attempts happen repeatedly. This will potentially be able to prevent account hacks even in cases where users have weak passwords.

Event Timeline

Keegan created this task.Aug 29 2017, 8:14 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 29 2017, 8:14 PM
Keegan triaged this task as Medium priority.Aug 29 2017, 8:14 PM

This task can probably have the problem reworded more elegantly.

Niharika updated the task description. (Show Details)Aug 29 2017, 8:44 PM
Keegan updated the task description. (Show Details)Aug 29 2017, 8:56 PM
TheDJ added a subscriber: TheDJ.Aug 30 2017, 8:47 AM
MaxSem renamed this task from Integrate Extension:LoginNotify with CheckUser tables to Log unsuccessful login attempts in CheckUser.Sep 22 2017, 8:11 PM
Samtar added a subscriber: Samtar.Oct 20 2017, 8:32 PM
mxn added a subscriber: mxn.Oct 24 2017, 5:25 PM
Huji added subscribers: kaldari, Legoktm, Huji.EditedFeb 3 2018, 3:29 AM

@kaldari @Legoktm can I ask your opinion here?

I am working on a patch that is modeled after how AbuseFilter stores its logs in CheckUser. The issue is, AbuseFilter actually has a log, and so it takes an entry from that log to create a RecentChange object using [[ https://phabricator.wikimedia.org/source/mediawiki/browse/master/includes/logging/LogEntry.php;eff03050215f757bf6e04f021abb9b9d4b0629b4$682 | RecentChage::newLogEntry ]] (via [[https://phabricator.wikimedia.org/diffusion/EABF/browse/master/includes/AbuseFilter.class.php;0019dc226502c3f6d5ab0cff864cc5ad6a5d344c$1204 | LogEntry::getRecentChange ]]) and then [[ https://phabricator.wikimedia.org/diffusion/EABF/browse/master/includes/AbuseFilter.class.php;0019dc226502c3f6d5ab0cff864cc5ad6a5d344c$1205 | passes that RecentChange object to CheckUser ]]. LoginNotify, on the other hand, does not have a log.

Should we create a log for LoginNotify (but restrict access to it, and most likely not give anyone access to it on WMF wikis)? Or do you know of a better strategy for generating an RC object?

Update: I am now more inclined to just add a new hook specifically for this use case. If you think that is the better approach, please abandon my patch below.

Huji claimed this task.Feb 3 2018, 11:19 PM

Change 408022 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/LoginNotify@master] WIP: DO NOT MERGE

https://gerrit.wikimedia.org/r/408022

Huji added a comment.Feb 4 2018, 1:38 AM

I created an initial non-working draft of the idea I have, in the patch above. Your guidance is much appreciated!

Another important question to answer at the same time: do we want to log the events in CU only for the initiating IP, or both for that and for that target user? In other words, if IP 1.2.3.4 makes a failed attempt to log into my account (User:Huji), when I run a check on 1.2.3.4 I will see that; but should I be able to also see that by running a check on User:Huji?

It would be beneficial to see that, because if a user complains about multiple failed login attempts, then I can find the origin of those and take action. But CU currently does not support that (the only way to achieve it is to generate two rows of CU data, one for the originator, one for the target).

Alternatively, we can say that only one row of data should be stored in CU, and that is for the originating IP. Users who have a complaint should report the IP to the CU themselves (that assumes we show the IP to the target users via Echo, subject of discussion in T174388; if we decide to only show a location and the IP, then this approach won't be realistic).

Change 408049 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/CheckUser@master] Log unsuccessful login attempts in CheckUser

https://gerrit.wikimedia.org/r/408049

Change 408022 abandoned by Huji:
WIP: DO NOT MERGE

https://gerrit.wikimedia.org/r/408022

Change 408050 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/LoginNotify@master] Log unsuccessful login attempts in CheckUser

https://gerrit.wikimedia.org/r/408050

Huji added a comment.Feb 4 2018, 3:09 PM

This approach (combinatino of https://gerrit.wikimedia.org/r/#/c/408050/ to call a hook and https://gerrit.wikimedia.org/r/#/c/408049/ to define the callback function) seems more reasonable. I cannot get it to work though; when I try it, I get this error message: MWException from line 167 of /var/www/html/includes/Hooks.php: Invalid callback CheckUserHooks::logLoginAttempt in hooks for LoginNotifyFailedAttempt

Not sure what I have done wrong.

@Hugi: I think CheckUserHooks::logLoginAttempt will need to be public, not protected.

Huji added a comment.EditedFeb 4 2018, 3:15 PM

@Platonides you are absolutely correct!

Now, when I run $dwb->insert() is there a way for me to get back the cuc_id of the newly inserted row? We would need this to be used in T174388

Use $dbw->insertId()

Masti added a subscriber: Masti.Feb 6 2018, 12:52 PM

Change 408050 abandoned by Huji:
Log unsuccessful login attempts in CheckUser

Reason:
Change of strategy in the dependency

https://gerrit.wikimedia.org/r/408050

Change 408049 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Log unsuccessful login attempts in CheckUser

https://gerrit.wikimedia.org/r/408049

Huji closed this task as Resolved.Feb 21 2018, 1:29 AM
Huji removed a project: Patch-For-Review.
jrbs added a subscriber: jrbs.May 3 2018, 5:03 PM

@Huji Sorry I'm late to the party - can you tell me where are you surfacing these in the UI? I'm wondering if we can do the same for the new Special:Investigate page.

Reedy added a subscriber: Reedy.EditedJun 3 2020, 2:24 AM

@Huji Sorry I'm late to the party - can you tell me where are you surfacing these in the UI? I'm wondering if we can do the same for the new Special:Investigate page.

It chucks a row in cu_changes :)

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CheckUser/+/408049/14/includes/CheckUserHooks.php

So anywhere that queries it should bring them up...

Except, it's not enabled on WMF wikis just yet as per T253802: Configure WMF wikis to log login attempts in CheckUser

Gotcha! Thanks @Reedy!