@Bawolff, thanks so much for finding this!!!!! Could you please confirm that this is the code in question, and explain a few more details about how it works?

frb.getMonthlyAmount = function() {
        var amount = null;

        // Check the "monthly other" amount box
        if (form.otherMonthlyAmount.value !== '') {
            var otherMonthlyAmount = form.otherMonthlyAmount.value;
            otherMonthlyAmount = otherMonthlyAmount.replace(/[,.](\d)$/, ':$10');
            otherMonthlyAmount = otherMonthlyAmount.replace(/[,.](\d)(\d)$/, ':$1$2');
            otherMonthlyAmount = otherMonthlyAmount.replace(/[$£€¥,.]/g, '');
            otherMonthlyAmount = otherMonthlyAmount.replace(/:/, '.');
            amount = otherMonthlyAmount;
        }

        amount = parseFloat(amount);

        if ( isNaN(amount) ) {
            return 0;
        } else {
            var totalMonthlyAmountFormatted = frb.formatCurrency(currency, amount, language);
            $('.frb-monthly-total').text(totalMonthlyAmountFormatted);

            return amount;
        }
    };

I assume since the example contains %AVERAGE% it's the following code (and a similar bit for replacing %MINIMUM%) that occurs in https://meta.wikimedia.org/wiki/MediaWiki:FundraisingBanners/LocalizeJS-2017.js and hence all our banners

$('.frb').each(function(index){
    var newHtml = $(this).html().replace(/%AVERAGE%/g, '<span class="frb-replaced" style="white-space: nowrap;">' + avgString + '</span>');
    $(this).html(newHtml);
});

Switching .html() to .text() just isn't going to be feasible right now, there are too many places we can possibly put the %AVERAGE% and %MINIMUM%. I think adding data-mw-banner could work though, looking into implementing that now.

In the longer term we should probably switch these to use replaced <span>s, like the rest of the replaced parts of the banner (country name etc). It makes the translations more clunky, but just seems so much easier to work with.

In T239922#5715700, @Pcoombe wrote:

I assume since the example contains %AVERAGE% it's the following code (and a similar bit for replacing %MINIMUM%) that occurs in https://meta.wikimedia.org/wiki/MediaWiki:FundraisingBanners/LocalizeJS-2017.js and hence all our banners

Thanks @Pcoombe!!

Iterating over DOM elements and adding a new element with the updated content should be feasible, no?

Besides banners that include this file, are there other instances of similar code? Since the XSS works with the preview feature, any existing banner, whether live in a campaign or not, exposes the site.

In T239922#5715700, @Pcoombe wrote:

I assume since the example contains %AVERAGE% it's the following code (and a similar bit for replacing %MINIMUM%) that occurs in https://meta.wikimedia.org/wiki/MediaWiki:FundraisingBanners/LocalizeJS-2017.js and hence all our banners

$('.frb').each(function(index){
    var newHtml = $(this).html().replace(/%AVERAGE%/g, '<span class="frb-replaced" style="white-space: nowrap;">' + avgString + '</span>');
    $(this).html(newHtml);
});

Yes, that is the piece of code. There were other instances of the similar pattern of take .html(), run regex, add back .html() which might be vulnerable but most of them seemed to be not really run on user controlled html. But i didnt look at other banners or exhaustively check this banner.

In T239922#5715740, @Pcoombe wrote:

Switching .html() to .text() just isn't going to be feasible right now, there are too many places we can possibly put the %AVERAGE% and %MINIMUM%. I think adding data-mw-banner could work though, looking into implementing that now.

I think the idea would be to use depth first search/breadth first search to have js find all the descendent text nodes.

Another perhaps hacky fix, is to get rid of the inline style (use class instead), and quotes around class name. That way if the replacement is inside of an attribute, it cant break out as the replacement text has no quotes in it. But its kind of a fragile/hacky solution.

@Bawolff, if I understand correctly, this is dependent on the injection into the DOM of content using the preloadtitle URL parameter, correct? I was unaware of this Mediawiki feature... now looking at the doc for this...

Are there pages other than edit pages where URLs can add content to the DOM like that?

CentralNotice banners don't display on edit pages. If this would only work on edit pages, then the XSS does not exist for live banners, only for banner previews.

Perhaps the fastest solution would be to disable banner previews on edit pages, too?

Thanks again!!

In T239922#5715930, @AndyRussG wrote:

@Bawolff, if I understand correctly, this is dependent on the injection into the DOM of content using the preloadtitle URL parameter, correct? I was unaware of this Mediawiki feature... now looking at the doc for this...

Are there pages other than edit pages where URLs can add content to the DOM like that?

