Page MenuHomePhabricator

CentralNotice setting a surprising content security policy in production when using &banner= URL parameter
Closed, ResolvedPublic1 Story Points

Description

https://en.wikipedia.org/wiki/Main_Page

-> No CSP set.

https://en.wikipedia.org/wiki/Main_Page?banner=Foo

->

content-security-policy:
  default-src *.wikimedia.org *.wikipedia.org *.wiktionary.org *.wikisource.org *.wikibooks.org *.wikiversity.org *.wikiquote.org *.wikinews.org www.mediawiki.org www.wikidata.org *.wikivoyage.org data: blob: 'self';
  script-src *.wikimedia.org 'unsafe-inline' 'unsafe-eval' 'self';
  style-src *.wikimedia.org data: 'unsafe-inline' 'self';

Related Objects

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 6 2019, 10:57 PM
Krenair added a subscriber: Krenair.

If nothing else, *.wikimedia.org is pretty bad. But mostly this is surprising that this is happening when both $wgCSPHeader and $wgCSPReportOnlyHeader are false, and the CSP header doesn't agree with the CSP report-only header (which presumably is also coming from CN?):

content-security-policy-report-only: script-src 'unsafe-eval' 'self' meta.wikimedia.org *.wikimedia.org *.wikipedia.org *.wikinews.org *.wiktionary.org *.wikibooks.org *.wikiversity.org *.wikisource.org wikisource.org *.wikiquote.org *.wikidata.org *.wikivoyage.org *.mediawiki.org 'unsafe-inline'; default-src 'self' data: blob: upload.wikimedia.org https://commons.wikimedia.org meta.wikimedia.org *.wikimedia.org *.wikipedia.org *.wikinews.org *.wiktionary.org *.wikibooks.org *.wikiversity.org *.wikisource.org wikisource.org *.wikiquote.org *.wikidata.org *.wikivoyage.org *.mediawiki.org wikimedia.org; style-src 'self' data: blob: upload.wikimedia.org https://commons.wikimedia.org meta.wikimedia.org *.wikimedia.org *.wikipedia.org *.wikinews.org *.wiktionary.org *.wikibooks.org *.wikiversity.org *.wikisource.org wikisource.org *.wikiquote.org *.wikidata.org *.wikivoyage.org *.mediawiki.org wikimedia.org 'unsafe-inline'; report-uri /w/api.php?action=cspreport&format=json&reportonly=1&

16:03:55 <RoanKattouw> James_F: I can find no reason why this wouldn't have been working this way since April 2018, but it's conditional on the banner parameter being present in the URL. It doesn't appear to be used for regular page views, even regular page views with banners on them

The code from CentralNotice that does this: https://github.com/wikimedia/mediawiki-extensions-CentralNotice/blob/master/includes/CentralNoticeHooks.php#L230-L240

		// FIXME: as soon as I80f6f469ba4c0b60 is available in core, get rid
		// of $wgCentralNoticeContentSecurityPolicy and use their stuff.
		if (
			$wgCentralNoticeContentSecurityPolicy &&
			$wgRequest->getVal( 'banner' )
		) {
			$wgRequest->response()->header(
				"content-security-policy: $wgCentralNoticeContentSecurityPolicy"
			);
			$out->addModules( 'ext.centralNotice.cspViolationAlert' );
		}

(regarding the FIXME comment: the patch it references was merged in May 2018)

Indeed this should be fixed... The point of this was to warn people testing banners of potential problems. Thanks @Catrope, @Jdforrester-WMF!!

Specifically, this was found by @notconfusing on https://pl.wikipedia.org/w/index.php?title=Wikipedia:Strona_g%C5%82%C3%B3wna&banner=CS_Test5&force=1 as breaking the cross-site scripts loaded by their Common.js (and JS alerting to that fact).

Am I correct in thinking that CentralNotice banners that are displayed organically, that is not via the &banner= url parameter, will not give the CSP error? Otherwise this probably would have popped up sooner, right?

Ejegg added a subscriber: Ejegg.Jun 7 2019, 5:02 AM

@notconfusing yep, you are correct. This CSP is only applied on a forced banner view, like those from preview links in the CentralNotice admin UI.

This was in response to an incident where a well-intentioned person included a non-wmf-hosted element in a banner, which ended up leaking referrers to someone else (might have even been to Facebook). We wanted to make sure that banner makers got a big warning when they were about to do something that could compromise user privacy.

Krenair renamed this task from CentralNotice setting a surprising content security policy in production to CentralNotice setting a surprising content security policy in production when using &banner= URL parameter.Jun 10 2019, 8:15 PM

I'll also point out that banner= are subject to similar security issues as the "withJS" parameter hack that exist on many wikis, in that they are able to mostly arbitrary load code from the MediaWiki namespace, including scripts that have been disabled and unaudited for many years.

As such, have a restrictive and enforced (not just warn-only) CSP policy when this parameter is enabled makes a lot of sense I think. That is, any violation coming from a banner in this mode, must not make it to "production" mode as an actual served banner. And assuming it isn't, then it must not be possible to evaluate that banner by a page view url that could be loaded without the user's knowledge. While users might not intentionally navigate to such url, it would be trivial to do as part of a hidden iframe, or shorted url shared by other means (e.g. clickbait).

Per @Jdforrester-WMF's comment, while a non-warn CSP policy is good, and better than nothing, it should be even stricter and not allow all of *.wikimedia.org, given that this top-level domain included untrusted and non-wiki content (and hence is the reason we don't use it for prod cookies, and for prod's main CSP header).

Specifically, this was found by @notconfusing on https://pl.wikipedia.org/w/index.php?title=Wikipedia:Strona_g%C5%82%C3%B3wna&banner=CS_Test5&force=1 as breaking the cross-site scripts loaded by their Common.js (and JS alerting to that fact).

This has been previously mentioned on T193332

greg added a subscriber: greg.

(We aren't working on this, and afaict, we aren't in the critical path?)

Change 526756 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[operations/mediawiki-config@master] CSP for banner preview: allow remind me later host

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

Change 527183 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[operations/mediawiki-config@master] Make banner-preview CSP match normal CSP

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

Ejegg triaged this task as Normal priority.Aug 8 2019, 3:42 PM
Ejegg changed the point value for this task from 0 to 1.
Ejegg moved this task from Backlog to Review on the Fundraising Sprint Princess Mongodb board.

Change 527183 merged by jenkins-bot:
[operations/mediawiki-config@master] Make banner-preview CSP match normal CSP

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

Mentioned in SAL (#wikimedia-operations) [2019-09-16T23:13:23Z] <jforrester@deploy1001> Synchronized wmf-config/CommonSettings.php: T225261 T194019 Adjust CentralNotice CSP for banner previews for FR-tech (duration: 00m 55s)

OK, the config change has been deployed. CSP for forced banner previews should now be similar to that on regular pageviews, just enforcing.

Ejegg closed this task as Resolved.Sep 17 2019, 7:48 PM