From 41417a401a15df1b6a7993a4669e0ec95aceda5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Tisza?= Date: Wed, 5 Oct 2016 19:35:31 +0000 Subject: [PATCH] SECURITY: Fix bugs with handling of missing request parameters * make MWOAuthDAO methods type-safe in the face of MySQL non-strict mode weirdness * throw an exception when cancelled with an invalid consumer key (should not happen under normal circumstances as the authorization dialog won't display) * fix phpdoc so it's easier to follow in an IDE what happens Bug: T147414 Change-Id: Ibb938ccb9bfae6c52444f7676dc475f6a3024cd8 --- backend/MWOAuthConsumer.php | 8 ++++++-- backend/MWOAuthConsumerAcceptance.php | 7 ++++--- backend/MWOAuthDAO.php | 2 +- backend/MWOAuthDataStore.php | 6 +++--- backend/MWOAuthServer.php | 5 ++++- frontend/specialpages/SpecialMWOAuth.php | 3 +++ 6 files changed, 21 insertions(+), 10 deletions(-) diff --git a/backend/MWOAuthConsumer.php b/backend/MWOAuthConsumer.php index 2289888..d63fde3 100644 --- a/backend/MWOAuthConsumer.php +++ b/backend/MWOAuthConsumer.php @@ -163,7 +163,7 @@ class MWOAuthConsumer extends MWOAuthDAO { public static function newFromKey( \DBConnRef $db, $key, $flags = 0 ) { $row = $db->selectRow( static::getTable(), array_values( static::getFieldColumnMap() ), - array( 'oarc_consumer_key' => $key ), + array( 'oarc_consumer_key' => (string)$key ), __METHOD__, ( $flags & self::READ_LOCKING ) ? array( 'FOR UPDATE' ) : array() ); @@ -190,7 +190,11 @@ class MWOAuthConsumer extends MWOAuthDAO { ) { $row = $db->selectRow( static::getTable(), array_values( static::getFieldColumnMap() ), - array( 'oarc_name' => $name, 'oarc_version' => $version, 'oarc_user_id' => $userId ), + array( + 'oarc_name' => (string)$name, + 'oarc_version' => (string)$version, + 'oarc_user_id' => (int)$userId + ), __METHOD__, ( $flags & self::READ_LOCKING ) ? array( 'FOR UPDATE' ) : array() ); diff --git a/backend/MWOAuthConsumerAcceptance.php b/backend/MWOAuthConsumerAcceptance.php index 1851278..474f992 100644 --- a/backend/MWOAuthConsumerAcceptance.php +++ b/backend/MWOAuthConsumerAcceptance.php @@ -81,7 +81,7 @@ class MWOAuthConsumerAcceptance extends MWOAuthDAO { public static function newFromToken( \DBConnRef $db, $token, $flags = 0 ) { $row = $db->selectRow( static::getTable(), array_values( static::getFieldColumnMap() ), - array( 'oaac_access_token' => $token ), + array( 'oaac_access_token' => (string)$token ), __METHOD__, ( $flags & self::READ_LOCKING ) ? array( 'FOR UPDATE' ) : array() ); @@ -108,9 +108,10 @@ class MWOAuthConsumerAcceptance extends MWOAuthDAO { ) { $row = $db->selectRow( static::getTable(), array_values( static::getFieldColumnMap() ), - array( 'oaac_user_id' => $userId, + array( + 'oaac_user_id' => (int)$userId, 'oaac_consumer_id' => $consumer->get( 'id' ), - 'oaac_wiki' => $wiki + 'oaac_wiki' => (string)$wiki ), __METHOD__, ( $flags & self::READ_LOCKING ) ? array( 'FOR UPDATE' ) : array() diff --git a/backend/MWOAuthDAO.php b/backend/MWOAuthDAO.php index 6b3d76f..62683e9 100644 --- a/backend/MWOAuthDAO.php +++ b/backend/MWOAuthDAO.php @@ -75,7 +75,7 @@ abstract class MWOAuthDAO implements \IDBAccessObject { final public static function newFromId( \DBConnRef $db, $id, $flags = 0 ) { $row = $db->selectRow( static::getTable(), array_values( static::getFieldColumnMap() ), - array( static::getIdColumn() => $id ), + array( static::getIdColumn() => (int)$id ), __METHOD__, ( $flags & self::READ_LOCKING ) ? array( 'FOR UPDATE' ) : array() ); diff --git a/backend/MWOAuthDataStore.php b/backend/MWOAuthDataStore.php index 2dd2d81..76de10f 100644 --- a/backend/MWOAuthDataStore.php +++ b/backend/MWOAuthDataStore.php @@ -149,7 +149,7 @@ class MWOAuthDataStore extends OAuthDataStore { * Return a consumer key associated with the given request token. * * @param MWOAuthToken $requestToken the request token - * @return String the consumer key + * @return string|false the consumer key or false if nothing is stored for the request token */ public function getConsumerKey( $requestToken ) { $cacheKey = MWOAuthUtils::getCacheKey( 'consumer', 'request', $requestToken ); @@ -163,10 +163,10 @@ class MWOAuthDataStore extends OAuthDataStore { * A stored callback URL parameter is deleted from the cache once read for the first * time. * - * @param string the consumer key + * @param string $consumerKey the consumer key * @param string $requestKey original request key from /initiate * @throws MWOAuthException - * @return String the stored callback URL parameter + * @return string|false the stored callback URL parameter */ public function getCallbackUrl( $consumerKey, $requestKey ) { $cacheKey = MWOAuthUtils::getCacheKey( 'callback', $consumerKey, 'request', $requestKey ); diff --git a/backend/MWOAuthServer.php b/backend/MWOAuthServer.php index fa178cc..e643ebf 100644 --- a/backend/MWOAuthServer.php +++ b/backend/MWOAuthServer.php @@ -3,11 +3,14 @@ namespace MediaWiki\Extensions\OAuth; class MWOAuthServer extends OAuthServer { + /** @var MWOAuthDataStore */ + protected $data_store; + /** * Return a consumer key associated with the given request token. * * @param MWOAuthToken $requestToken the request token - * @return String the consumer key + * @return string|false the consumer key or false if nothing is stored for the request token */ public function getConsumerKey( $requestToken ) { return $this->data_store->getConsumerKey( $requestToken ); diff --git a/frontend/specialpages/SpecialMWOAuth.php b/frontend/specialpages/SpecialMWOAuth.php index 0397987..58da23c 100644 --- a/frontend/specialpages/SpecialMWOAuth.php +++ b/frontend/specialpages/SpecialMWOAuth.php @@ -218,6 +218,9 @@ class SpecialMWOAuth extends \UnlistedSpecialPage { MWOAuthConsumer::newFromKey( $dbr, $consumerKey ), $this->getContext() ); + if ( !$cmr ) { + throw new \ErrorPageError( 'mwoauth-error', 'mwoauth-invalid-consumer-key' ); + } $this->getOutput()->addSubtitle( $this->msg( 'mwoauth-desc' )->escaped() ); $this->getOutput()->addWikiMsg( -- 1.9.1