Page MenuHomePhabricator

Remove IEContentAnalyzer
Closed, ResolvedPublic

Description

IEContentAnalyzer was introduced to deal with content sniffing of resources on our potentially arbitrary user uploads.

IE8 introduced X-Content-Type-Options: nosniff, now supported by the major browsers, which if we would use it on all resources (esp. also uploads.wikimedia.org) should remove the need for IEContentAnalyzer if I understand it correctly.

Browsers listen and obey this header for all uris when they are attempted to be loaded as script or style resources. In addition the header triggers CORB protections if supported by the browser.

MediaWiki already returns x-content-type-options:nosniff` for all requests, but upload.wikimedia.org does not return results with X-Content-Type-Options: nosniff.

Removing IEContentAnalyzer was one of the notes of T232563: Drop IE6 and IE7 basic compatibility and security support. It is known that IEContentAnalyzer creates problems, it is the cause of T182264: IE content analyzer is executed on non-first chunks during chunk uploading, preventing legitimate uploads, which might actually explain some of the many user reports about failures with using chunked uploading. It is also blocking T28059: Add support for KML/KMZ filetype.

Event Timeline

Change 802592 had a related patch set uploaded (by TheDJ; author: TheDJ):

[mediawiki/core@master] Remove IEContentAnalyzer

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

Hi @Jgiannelos. I believe you have a bit of Swift knowledge ?

I would like to see the header X-Content-Type-Options: nosniff added to at least the originals (and archived originals) of upload.wikimedia.org (to prevent browsers from content sniffing)

This could of course be done via the headers sent to swift or, I was thinking of setting it on the varnish (in upload-frontend.inc.vlc.erb ??) like we do for Access-Control-Allow-Origin. Or both ? What do you think makes the most sense ?

Hey @TheDJ i have some experience with using Swift from a dev perspective but for the actual swift config I would defer to the data persistence team.

Adding Data-Persistence (work done) to request feedback

I would like to see the header X-Content-Type-Options: nosniff added to at least the originals (and archived originals) of upload.wikimedia.org (to prevent browsers from content sniffing)

This could of course be done via the headers sent to swift or, I was thinking of setting it on the varnish (in upload-frontend.inc.vlc.erb ??) like we do for Access-Control-Allow-Origin. Or both ? What do you think makes the most sense ?

I don't think Swift itself has a mechanism for setting X-Content-Type-Options; so I think if we want to do this, having it done by varnish is likely to be the way to go.

Change 890512 had a related patch set uploaded (by Legoktm; author: Legoktm):

[operations/puppet@production] varnish: Set `X-Content-Type-Options: nosniff` on upload requests

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

BCornwall triaged this task as Lowest priority.
BCornwall moved this task from Traffic team actively servicing to Ready for work on the Traffic board.
BCornwall changed the task status from Open to In Progress.Feb 21 2023, 6:26 PM
BCornwall reassigned this task from BCornwall to Legoktm.

@BBlack, @Vgutierrez: Is it reasonable to put this header into Varnish itself as per https://gerrit.wikimedia.org/r/c/890512? Seems sound to me since it's a blanket protection rather than on a per-request basis.... But I know there's a history of stuffing inappropriate solutions into Varnish so I want to make sure. :)

I think so, I've took the liberty of amending the commit and adding a test for the new header as well

Looks good to me, and appropriate at the Varnish layer in this case.

Carrying over a conversation from https://gerrit.wikimedia.org/r/c/802592 in which @tstarling says:

Ideally I would like there to be more help for third party users. Say, extra documentation, and check for the header with curl during install, and warn the user if it is not present. The installer currently has config-upload-help which links to https://www.mediawiki.org/wiki/Manual:Security , which doesn't say anything about X-Content-Type-Options. Instructions should be sprinkled throughout the manual pages on mediawiki.org, for example at https://www.mediawiki.org/wiki/Manual:Configuring_file_uploads

I agree! And it sounds like Tim would like that to be part of the acceptance criteria for this ticket (rather than, say, doing the docs later in a separate ticket). Happy to do that part. Not sure I agree with talking about the header in Manual:Configuring_file_uploads since this is a more general security setting that applies to more than file uploads. Manual:Security seems like a decent place considering the Apache/nginx documentation doesn't seem to include much detail on site configuration itself.

check for the header with curl during install, and warn the user if it is not present.

I guess we can do this for upgrades, but for fresh installs, it is a bit hard to guess up front if people are going to use thumbor or another image hosting service or proxy that is not supplying this header.

We don't have some sort of recurring environment checks anywhere right ?

Ugh.. ok. i see that the envCheckUploadsDirectory of the installer is not even checking wgUploadPath... and wgUploadBaseUrl either.

Change 891678 had a related patch set uploaded (by TheDJ; author: TheDJ):

[mediawiki/core@master] [WIP] Check images path responses for nosniff

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

whipped up a beginning for an installer check, but requires more work. (i'm about to go on vacation, so might be a while before i can get back to it)

@taavi suggested adding the header in core's pre-existing images/.htaccess; I'll submit a patch for that.

Change 891937 had a related patch set uploaded (by Legoktm; author: Legoktm):

[mediawiki/core@master] Set "X-Content-Type-Options: nosniff" header in images/.htaccess

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

Change 891937 merged by jenkins-bot:

[mediawiki/core@master] Set "X-Content-Type-Options: nosniff" header in images/.htaccess

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

Change 890512 merged by Vgutierrez:

[operations/puppet@production] varnish: Set `X-Content-Type-Options: nosniff` on upload requests

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

Mentioned in SAL (#wikimedia-operations) [2023-02-27T11:49:36Z] <vgutierrez> set "X-Content-Type-Options: nosniff" on upload.wm.o requests - T309787

vgutierrez@cp6001:~$ curl -H 'Host: upload.wikimedia.org' -k https://127.0.0.1/favicon.ico -s -v -o /dev/null 2>&1 |grep x-content-type
< x-content-type-options: nosniff

varnish is now setting the header as expected (give puppet 30 minutes to roll out this across the fleet)

<legoktm> TimStarling: curious if you have thoughts on https://www.mediawiki.org/wiki/Manual:Security#Upload_security recommending "AllowOverride None" to disable .htaccess files, which would prevent core's images/.htaccess from applying extra hardening in the future

I think "AllowOverride None" is prudent for web-writable directories. The risk is escalation from an arbitrary file write vulnerability to arbitrary execution by writing a malicious .htaccess file. The risk created by not having .htaccess is only XSS, which is not so bad. If we do a curl request on install/upgrade, as I suggested on Gerrit, we could prompt the user to modify their Apache config.

Change 802592 merged by jenkins-bot:

[mediawiki/core@master] Remove IEContentAnalyzer

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

Change 891678 merged by jenkins-bot:

[mediawiki/core@master] Check images server responses for nosniff

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