Old tokens are remaining valid within a new session
Closed, ResolvedPublic

Description

<unicornisaurous>: [...] It semes to me that the userrights token is not looked at closely by the API, and that the API only looks at the cookie. If I obtain a user rights token while logged in as one user, and then log out, my token now shows as a “bad token”, but after logging in again as a different user, the same token works again, but now the API thinks I’m the newly logged in user. Is this behavior intentional, or is something going wrong?

<anomie>: Something is possibly strange with that, but it's not in the API and not specific to the userrights token. What's going on is that the new login isn't establishing a new "secret" in the session, it's just keeping the existing secret so the old tokens are remaining valid. The best thing to do would probably be to file a security bug about it in Phabricator (see https://www.mediawiki.org/wiki/Reporting_security_bugs)

Steps to reproduce:

  1. Give the 'sysop' group the ability to add/remove the bot group from other users:
$wgAddGroups['sysop'][] = 'bot';
$wgRemoveGroups['sysop'][] = 'bot';
  1. Login as a user which is only in the 'sysop' group
  2. Get a userrights token from api.php?action=query&meta=tokens&type=userrights&format=json (Notice that I did not specify any specific users as described here, but that may be outdated documentation)
  3. Make a POST request (with token) to api.php?action=userrights&user=someUser&add=bot&format=json (Succeeds as expected)
  4. Logout and try to make the same request with the same token (Fails with Bad token error)
  5. Log in as a different 'sysop' user and try the same request, except changing 'add' to 'remove' to reverse the changes made in #4 (Succeeds)

Although I only obtained one userrights token for the whole test, in the log both of my sysop users were logged as making userrights changes during the respective times they were logged in.

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Dec 21 2015, 4:19 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 21 2015, 4:19 PM
Unicornisaurous updated the task description. (Show Details)
Unicornisaurous changed Security from None to Software security bug.
Unicornisaurous edited subscribers, added: Unicornisaurous, Anomie; removed: Aklapper.
Anomie renamed this task from Old userrights tokens are remaining valid within a new session to Old tokens are remaining valid within a new session.Dec 21 2015, 4:31 PM
Anomie updated the task description. (Show Details)

An easier test would be to use API action=checktoken; this illustrates that the issue isn't limited to the userrights token. The code there is also easier to examine to determine that it's not an API issue either.

csteipp triaged this task as High priority.Dec 21 2015, 4:50 PM
csteipp added a subscriber: csteipp.

This issue has confused me greatly - what good are tokens if the session cookie is whats's being looked at anyways in terms of identifying the user?

This issue has confused me greatly - what good are tokens if the session cookie is whats's being looked at anyways in terms of identifying the user?

The point of the tokens is to prevent cross-site request forgery. You can read the linked article for details, but in short: If we only look at the session cookie, some evil third-party site could embed JavaScript to delete pages and take other admin actions via the API in their page, and then somehow trick wiki admins to visit that page. The CSRF token mechanism prevents that by requiring that the posted request data include the token which the evil third-party site has no way to obtain.

The secret value that's involved in constructing the token is stored in the user's session, since that's an easy and performant way of storing an arbitrary secret value. We could fix this specific bug by just adding the user name to the data used to construct the token, but I suspect we'll prefer to actually change the token secret when the new session is constructed.

This issue has confused me greatly - what good are tokens if the session cookie is whats's being looked at anyways in terms of identifying the user?

The point of the tokens is to prevent cross-site request forgery. You can read the linked article for details, but in short: If we only look at the session cookie, some evil third-party site could embed JavaScript to delete pages and take other admin actions via the API in their page, and then somehow trick wiki admins to visit that page. The CSRF token mechanism prevents that by requiring that the posted request data include the token which the evil third-party site has no way to obtain.

The secret value that's involved in constructing the token is stored in the user's session, since that's an easy and performant way of storing an arbitrary secret value. We could fix this specific bug by just adding the user name to the data used to construct the token, but I suspect we'll prefer to actually change the token secret when the new session is constructed.

Sure, makes sense. I guess I was expecting a scheme like Oauth (which there happens to be an extension for) in which once the token was obtained, it is all that is needed to authenticate.

Here's a simple patch which, from my initial tests, seems to fix the problem. It just sets the wsEditToken session data to an empty string on logout (similar to how wsUserID is set to 0). I don't know if this is the only session data that needs to be reset, or whether setting this on logout is the best place to put this.

Here's a simple patch which, from my initial tests, seems to fix the problem. It just sets the wsEditToken session data to an empty string on logout

You fetched a token, logged out, then logged back in and saw the first token didn't work anymore. But try fetching a second token, then logging out a second time and logging back in.

Then delete your session entirely, log in, log out, log back in, and try that second token I had you fetch again.

Unicornisaurous added a comment.EditedDec 21 2015, 10:37 PM

Here's a simple patch which, from my initial tests, seems to fix the problem. It just sets the wsEditToken session data to an empty string on logout

You fetched a token, logged out, then logged back in and saw the first token didn't work anymore. But try fetching a second token, then logging out a second time and logging back in.

Then delete your session entirely, log in, log out, log back in, and try that second token I had you fetch again.

Looks like I forgot to check for an empty string...this one seems to fix the issue you described. Not convinced logout() is the right place for this though.

Edit: Changed == to ===

Bawolff added a subscriber: Bawolff.EditedDec 23 2015, 9:31 AM

Here's a simple patch which, from my initial tests, seems to fix the problem. It just sets the wsEditToken session data to an empty string on logout

You fetched a token, logged out, then logged back in and saw the first token didn't work anymore. But try fetching a second token, then logging out a second time and logging back in.

Then delete your session entirely, log in, log out, log back in, and try that second token I had you fetch again.

Looks like I forgot to check for an empty string...this one seems to fix the issue you described. Not convinced logout() is the right place for this though.

That's a really good catch.

I believe that this is still trigerable, if instead of logging out, you directly go to Special:Userlogin and login as a different user (without logging out in between).

I think wfResetSessionID() should not copy over wsEditToken (And possibly other stuff stored in session). Also wfResetSessionID() should be called during log out

I think we should blank/refresh the token on login as well as logout. I'm not sure if wfResetSessionID() is the right place to put it, maybe something in LoginForm directly, or a have LoginForm call something on the User object to have the User object reset the edit token.

This will all get shuffled around with SessionManager in a few weeks, so the exact place we put the code isn't too important right now.

Proposed patch:

Proposed patch:

Patch looks good to me, and I'm not seeing it affect negatively in dev. I'll deploy this soon.

The patch no longer applies since SessionManager was merged. In making and testing a replacement, I noticed that ApiLogin isn't resetting the session ID at all (because LoginForm does it in processLogin() rather than authenticateUserData()), so I fixed that too.

This patch is based on top of https://gerrit.wikimedia.org/r/264309, which lets us reset all CSRF tokens instead of just the edit token.

The patch no longer applies since SessionManager was merged. In making and testing a replacement, I noticed that ApiLogin isn't resetting the session ID at all (because LoginForm does it in processLogin() rather than authenticateUserData()), so I fixed that too.

This patch is based on top of https://gerrit.wikimedia.org/r/264309, which lets us reset all CSRF tokens instead of just the edit token.

Typo in SpecialUserLogin.php:

// Always make sure edit token is regenerated. (T114419)

I'm pretty sure you meant this task and not T114419: Event on "Make code review not suck"

The patch no longer applies since SessionManager was merged. In making and testing a replacement, I noticed that ApiLogin isn't resetting the session ID at all (because LoginForm does it in processLogin() rather than authenticateUserData()), so I fixed that too.

This patch is based on top of https://gerrit.wikimedia.org/r/264309, which lets us reset all CSRF tokens instead of just the edit token.

Typo in SpecialUserLogin.php:

// Always make sure edit token is regenerated. (T114419)

I'm pretty sure you meant this task and not T114419: Event on "Make code review not suck"

Ha, copy-pasted from the old patch.

MaxSem added a parent task: Restricted Task.Mar 2 2016, 11:01 PM

I can go ahead an deploy this patch, but I'm not able to reproduce the original issue in my local dev environments, or beta currently. Was this issue fixed by another patch?

It looks like wsTokenSecrets is removed on logout, even without resetToken/resetAllTokens being called. Although I'm not seeing where that's happening.

Anomie added a comment.Mar 8 2016, 3:58 PM

It was probably fixed by rMW6d4436c9156a: Unpersist the session on logout removing the whole session on logout. wsTokenSecrets will still be present in the session data for the rest of the request, but since the session is not persisted anymore it'll be thrown away once the request completes. Unless, of course, something else decides to re-persist it for some reason.

That makes sense, thanks for the background. I think we should do it, just in case something decides to re-persist the session.

I'll go ahead and deploy this as a security patch.

17:40 csteipp: deployed patch for T122056

Did this issue only affect 1.27, or do we need to backport anything to 1.26?

Anomie added a comment.Mar 9 2016, 7:47 PM

It appears it is present in 1.26. @Bawolff's

looks good for 1.26 and earlier, as long as you adjust for the includes/User.phpincludes/user/User.php rename that happened in 1.27.

csteipp closed this task as Resolved.May 5 2016, 6:03 PM
csteipp claimed this task.

This has been deployed for a while, and backports have been added for the tarball release.

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".May 20 2016, 5:25 PM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.
Restricted Application added a subscriber: Malyacko. · View Herald TranscriptMay 20 2016, 5:25 PM

Change 289889 had a related patch set uploaded (by Chad):
Reset wsEditToken on login

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

Change 289889 merged by jenkins-bot:
Reset all tokens on login

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

Change 292058 had a related patch set uploaded (by Chad):
Reset all tokens on login

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

Change 292059 had a related patch set uploaded (by Chad):
Reset all tokens on login

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

Change 292060 had a related patch set uploaded (by Chad):
Reset all tokens on login

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

Change 292058 merged by jenkins-bot:
Reset all tokens on login

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

Change 292059 merged by jenkins-bot:
Reset all tokens on login

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

Change 292060 merged by jenkins-bot:
Reset all tokens on login

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