Central auth global groups don't take session rights limit into account
Closed, ResolvedPublic

Description

From T129738

CentralAuth global group rights aren't being limitted by $this->getRequest()->getSession()->getAllowedUserRights()

Bawolff created this task.Jul 7 2016, 9:19 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 7 2016, 9:19 PM
Bawolff triaged this task as "High" priority.Jul 7 2016, 9:20 PM
Bawolff claimed this task.
Anomie added a subscriber: Anomie.Jul 7 2016, 9:27 PM

There are two possible solutions here:

  • Move the application of ->getAllowedUserRights() to after the hook. This prevents hook functions from overriding even if they want to.
  • Have CentralAuth apply ->getAllowedUserRights() itself. This means every other extension that adds rights has to do the same thing.

Both of these, code-wise, are simple. IMO, the first is probably the best. If some extension really needs the ability to override ->getAllowedUserRights(), we could always add a new hook specifically for that purpose that has the dangers clearly documented.

SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights().patch

There are two possible solutions here:

  • Move the application of ->getAllowedUserRights() to after the hook. This prevents hook functions from overriding even if they want to.
  • Have CentralAuth apply ->getAllowedUserRights() itself. This means every other extension that adds rights has to do the same thing.

    Both of these, code-wise, are simple. IMO, the first is probably the best. If some extension really needs the ability to override ->getAllowedUserRights(), we could always add a new hook specifically for that purpose that has the dangers clearly documented.

    SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights().patch

+1 to patch. I agree that first option is better.

Also, for when this is eventually public, I tried to make a phpunit test, but I had trouble getting it the existing test to run locally, so not really tested: 0001-Add-unit-test-I6392cf.patch

Also, for when this is eventually public, I tried to make a phpunit test, but I had trouble getting it the existing test to run locally, so not really tested: 0001-Add-unit-test-I6392cf.patch

That patch won't actually test it, since the test doesn't call $user->getRights() anywhere.

The best place for the test is probably in UserTest.php. 0001-SECURITY-Move-UserGetRights-call-before-application-.patch

Bawolff removed Bawolff as the assignee of this task.Jul 18 2016, 8:38 AM

This will be deployed today.

22:03 dapatrick: Deployed patch for T139670 to wmf.12

dpatrick closed this task as "Resolved".Aug 10 2016, 12:51 AM
dpatrick claimed this task.

The patch (0001-SECURITY-Move-UserGetRights-call-before-application-.patch) applies to both master (1.28) and 1.27. Branches 1.23 and 1.26 are not affected since there was no SessionManager.

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 23 2016, 1:24 AM
demon changed Security from Software security bug to None.

Change 306132 had a related patch set uploaded (by Ejegg):
SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights()

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

Change 306132 merged by jenkins-bot:
SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights()

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

Change 306093 merged by jenkins-bot:
SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights()

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

Change 306103 merged by jenkins-bot:
SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights()

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