Page MenuHomePhabricator

0001-SECURITY-Move-UserGetRights-call-before-application-.patch

Authored By
Anomie
Jul 14 2016, 5:20 PM
Size
3 KB
Referenced Files
None
Subscribers
None

0001-SECURITY-Move-UserGetRights-call-before-application-.patch

From 6099940734a922dc4fcea57806274aa89ae4ac45 Mon Sep 17 00:00:00 2001
From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Thu, 7 Jul 2016 17:24:50 -0400
Subject: [PATCH] SECURITY: Move 'UserGetRights' call before application of
Session::getAllowedUserRights()
This prevents hook functions from accidentally adding rights that should
be denied based on the session grants.
If some extension really needs to be able to override session grants,
add a new hook where the old call was, with documentation explicitly
warning about the security implications.
Bug: T139670
Change-Id: I6392cf4d7cc9d3ea96554b25bb5f8abb66e9031b
---
includes/user/User.php | 2 +-
tests/phpunit/includes/user/UserTest.php | 51 ++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/includes/user/User.php b/includes/user/User.php
index 4a92f65..19e273b 100644
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -3253,6 +3253,7 @@ class User implements IDBAccessObject {
public function getRights() {
if ( is_null( $this->mRights ) ) {
$this->mRights = self::getGroupPermissions( $this->getEffectiveGroups() );
+ Hooks::run( 'UserGetRights', [ $this, &$this->mRights ] );
// Deny any rights denied by the user's session, unless this
// endpoint has no sessions.
@@ -3263,7 +3264,6 @@ class User implements IDBAccessObject {
}
}
- Hooks::run( 'UserGetRights', [ $this, &$this->mRights ] );
// Force reindexation of rights when a hook has unset one of them
$this->mRights = array_values( array_unique( $this->mRights ) );
}
diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php
index 801ab91..110a5a6 100644
--- a/tests/phpunit/includes/user/UserTest.php
+++ b/tests/phpunit/includes/user/UserTest.php
@@ -93,6 +93,57 @@ class UserTest extends MediaWikiTestCase {
}
/**
+ * @covers User::getRights
+ */
+ public function testUserGetRightsHooks() {
+ $user = new User;
+ $user->addGroup( 'unittesters' );
+ $user->addGroup( 'testwriters' );
+ $userWrapper = TestingAccessWrapper::newFromObject( $user );
+
+ $rights = $user->getRights();
+ $this->assertContains( 'test', $rights, 'sanity check' );
+ $this->assertContains( 'runtest', $rights, 'sanity check' );
+ $this->assertContains( 'writetest', $rights, 'sanity check' );
+ $this->assertNotContains( 'nukeworld', $rights, 'sanity check' );
+
+ // Add a hook manipluating the rights
+ $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'UserGetRights' => [ function ( $user, &$rights ) {
+ $rights[] = 'nukeworld';
+ $rights = array_diff( $rights, array( 'writetest' ) );
+ } ] ] );
+
+ $userWrapper->mRights = null;
+ $rights = $user->getRights();
+ $this->assertContains( 'test', $rights );
+ $this->assertContains( 'runtest', $rights );
+ $this->assertNotContains( 'writetest', $rights );
+ $this->assertContains( 'nukeworld', $rights );
+
+ // Add a Session that limits rights
+ $mock = $this->getMockBuilder( stdclass::class )
+ ->setMethods( [ 'getAllowedUserRights', 'deregisterSession', 'getSessionId' ] )
+ ->getMock();
+ $mock->method( 'getAllowedUserRights' )->willReturn( [ 'test', 'writetest' ] );
+ $mock->method( 'getSessionId' )->willReturn(
+ new MediaWiki\Session\SessionId( str_repeat( 'X', 32 ) )
+ );
+ $session = MediaWiki\Session\TestUtils::getDummySession( $mock );
+ $mockRequest = $this->getMockBuilder( FauxRequest::class )
+ ->setMethods( [ 'getSession' ] )
+ ->getMock();
+ $mockRequest->method( 'getSession' )->willReturn( $session );
+ $userWrapper->mRequest = $mockRequest;
+
+ $userWrapper->mRights = null;
+ $rights = $user->getRights();
+ $this->assertContains( 'test', $rights );
+ $this->assertNotContains( 'runtest', $rights );
+ $this->assertNotContains( 'writetest', $rights );
+ $this->assertNotContains( 'nukeworld', $rights );
+ }
+
+ /**
* @dataProvider provideGetGroupsWithPermission
* @covers User::getGroupsWithPermission
*/
--
2.8.1

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
3867425
Default Alt Text
0001-SECURITY-Move-UserGetRights-call-before-application-.patch (3 KB)

Event Timeline