Page MenuHomePhabricator

Track ip addresses associated with a session and log when anomalous
Closed, DeclinedPublic

Description

When a new session is created, track the ip address that the request was received from. When an existing session is accessed check to see if the requesting IP has been previously associated with the session. If more than N distinct IPs are seen to have been using the same session in X units of time, write a debug log event recording the anomaly (e.g. "Session 'deadbeef' hit from 6 IPs in 10 minutes: 0.0.0.0, 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5"). The log events should be sent to a distinct channel that only tracks these events so that they do not pollute other logs and can be managed securely as they will contain PII (IPs) and sensitive data (session id).

N and X should be configurable with some sort of sane sounding defaults like N=5 & X = 10 minutes.

Implementation could be to log on every access of the session but vary the event log level based on thresholds as well with debug for C == 1, info for 1 < C < N, and warning for C >= N.

Related Objects

Event Timeline

bd808 raised the priority of this task from to High.
bd808 updated the task description. (Show Details)
bd808 added subscribers: bd808, Anomie, Tgr, csteipp.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 1 2016, 10:23 PM

Change 268972 had a related patch set uploaded (by Gergő Tisza):
[WIP] Log multiple IPs using the same session or the same user account

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

Change 268972 merged by jenkins-bot:
Log multiple IPs using the same session or the same user account

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

Tgr added a comment.Feb 12 2016, 3:24 AM

Tested in beta, works as expected.

Change 270240 had a related patch set uploaded (by BryanDavis):
Log multiple IPs using the same session or the same user account

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

Change 270240 merged by jenkins-bot:
Log multiple IPs using the same session or the same user account

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

Mentioned in SAL [2016-02-12T19:15:12Z] <bd808> Synced files for T125455 in wrong order; broke all wikis

Mentioned in SAL [2016-02-12T19:15:44Z] <bd808@mira> Synchronized php-1.27.0-wmf.13/includes/session/SessionManager.php: Log multiple IPs using the same session or the same user account (4d8b8ca) (T125455) (duration: 01m 17s)

Change 270359 had a related patch set uploaded (by Gergő Tisza):
Send log messages from session-ip channel to logstash

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

Change 270359 merged by jenkins-bot:
Send log messages from session-ip channel to logstash

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

Mentioned in SAL [2016-02-12T21:30:42Z] <tgr@mira> Synchronized wmf-config/InitialiseSettings.php: T125455: log session-ip channel to logstash (duration: 01m 17s)

Mentioned in SAL [2016-02-12T19:15:12Z] <bd808> Synced files for T125455 in wrong order; broke all wikis

The incident report for this mistake is at https://wikitech.wikimedia.org/wiki/Incident_documentation/20160212-AllWikisOutage

ori added a subscriber: ori.Feb 23 2016, 6:09 AM

Discussed this briefly with @Tgr. I don't think we should keep the current implementation. It won't help us spot accounts that have been compromised, because an attacker is not likely to use enough IPs to set off the warning. When one of my e-mail accounts was compromised last year, the attacker used Tor, which caused my account to be accessed from a half-dozen IPs over the course of several days. This rate of change is equivalent to what you would see if a person was traveling.

The logging added in f22549a605 would be primarily useful for detecting a major failure of the authentication system. If that were to happen again -- which I think is unlikely -- we would hear about it.

I do think it would be useful to provide users with a dashboard which shows them recent IPs that had been used to access their account, but I think it would be easier to implement something like that from scratch than adapt the code in f22549a605 for that purpose.

So: it's a lot of complicated logic and a round-trip to Redis on every request. I propose reverting it, now that the session pollution issue has been resolved.

Tgr added a comment.Feb 23 2016, 11:13 AM

The logging added in f22549a605 would be primarily useful for detecting a major failure of the authentication system. If that were to happen again -- which I think is unlikely -- we would hear about it.

Indeed, the main purpose was detecting something like a cached authentication cookie that affects lots of people. We were unsure how likely it would be that we hear about it, given that the past two records were from super power users - that might indicate it's a frequent issue but only reported by people with a good understand of tech issues and how they are handled. (Or it could indicate that there is something in the power user or global usage pattern that triggers the bug.)

I do think it would be useful to provide users with a dashboard which shows them recent IPs that had been used to access their account, but I think it would be easier to implement something like that from scratch than adapt the code in f22549a605 for that purpose.

That's T107707 (sort of) but that would require very different implementation. The IP logging code was always meant to be temporary.

So: it's a lot of complicated logic and a round-trip to Redis on every request. I propose reverting it, now that the session pollution issue has been resolved.

Apparently not :( T126069#2054327

ori added a comment.Feb 23 2016, 7:08 PM

The logging added in f22549a605 would be primarily useful for detecting a major failure of the authentication system. If that were to happen again -- which I think is unlikely -- we would hear about it.

Indeed, the main purpose was detecting something like a cached authentication cookie that affects lots of people. We were unsure how likely it would be that we hear about it, given that the past two records were from super power users - that might indicate it's a frequent issue but only reported by people with a good understand of tech issues and how they are handled. (Or it could indicate that there is something in the power user or global usage pattern that triggers the bug.)

I do think it would be useful to provide users with a dashboard which shows them recent IPs that had been used to access their account, but I think it would be easier to implement something like that from scratch than adapt the code in f22549a605 for that purpose.

That's T107707 (sort of) but that would require very different implementation. The IP logging code was always meant to be temporary.

So: it's a lot of complicated logic and a round-trip to Redis on every request. I propose reverting it, now that the session pollution issue has been resolved.

Apparently not :( T126069#2054327

Well, the logging did not help us in this case, right? Is there even an indication that the account had been accessed from enough IPs to hit the threshold?

Change 272795 had a related patch set uploaded (by Ori.livneh):
Revert "Log multiple IPs using the same session or the same user account"

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

Change 272795 merged by jenkins-bot:
Revert "Log multiple IPs using the same session or the same user account"

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

Tgr closed this task as Declined.Feb 24 2016, 1:13 AM

Wasn't all that useful, might revisit later.