Last modified: 2014-09-23 23:06:12 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 9838 - Send notification to account owner on multiple unsuccessful login attempts
Send notification to account owner on multiple unsuccessful login attempts
Status: NEW
Product: MediaWiki
Classification: Unclassified
User login and signup (Other open bugs)
1.20.x
All All
: Normal enhancement with 3 votes (vote)
: ---
Assigned To: Nobody - You can work on this!
http://en.wikipedia.org/wiki/Special:...
: patch, patch-reviewed
Depends on: 32281
Blocks: 1932 9816
  Show dependency treegraph
 
Reported: 2007-05-08 02:51 UTC by Titoxd
Modified: 2014-09-23 23:06 UTC (History)
15 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Uses a session variable to check the number of password attempts (3.47 KB, patch)
2009-02-03 22:23 UTC, Tyler Romeo
Details
Use memcached to check the number of failed login attempts and notify the user. (6.56 KB, patch)
2011-07-17 07:33 UTC, Tyler Romeo
Details
Addition of user notification functionality to user login. (7.55 KB, patch)
2011-07-17 18:21 UTC, Tyler Romeo
Details
New solution to bug (52.11 KB, patch)
2012-06-17 05:11 UTC, Tyler Romeo
Details

Description Titoxd 2007-05-08 02:51:41 UTC
Thinking along the lines of Bug 9816, it would be advisable to send an email
notification to an account owner if someone is trying to log in to an account
and fails X number of times within a particular period of time.
Comment 1 Titoxd 2007-05-08 02:54:27 UTC
Gah, never mind.

*** This bug has been marked as a duplicate of 9836 ***
Comment 2 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-05-09 16:50:13 UTC
Reopening since bug 9836 has been RESOLVED FIXED due to implementation of
captcha.  Repurposing that to be a bit more specific.
Comment 3 Tyler Romeo 2009-02-03 22:23:02 UTC
Created attachment 5780 [details]
Uses a session variable to check the number of password attempts

This diff uses a session variable to e-mail the user every certain number of unsuccessful login events. However, I am not sure how secure this is considering the user could simply end their session by deleting the cookie.
Comment 4 Platonides 2009-02-13 12:46:08 UTC
Instead of a session variable, it should use memcached, as the password throttle.
With that implementation, asking for the password 9 times, produces 3 emails saying that there were 'three or more attempts'. It should try to provide the exact number.
In fact, the email should be delayed until after X time without login attempts or the ip is throttled.

Also it has to be considered that it is a way of spamming.
Comment 5 Tyler Romeo 2009-02-13 18:17:04 UTC
It would be an advantage to have it memcached, but it would need to fall back on something for implementations that do not use caching.
Comment 6 p858snake 2011-04-30 00:10:17 UTC
*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*
Comment 8 Tyler Romeo 2011-07-17 07:33:11 UTC
Created attachment 8793 [details]
Use memcached to check the number of failed login attempts and notify the user.

Hmm, what do you know, now that the bug has resurfaced I actually disagree with what I said before. XD

Regardless, attached is a new patch with a number of differences from my old patch:

1) Using memcached instead of a session variable as suggested because of the numerous advantages it has (such as if the user resets the session). Made the functionality similar to how login throttling works.
2) The wfGetIp will throw an error if it doesn't actually return an IP, so there's no need to have that checked when notifying the user.
3) Improved my previous notification, since the last one I wrote was pretty bad.
4) Removed the error that is returned if the user does not have an email. The behavior should be that the wiki stays silent b/c all throwing an error does is tip off the hacker that the user has no email and will never be notified about the failed password attempts.
5) Removed the hook for notifying the user. IMO it seems pretty useless. If anything, a hook MIGHT be useful in the actual mechanism for measuring the number of password attempts, but that's an unrelated issue that could be raised in another bug report if the issue arises.
6) I implemented a system where the user receives one email after the first three failed attempts and then a more serious email if the problem progresses, mainly because this functionality will (and should) not notify the user constantly, so the user may choose to ignore the first email while failed password attempts to continue in the background.

