Page MenuHomePhabricator

Security review of GlobalGroupPermissions
Closed, ResolvedPublic

Description

As a result of T134863, we should look closer at Special:GlobalGroupPermissions.

Event Timeline

Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptMay 10 2016, 9:15 PM
dpatrick triaged this task as Medium priority.May 10 2016, 9:17 PM

As one with access to such special page, I'd like to know what T134863 was about. Can I be added to that ticket?

As one with access to such special page, I'd like to know what T134863 was about. Can I be added to that ticket?

Sure thing. I've added you.

Bawolff set Security to Software security bug.Sep 13 2016, 9:50 AM
Bawolff added a project: Security.
Bawolff changed the visibility from "Public (No Login Required)" to "Custom Policy".
Bawolff added a subscriber: Bawolff.

Making it secret so I can talk about results so far.

Restricted Application removed a subscriber: Zppix. · View Herald TranscriptSep 13 2016, 9:50 AM

/me thinks it might be prudent to look at all the Special pages in Central Auth, not just that one.


Thus far, (only done Special:GlobalGroupPermissions and Special:CentralAuth as of yet), things I'm concerned about:

  • Special:GlobalGroupPermissions:
    • buildGroupView() lines 235, 243, 244 - Group name is not escaped [low severity as its unlikely for the attacker to have control of the group name]
    • line 376 - misleading code comment
  • Special:CentralAuth
    • Username in showInfo() on line 306 [Low severity, as usernames normally can't have < or > in them, but should not rely on that.]
Bawolff added a comment.EditedSep 27 2016, 2:48 PM

Ok I've looked at all the special pages, except Special:MergeAccount. Mostly I discovered a bunch of low severity sketchy stuff that's not exploitable (Not escaping i18n messages, double escaping some things, etc).

My not really tested yet patch is:

This patch does not include the following issues:

  • Special:MergeAccount (Because I haven't fully looked at the html output code of it yet, but it definitely should be carefully checked as its gnarly)
    • Also Special:MergeAccount::initSession() uses mt_rand() to generate numbers that should be cryptographically random. This is bad.
  • interwiki redirection in Special:CentralAuthAutoLogin similar to T109140

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

This includes all issues except for the interwiki redirection thing.

See also T149679

Bawolff moved this task from Backlog / Other to Patch pending review on the Security board.

Patch for final bit.

It is a limited open redirect which is probably enough of a real security issue that it should go through secret security patch process and not just be dumped on gerrit.

Reedy added a subscriber: Reedy.Jul 4 2017, 8:24 PM


Patch for final bit.
It is a limited open redirect which is probably enough of a real security issue that it should go through secret security patch process and not just be dumped on gerrit.

CR+1

Should be fine to deploy that whenever

Deployed to Wikimedia:

[22:28]	logmsgbot	!log bawolff@tin Synchronized php-1.30.0-wmf.7/extensions/CentralAuth/includes/specials/SpecialCentralAutoLogin.php: T134931 (duration: 00m 44s)
demon added a subscriber: demon.Jul 18 2017, 4:25 PM

Is anything left for this?

Is anything left for this?

Last remaining thing was me asking around if we are getting cve numbers for the open redirect issue.

demon added a comment.EditedJul 18 2017, 5:05 PM

For CentralAuth? Eh, if you think it's necessary. I say just toss the fix over the wall and e-mail wikitech-l like usual. It's a pretty WMF-centric extension.

Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".Jul 19 2017, 4:16 AM

Change 366187 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/extensions/CentralAuth@master] SECURITY: Use getFullUrlForRedirect() in Special:CentralAuthAutoLogin/setCookies

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

Change 366187 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] SECURITY: Use getFullUrlForRedirect() in Special:CentralAuthAutoLogin/setCookies

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

Change 366188 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/extensions/CentralAuth@REL1_29] SECURITY: Use getFullUrlForRedirect() in Special:CentralAuthAutoLogin/setCookies

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

Change 366189 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/extensions/CentralAuth@REL1_28] SECURITY: Use getFullUrlForRedirect() in Special:CentralAuthAutoLogin/setCookies

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

Change 366191 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/extensions/CentralAuth@REL1_27] SECURITY: Use getFullUrlForRedirect() in Special:CentralAuthAutoLogin/setCookies

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

Change 366189 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@REL1_28] SECURITY: Use getFullUrlForRedirect() in Special:CentralAuthAutoLogin/setCookies

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

Change 366191 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@REL1_27] SECURITY: Use getFullUrlForRedirect() in Special:CentralAuthAutoLogin/setCookies

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

Change 366188 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@REL1_29] SECURITY: Use getFullUrlForRedirect() in Special:CentralAuthAutoLogin/setCookies

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

Bawolff closed this task as Resolved.Jul 19 2017, 5:18 AM
Bawolff claimed this task.

Everything done and publically announced.