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.

@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.

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)

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.

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:

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?

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

+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

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

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

@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.

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!!!

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.

Patches for branch 1.27:

1.27 patches (fixed)

Patches for REL1_29:

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

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

In T171987#4032285, @Ejegg wrote:

@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.

In T171987#4025845, @AndyRussG wrote:

@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.

In T171987#4003011, @AndyRussG wrote:

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.

In T171987#4002771, @AndyRussG wrote:

[...]

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() );

In T171987#4004944, @AndyRussG wrote:

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()

In T171987#4045589, @Base wrote:

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.