CentralNotice: Sanitize data for adding a campaign, changing campaign settings, and displaying info about campaigns
Closed, ResolvedPublic

Description

Just found that when you add a campaign in CentralNotice, the name is not validated or sanitized. Malicious JS code can be trivially injected and run on a user's browser.

Note: Please don't try the following on any public wiki, including test wikis or the beta cluster!! Please only use a local install of Mediawiki.

Locally, I was able to add a campaign named <script>alert('foo')</script>. When I clicked on the URL for this campaign, the code ran in my browser.

The campaign name is set directly in the database with no sanitization other than that provided by Mediawiki's DB classes. See SpecialCentralNotice.php and Campaign.php.

In view of this heinousness, we should carefully check all CN code for adding a campaign, changing settings, and displaying campaign info in the browser. Sanitizaiton should be added.

Only a handful of people have administration rights on CentralNotice, and only those people could perform the action described above. CentralNotice itself allows easy JS, HTML and CSS injection into almost any production wiki--that's what it's for, really. However, I imagine that if a CN admin's account were compromised, the lack of sanitation might be leveraged without the account's compromise getting noticed quickly.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 28 2017, 7:34 PM

Just prodding to see where else bad stuff can happen... At least it seems that Campaign mixin parameter values are properly escaped when saved and included in HTML.

Bawolff triaged this task as High priority.Aug 17 2017, 5:25 PM
dpatrick added a subscriber: dpatrick.EditedAug 17 2017, 5:29 PM

@DStrine, can mitigation work for this issue be added to the workboard for @AndyRussG and @awight?

@Bawolff is also going to take a look as an ad-hoc security review to try to put bounds on the severity of the issue.

Bawolff added a comment.EditedSep 6 2017, 4:02 PM

Some other things from a very quick look over:

Major

I haven't really tested any of these, so its possible I may be mistaken

  • CentralNoticeBannerLogPager.php" line 169 showInitialSettings() - Things like $row->tmplog_end_landingpages and $row->tmplog_end_devices seem like possible xss vectors

*"CentralNoticeBannerLogPager.php" line 178 - XSS when you add or remove a categroy, the category name is not escaped in the banner log.

  • "CentralNoticeCampaignLogPager.php" line 142 notlog_comment appears to not be escaped. This seems like high likelyhood xss.
  • "CentralNoticeCampaignLogPager.php" line 215 country list and langauge list appear not to be escaped. Not sure how constrained those inputs are, but this seems suspicious.
  • "CentralNoticeCampaignLogPager.php" line 297 suspicious that beginBannerKey and endBannerKey are not escaped
  • CentralNoticeCampaignLogPager::testSetChange should probably escape stuff

