Page MenuHomePhabricator

Security review of GlobalGroupPermissions
Closed, ResolvedPublic

Description

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

Event Timeline

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: acl*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.]

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 acl*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.

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)

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.

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 claimed this task.

Everything done and publically announced.