Page MenuHomePhabricator

[Bug] Oauth returns fatal when cancelling approval
Closed, ResolvedPublic

Description

How to reproduce:

  1. go to https://www.mediawiki.org/wiki/Special:OAuth/authorize
  2. cancel the authorization
  3. get:

[V-TudgpAEK0AADMqtE8AAACF] 2016-10-05 12:13:42: Fatal error "BadMethodCallException"

Full log line:

OAuth/frontend/specialpages/SpecialMWOAuth.php:225 Call to a member function get() on a non-object (boolean)

Event Timeline

Matanya created this task.Oct 5 2016, 12:31 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 5 2016, 12:31 PM
greg added a subscriber: greg.

no data loss, user already wanted to cancel the operation, not a blocker

demon added a subscriber: demon.Oct 5 2016, 6:46 PM

Here's the relevant code:

$cmr = MWOAuthDAOAccessControl::wrap(
	MWOAuthConsumer::newFromKey( $dbr, $consumerKey ),
	$this->getContext()
);

$this->getOutput()->addSubtitle( $this->msg( 'mwoauth-desc' )->escaped() );
$this->getOutput()->addWikiMsg(
	'mwoauth-acceptance-cancelled',
	$cmr->get( 'name' )
);

Happens easily when MWOAuthDAOAccessControl::wrap() returns null/false, which is the second condition it can do. $cmr ends up not being an object, which we clearly can't call get() on.

Should probably show an alternative message in that case, or clean up the calling so it won't return null/false

demon triaged this task as High priority.Oct 5 2016, 6:47 PM
Tgr set Security to Software security bug.Oct 5 2016, 7:03 PM
Tgr added a project: Security.
Tgr changed the visibility from "Public (No Login Required)" to "Custom Policy".
Tgr added a subscriber: Tgr.

The user should never be presented with an authorization dialog since the URL does not specify a consumer. Yet somehow we present a dialog for a specific consumer (Hello World, which is the first DB record, I think). Let's call this a security issue until we know what's going on (the borked part seems to be MWOAuthServer::getConsumerKey).

Tgr added a comment.Oct 5 2016, 7:25 PM
tgr@terbium:~$ mwscript eval.php --wiki=mediawikiwiki
> var_dump( $db->selectRow( 'oauth_registered_consumer', [ 'oarc_name' ], [ 'oarc_consumer_key' => false ] ) );
object(stdClass)#221 (1) {
  ["oarc_name"]=>
  string(38) "MWOAuth Hello World (end user version)"
}
> echo $db->selectSQLText( 'oauth_registered_consumer', [ 'oarc_name' ], [ 'oarc_consumer_key' => false ] );
SELECT  oarc_name  FROM `oauth_registered_consumer`    WHERE oarc_consumer_key = 0

Huh.

Tgr added a comment.Oct 5 2016, 7:31 PM

I guess MySQL does a string => int transform on the consumer key so it will just find the first key which does not start with a number? Crazy stuff. I don't think there is any security impact though.

Anomie added a subscriber: Anomie.Oct 5 2016, 7:35 PM

Apparently MySQL has weird type casting like PHP, such that comparing a string and an integer casts the string to an integer with stupid casting rules. Sigh.

This started happening here thanks to a fix for a different bug causing boolean false to be turned into 0 rather than empty-string by Database. Apparently this code was depending on the former behavior to not screw up when MWOAuthConsumer::newFromKey() gets passed false for $key. :/

Tgr added a comment.Oct 5 2016, 7:40 PM

This started happening here thanks to a fix for a different bug causing boolean false to be turned into 0 rather than empty-string by Database.

Should that maybe be changed to '0' to avoid similar issues elsewhere? For int fields that will only result in an unnecessary typecast, for string matches it would behave significantly more sanely.

Anomie added a comment.EditedOct 5 2016, 7:45 PM

Anyway,

Tgr added a comment.EditedOct 5 2016, 8:14 PM

I took a slightly different approach (and then got distracted waiting for vagrant to update and did not upload it):

I think this task can be unsecuritized (there is no way to use the bug to get around any authorization check) but waiting for a second opinion with that.

Anomie added a comment.EditedOct 5 2016, 8:23 PM
+			throw new \ErrorPageError( 'mwoauth-error', 'mwoauth-invalid-consumer-key' );

Just throw an MWOAuthException( 'mwoauth-invalid-consumer-key' ) and let the catch in
SpecialMWOAuth::execute() handle it.

Tgr added a comment.Oct 5 2016, 8:30 PM

Just throw an MWOAuthException( 'mwoauth-invalid-consumer-key' ) and let the catch in
SpecialMWOAuth::execute() handle it.

Thanks, fixed.

Tgr added a comment.Oct 6 2016, 1:18 AM

Filed T147537 about the generic issue.

Anomie added a comment.Oct 6 2016, 3:04 PM

I took a slightly different approach (and then got distracted waiting for vagrant to update and did not upload it):

Looks good to me.

With T147537 resolved, this can probably be made public, and the patch above merged in Gerrit? (It looks like the original issue here is no longer a problem, but the patch has some nice code quality improvements.)

Tgr removed a project: Security.Oct 11 2016, 7:56 PM
Tgr changed the visibility from "Custom Policy" to "Public (No Login Required)".
Tgr changed Security from Software security bug to None.

Change 315346 had a related patch set uploaded (by Gergő Tisza):
Fix bugs with handling of missing request parameters

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

Change 315346 merged by jenkins-bot:
Fix bugs with handling of missing request parameters

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

Anomie closed this task as Resolved.Oct 12 2016, 4:33 PM
Anomie assigned this task to Tgr.