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:

Give the 'sysop' group the ability to add/remove the bot group from other users:

$wgAddGroups['sysop'][] = 'bot';
$wgRemoveGroups['sysop'][] = 'bot';

Login as a user which is only in the 'sysop' group

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)

Make a POST request (with token) to api.php?action=userrights&user=someUser&add=bot&format=json (Succeeds as expected)

Logout and try to make the same request with the same token (Fails with Bad token error)

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.

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.

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.

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.

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.

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.

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.

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.