CentralNotice banners don't display on edit pages. If this would only work on edit pages, then the XSS does not exist for live banners, only for banner previews.

Perhaps the fastest solution would be to disable banner previews on edit pages, too?

Thanks again!!

I mostly used that feature to demo it without making any edits on wiki (as i wanted to show the issue without leaving any published traces. If im being honest, also because i enjoyed the puzzle of how to force a banner to show up somewhere that has reflected content from a url, as banners dont like special:expandTemplates or ?action=edit or ?preview=yes, and as you say there arent a lot of places in mw that allow you to reflect url content without making a live edit) an attacker could just edit a page to introduce the trigger code instead of relying on url based injection.

@Pcoombe wrote (on IRC):
presumably the vulnerability is that code could be saved in a page. and they just used preloadtitle to avoid creating a page demonstrating it that anyone can see

Hmmm ok so I guess it would work on a page that anyone could save...

In T239922#5715890, @Bawolff wrote:

In T239922#5715740, @Pcoombe wrote:

Switching .html() to .text() just isn't going to be feasible right now, there are too many places we can possibly put the %AVERAGE% and %MINIMUM%. I think adding data-mw-banner could work though, looking into implementing that now.

I think the idea would be to use depth first search/breadth first search to have js find all the descendent text nodes.

You can do this more conveniently with TreeWalker (https://developer.mozilla.org/en-US/docs/Web/API/TreeWalker). Along the lines of:

var avgString = 'blahblah';
$( '.frb' ).each( function () {
	var textNode, match,
		treeWalker = document.createTreeWalker( this, NodeFilter.SHOW_TEXT, null, false );
	while ( ( textNode = treeWalker.nextNode() ) ) {
		if ( ( match = textNode.nodeValue.match( /%AVERAGE%/ ) ) ) {
			var textNode2 = textNode.splitText( match.index );
			var textNode3 = textNode2.splitText( match[ 0 ].length );
			$( textNode2 ).replaceWith( '<span class="frb-replaced" style="white-space: nowrap;">' + avgString + '</span>' );
		}
	}
} );

(I wrote similar code recently for the DiscussionTools extension)

In T239922#5715938, @Bawolff wrote:
an attacker could just edit a page to introduce the trigger code instead of relying on url based injection.

Yep, got it, thanks!!!

The trouble with .text() was that the %AVERAGE% could appear as text in an element alongside other HTML elements (such as the aforementioned country name replacements). So I couldn't see a way to use that without breaking other elements.

I've now fixed the issue for current banners by just removing the HTML from the replacement completely (lines 450 and 462 in diff). None of the currencies we're using at the moment actually have whitespace in, so white-space: nowrap; isn't important. And the class frb-replaced was just a convenience feature for debugging, we can manage without it. Also took the opportunity to replace some other uses of .html() with .text() where I could.

I would like to revisit using <span>s instead of the %AVERAGE% replacement, just for consistency, but that can wait until after the English fundraiser is done.

This functionality has been around for a while. The edit above should take care of all banners since late 2017, but next I'll check to see if there are similar issues in any older ones that might be exploited via the preview feature.

Thanks @Bawolff for finding this, and everyone for the help fixing!

Okay, I believe all instances of this problem in old banner code are now fixed too, resolving the preview exploit. Checked all the subpages of our shared banner code templates, and then searched Meta for similar code and couldn't find any.

Diffs of fixes: 1, 2, 3, 4, 5

@Pcoombe Thanks so much for this fix!!! Looks great!

Just trying to understand fully how this works, and what dangers there could be going forward... So, to summarize, the attack was enabled by the fact that %AVERAGE% was replaced with a string containing tags with attributes in quotes and such. So, when an attacker places %AVERAGE% inside a quoted attribute, the browser gets some malformed HTML with wonky quotes, and can interpret as DOM elements what was previously just a string inside an attribute following the one with %AVERAGE%. Is that correct?

So, the attacker places this in the HTML:

<div class=fbr><span class="%AVERAGE%" data-foo="<script>alert('XSS bla bla bla')</script>"></span></div>

After the in-banner script runs, we get:

<div class=fbr><span class="<span class="frb-replaced" style="white-space: nowrap;">$2.75</span>" data-foo="<script>alert('XSS bla bla bla')</script>"></span></div>

It seems that the changes made by @Pcoombe prevent the specific XSS @Bawolff found. Can anyone see of any other vectors in the new version of the code?

In addition to future changes to use text() instead of html() and/or a TreeWalker-solution like the one suggested by @matmarex, what else could we do to harden this?

The code that creates the localized currency strings is non-trivial, though it doesn't seem to be susceptible to user input. Still, what about running it through [[ https://doc.wikimedia.org/mediawiki-core/master/js/#!/api/mw.html | mw.html.escape() ]] just for good measure?

@Pcoombe are there other bits of FR in-banner JS that rely on html.replace() or html()?

@Bawolff, @sbassett, @matmarex, any comments on the fix and further measures?

Thanks again, all!!!

In T239922#5718818, @AndyRussG wrote:

In addition to future changes to use text() instead of html() and/or a TreeWalker-solution like the one suggested by @matmarex, what else could we do to harden this?

This would definitely be a good start, IMO, where feasible. Though it sounds like there may still be html manipulation and replacement occurring within certain pieces of the CN/banner (or dependent JavaScript) code which may not be feasible to mitigate in such a fashion.

The code that creates the localized currency strings is non-trivial, though it doesn't seem to be susceptible to user input. Still, what about running it through [[ https://doc.wikimedia.org/mediawiki-core/master/js/#!/api/mw.html | mw.html.escape() ]] just for good measure?

The security best practice would be that anything manipulable by the user (or any untrusted data source) should be sanitized in an appropriate context while avoiding double-escaping. mw.html.escape() is certainly handy for many situations where such sanitization is required, but is not necessarily foolproof and not necessarily the correct solution when certain allow lists of html/JavaScript are a functional requirement.

@Pcoombe are there other bits of FR in-banner JS that rely on html.replace() or html()?

It would probably be a good idea to audit any additional JavaScript used by any existing banners for such patterns just to be safe. Given that this specific issue now appears resolved by @Pcoombe's patches (thanks), could this be broken out into a separate security-protected task?

(also wondering if we want to delay making this task public until after the Big English campaign is over?)

In theory this is one of the things that ContentSecurityPolicy can protect against (by disabling 'unsafe-inline'). In practice though, we probably won't be able to use that on wikimedia wikis for various reasons.

The code that creates the localized currency strings is non-trivial, though it doesn't seem to be susceptible to user input. Still, what about running it through mw.html.escape() just for good measure?

If your replacement string contains no html and is not already escaped (Which is not the case originally but is maybe the case now), then putting it through mw.html.escape() can make sense, especially if the replacement string might come from translators or external sources. This might not even be due to malice, you can always have accidental special characters, e.g. if a translatable message has <3 in it. But as Scott said, this doesn't work if any of your replacement text has html in it.

Thanks again, @sbassett and @Bawolff!

It would probably be a good idea to audit any additional JavaScript used by any existing banners for such patterns just to be safe. Given that this specific issue now appears resolved by @Pcoombe's patches (thanks), could this be broken out into a separate security-protected task?

Yes, let's do that!

(also wondering if we want to delay making this task public until after the Big English campaign is over?)

Yeah, I think that makes sense, if only to avoid drawing attention to in-banner code for now.

@sbassett and @Bawolff Please sync up with @spatton @Pcoombe to figure out how to put that review on their radar. They do track some of this work in phabricator but I'm not sure how to tag that new task correctly.

In T239922#5720376, @AndyRussG wrote:

It would probably be a good idea to audit any additional JavaScript used by any existing banners for such patterns just to be safe. Given that this specific issue now appears resolved by @Pcoombe's patches (thanks), could this be broken out into a separate security-protected task?

Yes, let's do that!

@AndyRussG @DStrine - when you have a second, would you be able to create a security review request for this, with the relevant banners and JavaScript libraries documented? We should be able to review this most likely sometime early in 2020.

(also wondering if we want to delay making this task public until after the Big English campaign is over?)

Yeah, I think that makes sense, if only to avoid drawing attention to in-banner code for now.

Will do.

(also wondering if we want to delay making this task public until after the Big English campaign is over?)

Yeah, I think that makes sense, if only to avoid drawing attention to in-banner code for now.

Will do.

I guess this should be made public now?

In T239922#5863163, @Bawolff wrote:

I guess this should be made public now?

In theory, yes, though I think we tend to like to double-check such things with WMF-Legal first, these days. (@Dsharpe)

In T239922#5728749, @sbassett wrote:

In T239922#5720376, @AndyRussG wrote:

It would probably be a good idea to audit any additional JavaScript used by any existing banners for such patterns just to be safe. Given that this specific issue now appears resolved by @Pcoombe's patches (thanks), could this be broken out into a separate security-protected task?

Yes, let's do that!

@AndyRussG @DStrine - when you have a second, would you be able to create a security review request for this, with the relevant banners and JavaScript libraries documented? We should be able to review this most likely sometime early in 2020.

Just pinging @AndyRussG and @DStrine again on this. If you'd like to submit an audit request, the Security-Team would be happy to work with you on that. We'd definitely want to properly scope and document any relevant JS libraries, resources, etc. within such a request. Thanks.