One other thing I would ask for some comment on is the possibility of having multiple sets of password throttling settings: the current functionality of password throttling is so that no IP can try too many password attempts in a given amount of time, but what if a cracker continues to try password upon password? IMO, it would be helpful to, rather than just have the person wait 300 seconds to begin trying, have the possibility (though not necessarily the requirement) of more serious throttling as the failed password attempts continue. This is really unrelated and belongs in a separate bug report, but I figured I'd ask about it before making a new report and whatnot.
Comment 9 Platonides 2011-07-17 13:34:29 UTC
A few minor issues:
I guess you are using "if ( is_array( $wgFailedPasswordAttemptNotifcation ) ) {" so that it could be disabled with $wgFailedPasswordAttemptNotifcation = false; or something similar. Please document.
New messages have to be added to maintenance/language/messages.inc
Do not use magic values as 172800 (48h)
Use tabs, not spaces.
There's a mising } at notifyUser (line 839)
The code will not send an email if there is one check at epoch X, three more at X + 172780, then four at X + 172810 (7 checks in 30 seconds). Is this expected?

Now about the content:
I am not sure about the email sent. What do we expect the people on {{SITENAME}} to do? A sysadmin could filter it on the firewall, but not a sysop.
This is not able to detect attacks from several ips (eg. a botnet), although our throttling has the same limitation.
Could this have implications with WMF privacy policy? (for WMF sites)
Comment 10 Tyler Romeo 2011-07-17 18:21:17 UTC
Created attachment 8794 [details]
Addition of user notification functionality to user login.

Yeah, I really just took the checking for array stuff from the code for throttling since it seemed to work quite nicely. Anyway, I added the documentation, added the messages to messages.inc, changes 172800 to 2*24*3600 (the notation used in other parts of DefaultSettings.php), changed all to tabs, fixed the missing brace, and fixed the mentioned bug by using a third memcached key that keeps track of the last time a failed password attempt was made.

As for content, I removed the part about reporting the activity to SITENAME, because really there is nothing the user can do if somebody is making failed attempts to access their account. The only real action a user can take is to ensure his or her password is secure and whatnot.

In the case of a botnet attack, there is little that can be done other than just throttle individual IPs like is done in the current functionality. Throttling in general is a bad idea because then a botnet attack can be turned into a DoS attack against the user.

And sending the IP address in the email is not a violation of WMF privacy policy, primarily because the attacker is not (yet) logged in and thus not covered under the IP amnesty that registered users receive. Also, the privacy policy allows for the collection and use of a user's IP address if such use "may be used to identify the source(s) of the abusive behavior".
Comment 11 Sumana Harihareswara 2011-11-09 03:57:45 UTC
Tyler Romeo, thank you for the patch.  +need-review to signal to developers that this patch needs reviewing.
Comment 12 Platonides 2012-04-13 18:36:09 UTC
What about adding the missing i to make the Notifcation a Notification?

Still spaces instead of tabs at  $failedCount block, lines 574-583, 594-597, 603-625, 833-844, and messages.inc

Also, the code fails if the checks are done from multiple ips.

I think the approach should be something like:

On bad login:
 store[$USER] += (date, ip)

On good login:
 Show store[$USER]
 Empty store[$USER]

Daily:
 For each $USER:
   Email store[$USER] to $USER unless disabled in preferences
   Empty store[$USER]

I'm not keen on using a db table, though.
Comment 13 Sumana Harihareswara 2012-05-25 03:06:30 UTC
Marking patch as reviewed following Platonides's comments.

Tyler: Thanks again for the patch.  Are you interested in using developer access to directly suggest your revised patch into our Git source control system?

https://www.mediawiki.org/wiki/Developer_access

