Page MenuHomePhabricator

OAuth /identify endpoint does not check whether consumer is approved
Closed, ResolvedPublic

Description

Steps to reproduce (on vagrant with oauth role on):

  • visit oauth hello world app
  • authorize
  • disable hello world app
  • verify identity

Event Timeline

Tgr created this task.Oct 19 2016, 1:35 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 19 2016, 1:35 AM
Tgr added a comment.Oct 19 2016, 1:42 AM

Also, the API disallows actions when the user is blocked and $wgBlockDisablesLogin is on, but /identify does not.

Tgr added a comment.Oct 19 2016, 2:48 AM

Patch:

Anomie added a subscriber: Anomie.Oct 19 2016, 1:58 PM

Patch:

-					if ( !$localUser || !$localUser->isLoggedIn() ) {
+					if ( !$localUser || !$localUser->isLoggedIn() || $localUser->isLocked() ) {
 						throw new MWOAuthException( 'mwoauth-invalid-authorization-invalid-user' );

Instead of throwing 'mwoauth-invalid-authorization-invalid-user' for the added condition, it would make more sense to throw 'mwoauth-invalid-authorization-blocked-user'. Also, you're not checking the $wgBlockDisablesLogin && ->isBlocked() case.

Tgr added a comment.Oct 19 2016, 10:42 PM

Fixed:


(Blocked users should be handled in the auth layer, as in T129738, but no harm in double-checking.)

Fixed:

Looks good, +1.

@Tgr, I will can deploy this on Monday if you've not already done so.

dpatrick closed this task as Resolved.Oct 24 2016, 11:08 PM
dpatrick claimed this task.

23:07 dapatrick: Deployed patch for T148600 to wmf22

You guys can go ahead and announce and release this now.

Tgr reopened this task as Open.Oct 25 2016, 12:16 AM
Tgr removed dpatrick as the assignee of this task.
Tgr added a subscriber: demon.

Thanks! (And sorry for forgetting to respond. I wanted to deploy it and then got distracted.)
Let's keep it open until it's merged in master (and the task can be made public), though.

@demon do you want to make a proper security release for this? IMO it is significant enough to warrant a backport (/identify was added in 1.23 so that means 1.23, 1.26 and 1.27) but not severe enough for the whole security release process with the pre-announcement and whatnot. If the old branches will also get new releases when 1.28 is released, IMO we can just merge to master and backport and be done with it.

Thanks! (And sorry for forgetting to respond. I wanted to deploy it and then got distracted.)
Let's keep it open until it's merged in master (and the task can be made public), though.
@demon do you want to make a proper security release for this? IMO it is significant enough to warrant a backport (/identify was added in 1.23 so that means 1.23, 1.26 and 1.27) but not severe enough for the whole security release process with the pre-announcement and whatnot. If the old branches will also get new releases when 1.28 is released, IMO we can just merge to master and backport and be done with it.

Since it's not bundled I say let's do that. Push to master + all supported branches and send an announcement to the usual places.

Tgr closed this task as Resolved.Oct 25 2016, 1:18 AM
Tgr claimed this task.
Tgr changed the visibility from "Custom Policy" to "Public (No Login Required)".
Tgr changed Security from Software security bug to None.

Merged as https://gerrit.wikimedia.org/r/#/c/317733/ (I probably should have made the task public first) and backported.

Change 318228 had a related patch set uploaded (by Gergő Tisza):
SECURITY: check stage and user blocked/locked status in /identify

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

Change 318228 merged by jenkins-bot:
SECURITY: check stage and user blocked/locked status in /identify

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

Tgr added a comment.Oct 27 2016, 11:13 PM

A mistake in the patch caused T149194.