Last modified: 2014-09-23 23:06:12 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.
Gah, never mind. *** This bug has been marked as a duplicate of 9836 ***
Reopening since bug 9836 has been RESOLVED FIXED due to implementation of captcha. Repurposing that to be a bit more specific.
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.
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.
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.
*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*
En.wp community support here: https://secure.wikimedia.org/wikipedia/en/wiki/Wikipedia:Village_pump_%28proposals%29/Account_security#Notify_users_via_email_when_someone_tries_to_log_into_their_account_with_the_wrong_password
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.
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)
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".
Tyler Romeo, thank you for the patch. +need-review to signal to developers that this patch needs reviewing.
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.
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
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).
(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.
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.
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.
(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.
(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).
> 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 :)
(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?
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.
(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/)
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.
$_SESSION wouldn't work for counting unsuccessful logins, as that is per user.
(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.
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.
Automated tests failed: https://integration.mediawiki.org/ci/job/MediaWiki-Tests-Misc/1976/console
Yeah I saw. My SQLite is a little rusty. Working on it now. Shouldn't take that long.
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?
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
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.
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.
Code review: Gerrit change #11739
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.
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.
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.
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.
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?