https://www.mediawiki.org/wiki/Git/Workflow#How_to_submit_a_patch
Comment 14 Tyler Romeo 2012-05-27 04:53:41 UTC
Sure, I'd be glad to. Of course I'd have to address Platonides' comments first and make adjustments to the patch (as well as test it more).
Comment 15 Tyler Romeo 2012-05-27 05:17:31 UTC
(In reply to comment #12)
> What about adding the missing i to make the Notifcation a Notification?
> 
> Still spaces instead of tabs at  $failedCount block, lines 574-583, 594-597,
> 603-625, 833-844, and messages.inc
> 
> Also, the code fails if the checks are done from multiple ips.
> 
> I think the approach should be something like:
> 
> On bad login:
>  store[$USER] += (date, ip)
> 
> On good login:
>  Show store[$USER]
>  Empty store[$USER]
> 
> Daily:
>  For each $USER:
>    Email store[$USER] to $USER unless disabled in preferences
>    Empty store[$USER]
> 
> I'm not keen on using a db table, though.

Actually, quick question. First of all, I like this approach a little better, because it'd be preferable to not be flooding users' emails. But I'm not sure on the whole daily email and empty idea, so here's my suggestion:

On bad login:
  store[$USER] += (date, ip)

On good login:
  Show store[$USER] where ip != curr_ip && abs(date - curr_date) < X

After X amount of time:
  Empty store[$USER]

Upon reaching threshold:
  Email store[$USER] unless disabled in prefs
  Empty store[$USER]

The first change is just a convenience because there's no need to tell a user about incorrect passwords made a few seconds ago at the same IP address they are logging in from (for obvious reasons).

The second change is because I think there's no need to worry users about people trying to login to their accounts unless it breaks a certain threshold. Under the daily approach, even just one incorrect login and at the end of the day the user gets a scary email warning them of breaches of security in their account.

In this method, users are always shown all incorrect logins upon their first successful login before the time period expires, but they're only notified by email if the incorrect logins are occurring often enough to be a problem.
Comment 16 Platonides 2012-05-28 21:57:26 UTC
I like it. I'd also add to the mix a Special page where you can view store[$YOURUSER], which could then be linked from the email (in case you want to manually verify if the guessing continues).

Additionally, with this setup we can store everything on $wgMemc.
Comment 17 Tyler Romeo 2012-05-31 04:19:44 UTC
OK, so here's my suggestion. I like the SpecialPage idea, but I'm a little weary about relying on memcached so much, mainly in the case of installations where the wiki is configured not to have any type of object caching. Also, like the rest of the world, I'm damn lazy and don't like writing a lot of code when we don't have to.

So here's my suggestion. Mediawiki already has a logging table...

After any login (good or bad):
    Log the login.

When logged in user browses any page:
    Show notification for bad logins (similar to new talk page
    message notification).

After X bad logins in Y amount of time w/o user logging in:
    Email user.

In this setup, the user is only emailed if they haven't logged in during the attacks, because with notifications there's no need to warn a user twice about the same thing. Also, all login attempts are stored permanently in Special:Log, which means the user can look at his authentication records whenever he/she wants. Users will be restricted to viewing only their own auth logs unless they have a special permission to view all auth logs. Also, there would be wiki configuration variables that can enable/disable logging good logins, enable/disable this feature altogether, enable/disable notifications, and set the email intervals.
Comment 18 Platonides 2012-06-08 22:44:49 UTC
(In reply to comment #17)
> OK, so here's my suggestion. I like the SpecialPage idea, but I'm a little
> weary about relying on memcached so much, mainly in the case of installations
> where the wiki is configured not to have any type of object caching. 

We refer to it as memcached, but $wgMemc can be backed by other things, including the DB. We could use a cache which defaults to CACHE_ANYTHING.


> Also, like
> the rest of the world, I'm damn lazy and don't like writing a lot of code when
> we don't have to.

:)

> So here's my suggestion. Mediawiki already has a logging table...
> 
> After any login (good or bad):
>     Log the login.
> 
> When logged in user browses any page:
>     Show notification for bad logins (similar to new talk page
>     message notification).
> 
> After X bad logins in Y amount of time w/o user logging in:
>     Email user.
>
> In this setup, the user is only emailed if they haven't logged in during the
> attacks, because with notifications there's no need to warn a user twice about
> the same thing. 

If it's not implemented as a SpecialPage or other separate action, they could be missing the notification, while the wiki counts it as "shown".
We could add a button to the special page to clear the store and thus reset the sending of an email.

> Also, all login attempts are stored permanently in Special:Log,
> which means the user can look at his authentication records whenever he/she
> wants. Users will be restricted to viewing only their own auth logs unless they
> have a special permission to view all auth logs. Also, there would be wiki
> configuration variables that can enable/disable logging good logins,
> enable/disable this feature altogether, enable/disable notifications, and set
> the email intervals.

WMF sites wouldn't want to permanently store the login attempts in the logs. Even if you only stored the bad ones.
Comment 19 Tyler Romeo 2012-06-08 23:16:06 UTC
(In reply to comment #18)
> (In reply to comment #17)
> > OK, so here's my suggestion. I like the SpecialPage idea, but I'm a little
> > weary about relying on memcached so much, mainly in the case of installations
> > where the wiki is configured not to have any type of object caching. 
> 
> We refer to it as memcached, but $wgMemc can be backed by other things,
> including the DB. We could use a cache which defaults to CACHE_ANYTHING.

Yeah, but it could also be CACHE_NONE. And $wgMainCacheType defaults to CACHE_NONE even though everything else is CACHE_ANYTHING. Furthermore, we have to keep in mind that memcached cannot be guaranteed, whereas a database entry is much more reliable.

The result can still be cached in memcached, but why only do that when we can also store it in user_newtalk like talk page messages are (and on that note, why aren't talk page notifications stored in memcached also?).

The solution I've been drafting would make user_newtalk a more general message storage table and enclose user messages in a UserMessage class, rather than cluttering User with five or so member functions. It would not increase request times because it would just become part of the same query for talk page messages.

> > So here's my suggestion. Mediawiki already has a logging table...
> > 
> > After any login (good or bad):
> >     Log the login.
> > 
> > When logged in user browses any page:
> >     Show notification for bad logins (similar to new talk page
> >     message notification).
> > 
> > After X bad logins in Y amount of time w/o user logging in:
> >     Email user.
> >
> > In this setup, the user is only emailed if they haven't logged in during the
> > attacks, because with notifications there's no need to warn a user twice about
> > the same thing. 
> 
> If it's not implemented as a SpecialPage or other separate action, they could
> be missing the notification, while the wiki counts it as "shown".
> We could add a button to the special page to clear the store and thus reset the
> sending of an email.

It would be part of Special:Log, specifically Special:Log/auth. The messaging would function exactly like new talk page messages, meaning the message is only wiped when the user visits the log (or presses a clear button if we want). As a database entry, there would be no way they could miss the notification.

> > Also, all login attempts are stored permanently in Special:Log,
> > which means the user can look at his authentication records whenever he/she
> > wants. Users will be restricted to viewing only their own auth logs unless they
> > have a special permission to view all auth logs. Also, there would be wiki
> > configuration variables that can enable/disable logging good logins,
> > enable/disable this feature altogether, enable/disable notifications, and set
> > the email intervals.
> 
> WMF sites wouldn't want to permanently store the login attempts in the logs.
> Even if you only stored the bad ones.

Why not? The only problem I can foresee is the fact that IP addresses of login attempts would be logged, in which case we can just have an option to disable IP logging. At that point, all that is being logged is a message saying somebody failed to login to a certain account, and even then this information could only be obtained by the user whose account it is (or the government/hackers if a court order/security breach is involved).

Overall, it is a much more theoretically reliable and secure idea to utilize MW's existing logging functionality to store authentication requests and their results rather then to cache them in memory pending the user's next visit. If a user doesn't log into Wikipedia for a week, and the threshold for email is also a week, then that memcached key is there for a whole seven days, which is generally not what memcached is used for (or at least not what I use it for).
Comment 20 Platonides 2012-06-10 20:38:11 UTC
> Why not?
http://wikimediafoundation.org/wiki/Privacy_policy#Details_of_data_retention


> Overall, it is a much more theoretically reliable and secure idea to utilize
> MW's existing logging functionality to store authentication requests and their
> results rather then to cache them in memory pending the user's next visit. If a
> user doesn't log into Wikipedia for a week, and the threshold for email is also
> a week, then that memcached key is there for a whole seven days, which is
> generally not what memcached is used for (or at least not what I use it for).

memcached at WMF is assumed to never lose entries due to memory pressure.
The use of that less-reliable method is on purpose :)
Comment 21 Tyler Romeo 2012-06-10 21:00:45 UTC
(In reply to comment #20)
> > Why not?
> http://wikimediafoundation.org/wiki/Privacy_policy#Details_of_data_retention

Like I said, just disable storage of IP addresses when logging authentication attempts and it will comply completely with WMF Privacy Policy.

> > Overall, it is a much more theoretically reliable and secure idea to utilize
> > MW's existing logging functionality to store authentication requests and their
> > results rather then to cache them in memory pending the user's next visit. If a
> > user doesn't log into Wikipedia for a week, and the threshold for email is also
> > a week, then that memcached key is there for a whole seven days, which is
> > generally not what memcached is used for (or at least not what I use it for).
> 
> memcached at WMF is assumed to never lose entries due to memory pressure.
> The use of that less-reliable method is on purpose :)

I understand, but why does user_newtalk exist then?
Comment 22 Platonides 2012-06-10 21:32:11 UTC
user_newtalk has existed since the first MediaWiki version, by Lee Daniel Crocker, before there were caching layers for mediawiki.
It could probably appear on either of those places without much problem.
Comment 23 Tyler Romeo 2012-06-10 23:08:11 UTC
(In reply to comment #22)
> user_newtalk has existed since the first MediaWiki version, by Lee Daniel
> Crocker, before there were caching layers for mediawiki.
> It could probably appear on either of those places without much problem.

I guess that's understandable, but I still don't believe using a memcached-only caching method is a reliable solution. It'd be like a CPU relying only on L1/2/3 cache and just not using regular memory whatsoever. Furthermore, I don't see any disadvantage in investing in a staged caching system, where the information is stored in memcached but also kept in the database in the case of a cache miss. Memcached is not meant for stuff like this. (I recommend reading this article for a better explanation: http://joped.com/2009/03/a-rant-about-proper-memcache-usage/)
Comment 24 Tyler Romeo 2012-06-11 18:49:32 UTC
Also, an additional question, why don't we also use $_SESSION? It seems that would be a lot faster than making a memcached request since it's stored as a variable in the PHP execution environment. There's probably a reason why MW doesn't do that, but I figured I'd ask.
Comment 25 Platonides 2012-06-12 22:43:25 UTC
$_SESSION wouldn't work for counting unsuccessful logins, as that is per user.
Comment 26 Tyler Romeo 2012-06-12 23:29:25 UTC
(In reply to comment #25)
> $_SESSION wouldn't work for counting unsuccessful logins, as that is per user.

That I know. I just meant in general. Grepping the MW source code for getSessionData shows it's only used for login tokens and storing usernames.

Anyway, I'm getting off topic. Mainly I just wanted to point out that there's no logical reason to rely entirely on Memcached, especially when the creators of Memcached tell you not to. As of now I have a working implementation of what I decribed earlier (including Memcached). If it's honestly that big a deal to not use the DB whatsoever then I'll trash it.
Comment 27 Tyler Romeo 2012-06-17 05:11:13 UTC
Created attachment 10762 [details]
New solution to bug

So here's what I have to offer: change I908109cf (patch also attached). It uses memcached but also stores in the database for reliability. It reworks the user notification system and adds invalid login notifications in the process. A number of configuration variables customize the process, including one that excludes logging of IP addresses.
Comment 28 Sumana Harihareswara 2012-06-17 11:10:42 UTC
Automated tests failed: https://integration.mediawiki.org/ci/job/MediaWiki-Tests-Misc/1976/console
Comment 29 Tyler Romeo 2012-06-17 16:19:54 UTC
Yeah I saw. My SQLite is a little rusty. Working on it now. Shouldn't take that long.
Comment 30 Tyler Romeo 2012-06-17 17:08:24 UTC
OK, now I don't know why it's failing.

Jenkins says no failures: https://integration.mediawiki.org/ci/job/MediaWiki-GIT-Fetching/2277/

Yet, it's still failed?
Comment 31 Alex Monk 2012-06-17 17:10:15 UTC
From https://integration.mediawiki.org/ci/job/MediaWiki-GIT-Fetching/2277/console:


17:06:31       [exec] Adding user_newtalk_type field to table user_newtalk...A database query syntax error has occurred.
17:06:31       [exec] The last attempted database query was:
17:06:31       [exec] "ALTER TABLE user_newtalk
17:06:31       [exec]  ADD user_msg_type INTEGER  NOT NULL default 0
17:06:31       [exec] "
17:06:31       [exec] from within function "DatabaseBase::sourceFile( /var/lib/jenkins/jobs/MediaWiki-GIT-Fetching/workspace/maintenance/sqlite/archives/patch-user_newtalk_type.sql )".
17:06:31       [exec] Database returned error "1: duplicate column name: user_msg_type"
17:06:31  
17:06:31  BUILD FAILED
17:06:31  /var/lib/jenkins/jobs/MediaWiki-GIT-Fetching/build.xml:65: exec returned: 1
Comment 32 Tyler Romeo 2012-06-17 17:14:40 UTC
Got it. Thanks for the help. This is my first encounter with Gerrit and Jenkins, so it's a bit confusing for me. Patch set 6 is verified.
Comment 33 Alex Monk 2012-06-17 17:16:31 UTC
I struggled to get my head around it as well. All you need to do is look at main console output, and the console output for each failing thing in the 'Downstream Builds' section.
Comment 34 Dereckson 2012-06-18 00:37:04 UTC
Code review: Gerrit change #11739
Comment 35 とある白い猫 2012-06-24 19:11:59 UTC
A few ideas:

Failed login IPs should be stored. It is nice to know someone is trying to steal a password but this is more helpful if attribution is possible.

I am not sure if privacy policy prevents revealing IPs of failed logins as this in my view falls under "Logged in users do not expose their IP address to the public except in cases of abuse" if we are going to count the user failing to log in as a logged in user.

A single failed login per account (per wiki) could be significant as SUL shares passwords for individual wikis and a quick way to fool such a system is attempting a different password once per wiki.

Even closed wikis should be subject to the same check as closed wikis may have the same password leftover from a SUL creation.

ALSO, it may be a good idea for checkusers and stewards to be able to see IPs where multiple failed login attempts are made to bulk number of accounts. We had a case on en.wikipedia where multiple admin accounts were stolen. People trying to steal multiple accounts at once isn't by any stretch of the imagination.
Comment 36 Tyler Romeo 2012-06-24 19:42:47 UTC
I think until the user has officially logged in successfully, there is no legal reason to not store their IP address as far as the privacy policy goes. And even when they are logged in, we would still be compliant with the policy so long as the information is not made public. Under the system I submitted with my patch, all logged in users would only be able to view their own login logs, with the exception of those with special permissions, which I'd imagine in WMF's case would be given to checkusers.

As far as SUL goes, that would have to be a whole different implementation. SUL in WMF is implemented using CentralAuth, which is separate from the core.

Furthermore, I think this bug should wait until the Echo notifications system is created. No need to add additional notifications logic when an entirely new system is about to be phased in.
Comment 37 Platonides 2012-06-24 20:32:06 UTC
Taking all wikis into account at once would be easy... if it wasn't for not-merged accounts. We wouldn't want zh:Foo to see the IP information related to en:Foo failed logins if they are not both in the same SUL account.
Comment 38 Tyler Romeo 2012-06-24 22:54:24 UTC
Yeah, but the primary issue is that we'd want to implement a core mechanism of invalid login notifications, and once that is implemented, edit the CentralAuth extension to work with it (or have CentralAuth have its own invalid notifications system altogether). Regardless, with the proposed centralized notifications system, it would be a lot easier implementing this.
Comment 39 とある白い猫 2012-06-25 20:26:03 UTC
I think the solution could be simple.

Just email the user the IP that attempted to make a bad login and let the info be visible from preferences, and store info on database in a manner checkusers can see.

If the account is merged, make the log for all wikis be available centrally.

I believe CentralAuth still uses at least one local login, right? Is it possible to login to CentralAuth without logging in some place locally?

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links