Minor

  • CentralNoticeBannerLogPager::formatRow - $loggedUser->getName() should be escaped (not really exploitable because < and > are invalid in usernames). (ditto "CentralNoticeCampaignLogPager.php::formatRow" line 88)
  • "CentralNoticeBannerLogPager.php" line 66 it would be better if $htmlOut used the Html class (i.e. didn't assume that stuff like $wgExtensionAssetPath would never have " in them. This is probably a safe assumption, but still unideal)
  • "CentralNoticeBannerLogPager.php" line 72 - should escape input to Xml::tags, even in probably safe cases like these.
  • There are many instances of system messages in CentralNoticeBannerLogPager.php which are ->text() which really should be either ->escaped() or ->html()
  • Campaign::campaignLogs is very sketchy imo.

*It would be better if getHistoricalBanner/getHistoricalCampaign escaped $ts

I started working on a patch to cleanup some the escaping (Both things that are lacking escaping, and also just make some of the escaping more clear/paranoia type stuff). Still wip, only did stuff in root directory and special/SpecialBannerAllocation.php so far

Ok, my final patch (Does not include T175900, which I'm keeping as a separate issue)

This fixes many minor escaping issues (e.g. wfMessage($foo)->text() when it should be ->escaped()), which aren't exploitable (by non-admins), but aren't quite following best practises. I believe this will fix all the issues this bug is about.

@AndyRussG Would your team be able to review this patch (warning, kind of big)? I did test it, but I'm not all that familar with CentralNotice.

Some other issues that I noticed or am not sure about (not covered by the patch):

  • SpecialCentralNoticeBanners.php line 1019 - Using ini_set to alter php memory limit seems sketchy. This is probably not something the extension should be doing. Memory limits are there for a reason, not to mention that in current production, that code is probably lowering not raising the limit, as I think the default limit is higher than that.
  • Its kind of unfortunate, that when people make banners they often do stuff like <a href="#" title="{{int:centralnotice-shared-hide}}"... (that is, use the message without escaping on the assumption that nobody would add a " to it). Now that's safe since the same set of people can edit mediawiki namespace as can edit banners, so unlikely anyone would do something malicious, but it'd still be cool if somehow banner authors were offered an easy way to say that a message they are using should be html escaped.

ping on this. Any chance of this happening before fundraiser? (probably too late)

AndyRussG changed the visibility from "Custom Policy" to "Custom Policy".Dec 19 2017, 9:06 PM
K4-713 added a subscriber: K4-713.Dec 19 2017, 9:11 PM
Ejegg added a subscriber: Ejegg.Feb 9 2018, 9:25 PM

Thanks so much for the patch @Bawolff . We'll review this early next week and put it on production as a security patch if it looks good.

Ejegg added a comment.Feb 12 2018, 4:12 PM

Rebased patch:

Hi! Thanks so much @Bawolff, @dpatrick, @Ejegg , for working on this!!

Been working through the patch... If it's OK, I'd like to divide it into a few smaller commits, each with related types of fixes.

(BTW, I did notice at least one problem--fixed by the patch!--that would allow someone with CN admin rights to inject arbitrary code into the browser of anyone viewing campaign logs. This could be done by changing a campaign and setting a malicious string for the change's comment. See CentralNoticeCampaignLogPager.php L146.)

The first of the smaller patches would include all the output escaping. @Bawolff, quick question about one related fix in the original patch: in Campaign.php, L530, why is the text() method used, rather than parse()? Is it because it's not close to actual output?

Thanks again!!

Since we can't put this in Gerrit just yet, we've been using this Google Doc for notes about this. Hope that's OK! (What do you normally use?)

More specific questions about escaping output:

ui.multiselect.js L170
Adding escaping here should not be harmful per se, but I fear it clutters the code of this hacked, copied 3rd-party library. Mixes in additional dependence on Mediawiki, and I think the escaping is unneeded, as the options input here are already escaped when the HTML is generated. I'd worry that when the time comes to replace this with a maintained library, someone will have to stare at it extra long to understand that the additional escaping was not needed, and that there's no need to hack the new library to do the same...

SpecialCentralNotice.php L1571
Why not escape or sanitize here? Don’t see any documentation about the escaping of these values further down the line… Though it wouldn’t be surprising…

Here is a first, smaller patch, with only the output-escaping bits.

Here's what I did regarding questions raised in previous comments:

  • SpecialCentralNotice.php L1571 Changed to parse()
  • ui.multiselect.js L170 Omitted change from original patch
  • Campaign.php L530 Left as in original patch

Hope this is OK! More soon!!!

Here's the whole series of patches (including a slightly revised version of the one already uploaded, above).




Series of patches again, rebased on current CentralNotice master:




AndyRussG added a comment.EditedMar 6 2018, 2:22 AM

Here are the changes from @Bawolff's original patch:

  • SpecialCentralNotice.php L1571 Changed to parse()
  • ui.multiselect.js L170 Omitted change from original patch
  • Campaign.php L1144, L1189, L1374 and L1380 Consolidated campaign settings name validation in Campaign::settingNameIsValid()
  • centralnotice.js L47 Used mw.Uri for URL parsing and generation

Thanks!!!!!!

@Bawolff, thanks again for such amazing work on this!!!!!! :)

The patches also resolve {T171989}. Just one possible thing left to do on that task is to add a unique constraint for campaign names in the database schema.

My only remaining concern before this becomes public is that we effectively have no restrictions on campaign names. Since we're escaping output and quoting database input, that should, in theory, be fine... However, I can't think of any good reason to allow arbitrary characters in campaign names, and it's always possible that we missed something. So, perhaps we add some sort of constraint and validation? Here's a possible approach:

  • Define a constraint on characters allowed in campaign name.
  • Query production DB to ensure all existing campaigns comply.
  • Add validation at the following points:
    • Client-side JS in form to create a campaign
    • Campaign::__construct()
    • Campaign::addCampaign()
    • All other static methods that retrieve or update settings using name as identifier
    • CentralNotice::addNoticeForm(), where we pre-fill name field for a new campaign based on a URL param
  • Possibly: Include a config setting to disable, in case external sites have campaign names that don’t comply with constraints

Thoughts?

Ejegg added a comment.Mar 6 2018, 7:41 PM

+2 on the rebased 0001-Security-cleanup-output-escaping.patch

Ejegg added a comment.Mar 6 2018, 8:16 PM

+2 on rebased 0002-Security-Cleanup-of-database-queries.patch, with 2 trivial things that could change.

commit prefix could be uppercased to SECURITY:
and this comment could be moved up along with the CNDatabase::getDb call
includes/Campaign.php L1429
// Read from the master database to avoid concurrency problems

Ejegg added a comment.Mar 6 2018, 8:23 PM

+2 on rebased 0003-Security-URL-parsing-for-log-switching.patch

Ejegg added a comment.Mar 6 2018, 8:42 PM

+2 on patch 0004, but as suggested in IRC this seems to be OK to put in gerrit

Ejegg added a comment.EditedMar 7 2018, 3:43 PM

@Bawolff any reason not to review the split-out patch with the hidden "title" inputs on the allocation pages in gerrit? I can't think of any exploit vectors there.

Ejegg added a comment.Mar 7 2018, 7:01 PM

Rebased, split patches, updated with casing of commit message and moved comment.

+2 @Ejegg's minor change in 0002-SECURITY-Cleanup-of-database-queries.patch :) Thanks!!!

Ejegg added a comment.Mar 7 2018, 11:04 PM

Parts 0001, 0002, and 0003 are deployed to production (wmf.23 and wmf.24) as security patches. Waiting till tomorrow to send an announcement and merge them to master.

Ejegg added a comment.Mar 8 2018, 8:11 PM

Patches for branch 1.27:

Ejegg added a comment.Mar 8 2018, 8:55 PM

1.27 patches (fixed)

Ejegg added a comment.Mar 8 2018, 10:50 PM

Patches for REL1_29:

Ejegg added a comment.Mar 8 2018, 10:56 PM

Patches for REL1_30:
{F14769190}

+2 Patches for REL1_27, reviewed and smoke tested

+2 Patches for REL1_29, reviewed and smoke tested

+2 Patches for REL1_30, reviewed and smoke tested

Ejegg changed the visibility from "Custom Policy" to "Public (No Login Required)".
Base added a subscriber: Base.EditedMar 13 2018, 6:22 AM

Basically people with ns8 edit right can just edit sidewide js or default gadgets and admins can also edit other users' js. It is cool to have sanitisation given it works well, but it is not a big priority :)

Oh whoops. Looks like I missed a whole bunch of messages directed at me. Sorry about that.

Thanks everyone for all your hard work getting this out :D

@Bawolff any reason not to review the split-out patch with the hidden "title" inputs on the allocation pages in gerrit? I can't think of any exploit vectors there.

Little late now, but yeah, it would have been fine to review the non-security sensitive parts on gerrit.

@Bawolff, thanks again for such amazing work on this!!!!!! :)

The patches also resolve {T171989}. Just one possible thing left to do on that task is to add a unique constraint for campaign names in the database schema.

My only remaining concern before this becomes public is that we effectively have no restrictions on campaign names. Since we're escaping output and quoting database input, that should, in theory, be fine... However, I can't think of any good reason to allow arbitrary characters in campaign names, and it's always possible that we missed something. So, perhaps we add some sort of constraint and validation? Here's a possible approach:

  • Define a constraint on characters allowed in campaign name.
  • Query production DB to ensure all existing campaigns comply.
  • Add validation at the following points:
    • Client-side JS in form to create a campaign
    • Campaign::__construct()
    • Campaign::addCampaign()
    • All other static methods that retrieve or update settings using name as identifier
    • CentralNotice::addNoticeForm(), where we pre-fill name field for a new campaign based on a URL param
  • Possibly: Include a config setting to disable, in case external sites have campaign names that don’t comply with constraints

    Thoughts?

Quoting is really the important part. Adding constraints to names can make it more difficult to exploit issues (Especially if the constraint is a whitelist of known good characters - e.g. /[a-zA-Z0-9_-]/. Constraints that are just blacklists of evilish characters are usually less useful as usually people miss cases and sometimes it leads to a false sense of security. e.g. There was recently a case in a third party extension where the author banned spaces, thinking you needed whitespace to do anything evil, but forgot to ban tabs and newlines. On the other hand. Banning /["'<>]/ in identifers would make it very difficult to exploit an XSS. Unfortunately, " and ' are generally useful characters to users...). So generally I would say, first and foremost make sure all the escaping is good. You can add constraints as a defense in depth if you want, but its much less important.

Since we can't put this in Gerrit just yet, we've been using this Google Doc for notes about this. Hope that's OK! (What do you normally use?)

Google doc seems to be the go to place at the foundation for private notes that can't be public. I think its a totally ok place to use. The only other choices are basically officewiki or private bugs/pastes/etc in phab.

Generally, security team does stuff on phab tickets, although usually that's more for small conversations. We do use google docs for team planning stuff. Its really up to whatever you find easiest.

[...]

The first of the smaller patches would include all the output escaping. @Bawolff, quick question about one related fix in the original patch: in Campaign.php, L530, why is the text() method used, rather than parse()? Is it because it's not close to actual output?

I think it gets escaped later in "special/SpecialGlobalAllocation.php" line 281 $this->msg( 'centralnotice-notice-heading', $grouping['label'] )->text() );

More specific questions about escaping output:

ui.multiselect.js L170
Adding escaping here should not be harmful per se, but I fear it clutters the code of this hacked, copied 3rd-party library. Mixes in additional dependence on Mediawiki, and I think the escaping is unneeded, as the options input here are already escaped when the HTML is generated. I'd worry that when the time comes to replace this with a maintained library, someone will have to stare at it extra long to understand that the additional escaping was not needed, and that there's no need to hack the new library to do the same...

Its calling option.text(). This unescapes the element, which is why we need to add back escaping. For example, if php outputs something like <option>&lt;b&gt;</option>, then option.text() will equal <b>

SpecialCentralNotice.php L1571
Why not escape or sanitize here? Don’t see any documentation about the escaping of these values further down the line… Though it wouldn’t be surprising…

I believe its escaped in BaseTemplate::makeLink()

Basically people with ns8 edit right can just edit sidewide js or default gadgets and admins can also edit other users' js. It is cool to have sanitisation given it works well, but it is not a big priority :)

Well in general we want to limit the ability of admins to inject javascript only to MediaWiki:Common.js/gadgets/etc as these pages are monitored much more closely than other random pages in the MediaWiki namespace (I recognize we haven't reached this goal, but hopefully one day). Additionally, sometimes it can be hard to see the full code flow. Sometimes what looks like a mistake which outputs an i18n message unsanitized, actually might be a more general vulnerability. Fixing all the i18n sanitization errors makes it easier to see at a glance that there are no sanitization errors at all, which in turn makes it much less likely for there to be a "real" security issue that's misidentified as just an i18n sanitization error.