There is now available a PHP library to mimic IE's stupid content sniffing behavior.

http://tstarling.com/blog/2008/12/secure-web-uploads/

It's possible that we should either use this code, or make sure that validation hooks are available to allow contrib to use such code to block uploads.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sreynen’s picture

Summary of "IE's stupid content sniffing behavior": IE will render some files as different types than the extension suggests (e.g. a .txt file might be rendered as an image or HTML file).

Pancho’s picture

Any progress?

jbrown’s picture

subscribing

pwolanin’s picture

pwolanin’s picture

Title: Investigate use of IE reverse-engineered code » Mitigate the security risks that come from IE and other browsers trying to sniff the mime type
pwolanin’s picture

We would need to change the core .htaccess file to check if mode_headers is enabled and then add the nosniff header when serving static content (or all content?)

http://httpd.apache.org/docs/2.2/mod/mod_headers.html#header

Pancho’s picture

I don't think we should really rely on the what browsers like to respect or not. One browser does, the other doesn't.
We should at least make sure validation hooks are available to allow contrib to double-check the data. That might include this reverse-engineered IE-code.

noamdanon’s picture

we were facing same problem with a large enterprise customer.
I think the solution should be inherent to Drupal core, and it is rather simple to solve most of it.
The rest 0.01% might need some special care.

Solution:
in upload module when validating uploaded images:
- if the file is 'txt' - make sure it does not contain HTML and if so do not pass validation
- if the file is an image (gif/jpeg) - call getimagesize() function to see if it is a real image. if False returned - do not allow it

This simple patch will solve this. And if I missed some scenario, it is easily added in the right place, where everyone using Drupal will be able to enjoy it without patching the .htaccess file.

greggles’s picture

- if the file is 'txt' - make sure it does not contain HTML and if so do not pass validation

That doesn't seem like a solution to me. It seems perfectly fine for a text file to contain a mix of different content including HTML.

Damien Tournoud’s picture

Version: 7.x-dev » 8.x-dev
Category: task » feature

If someone wants to implement content verification, let's not reinvent the wheel. Use the IEContentAnalyzer class from MediaWiki.

http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/includes/libs/IEC...

This can be easily implemented as a contrib module, let's bump this as a possible feature request for 8.x.

pwolanin’s picture

Priority: Normal » Major
Issue summary: View changes
pwolanin’s picture

Status: Active » Needs review
FileSize
1.28 KB

quick 1st pass

dawehner’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -96,6 +96,10 @@ public function onRespond(FilterResponseEvent $event) {
+    // Ask browsers not to sniff the content, since that can lead to
+    // XSS and other vulnerabilities
+    $response->headers->set('X-Content-Type-Options', 'nosniff', FALSE);

It would be great to link to some resource where this header is explained.

pwolanin’s picture

FileSize
1.42 KB
1.04 KB

how about this?

pwolanin’s picture

Should probably add a test that at least verifies the header is set any time PHP serves the response?

greggles’s picture

Makes sense to me.

pwolanin’s picture

FileSize
2.79 KB
1.75 KB

Here's a small added test, and fix the htaccess directive

greggles’s picture

Status: Needs review » Reviewed & tested by the community

This incremental change makes sense to me. Seems like a nice addition.

I filed #2400087: D7 port of 462950 - Mitigate the security risks that come from IE and other browsers trying to sniff the mime type to get this into a d7 environment so we can get broader testing just in case this doesn't get backported (or not quickly).

pwolanin’s picture

Issue tags: +Needs backport to 7.x

Well, let me see about rolling a Drupal 7 version - it should be easy, but we can use that other issue for now to run the tests.

alexpott’s picture

Category: Feature request » Task
Status: Reviewed & tested by the community » Patch (to be ported)

The implementation turned out not to be a feature. Given that facebook and google use this I think it makes sense.

This issue is a major task that will improve security and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase.

Committed 1f380a6 and pushed to 8.0.x. Thanks!

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev

  • alexpott committed 1f380a6 on 8.0.x
    Issue #462950 by pwolanin: Mitigate the security risks that come from IE...
pwolanin’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.49 KB

copying the patch over from the other issue. Not sure why it fails. There is something funny about testbot compared to local results for me.

Status: Needs review » Needs work

The last submitted patch, 23: 462950-23.patch, failed testing.

xjm’s picture

Issue tags: -Needs backport to 7.x +Needs backport to D7

Status: Needs work » Needs review

pwolanin queued 23: 462950-23.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 23: 462950-23.patch, failed testing.

Pere Orga’s picture

Status: Needs work » Needs review
FileSize
1.47 KB

Rerolled patch #23.

Pere Orga’s picture

FileSize
1.47 KB

Now with the right extension

The last submitted patch, 28: 462950-28.diff, failed testing.

Status: Needs review » Needs work

The last submitted patch, 29: 462950-29.patch, failed testing.

Pere Orga’s picture

That test is working in my local install. Is that an issue with the test bot?

pwolanin’s picture

@Pere Orga - that's the same question I had in #23 - passed locally but not on the bot

Pere Orga’s picture

Status: Needs work » Reviewed & tested by the community

ok, so this is technically Reviewed and Tested By the Community but not Ready To Be Committed, until we fix the bot.

Should we create a separate issue for the bot somewhere?

Pere Orga’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
854 bytes
473 bytes

Otherwise, we can also commit it without the test. Tests can be added later but shouldn't delay a security improvement imho

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Agree, lets do it.

Pere Orga’s picture

Didn't want to hide D8 patch

pwolanin’s picture

Title: Mitigate the security risks that come from IE and other browsers trying to sniff the mime type » Mitigate the security risks that come from IE, Chrome and other browsers trying to sniff the mime type
Priority: Major » Critical

This is actually critical, since Chrome is affected too in some cases.

David_Rothstein’s picture

Title: Mitigate the security risks that come from IE, Chrome and other browsers trying to sniff the mime type » (followup) Mitigate the security risks that come from IE, Chrome and other browsers trying to sniff the mime type
Status: Reviewed & tested by the community » Needs work
Issue tags: +7.40 release notes, +7.40 release announcement

  • David_Rothstein committed c9d1889 on 7.x
    Issue #462950 by pwolanin, Pere Orga: Mitigate the security risks that...
David_Rothstein’s picture

Committed to 7.x - thanks!

I'm not sure if "Header always set" will break Apache 1.x, but we may have reached the point where we officially stop caring :)

Question, though - we are setting this header on uncached page requests, but not on cached ones - is that an oversight? Leaving the issue open for that, for now.

I also fixed this on commit (not sure why the code comment from Drupal 8 wasn't backported):

diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc
index 5504c38..b2f2b04 100644
--- a/includes/bootstrap.inc
+++ b/includes/bootstrap.inc
@@ -1262,6 +1262,9 @@ function drupal_page_header() {
   $default_headers = array(
     'Expires' => 'Sun, 19 Nov 1978 05:00:00 GMT',
     'Cache-Control' => 'no-cache, must-revalidate, post-check=0, pre-check=0',
+    // Prevent browsers from sniffing a response and picking a MIME type
+    // different from the declared content-type, since that can lead to
+    // XSS and other vulnerabilities.
     'X-Content-Type-Options' => 'nosniff',
   );
   drupal_send_headers($default_headers);
David_Rothstein’s picture

Removing tag - this doesn't have a change record, and probably doesn't need one actually.

pwolanin’s picture

@David_Rothstein

I think the headers are added to the cache and should be added by drupal_serve_page_from_cache():

  // Send the remaining headers.
  foreach ($cache->data['headers'] as $name => $value) {
    drupal_add_http_header($name, $value);
  }
kaosagnt’s picture

This seems to have been added to Drupal 7.40 as an apache mod_headers directive

Header always set blah

Why was it not a Header merge blah?

Drupal module seckit allowed setting this header and now you wind up with

X-Content-Type-Options: nosniff, nosniff

in the headers. A merge still sets the header if it is not set.

pwolanin’s picture

The docs claims it will replace the value:

set
The response header is set, replacing any previous header with this name. The value may be a format string.

http://httpd.apache.org/docs/2.2/mod/mod_headers.html

what apache version are you using?

kaosagnt’s picture

A mixture of

Server version: Apache/2.2.15 (Unix)
Server version: Apache/2.4.12 (Red Hat)

merge works fine for me.

roball’s picture

I can confirm that as of Drupal 7.40, every page has the following response header included:
X-Content-Type-Options: nosniff, nosniff

The problem that the nosniff value is set twice to the X-Content-Type-Options header originates by the following addition to the .htaccess file:

Header always set X-Content-Type-Options nosniff

The problem in the above Apache directive is the always condition. That must be removed, so the directive reads

Header set X-Content-Type-Options nosniff

I am using Apache 2.2.15 from a stock CentOS 6. No need to change the action from set to merge.

roball’s picture

Status: Needs work » Needs review
FileSize
734 bytes

Here is the patch that solves the problem mentioned in #47.

  • alexpott committed 1f380a6 on 8.1.x
    Issue #462950 by pwolanin: Mitigate the security risks that come from IE...
pwolanin’s picture

This change looks reasonable, but we should make it for Drupal 8 also?

roball’s picture

This issue still needs to be RTBC.

pwolanin’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Needs review » Needs work

So I'm seeing the opposite problem in Drupal 7 and 8, if I take out the "always" I get 2x nosniff header lines like:

$ ~/headers www.drupal-7.local:8083/user
HTTP/1.1 200 OK
Date: Fri, 11 Dec 2015 01:08:04 GMT
Server: Apache/2.4.10 (Unix) mod_fcgid/2.3.9
X-Powered-By: PHP/5.5.17
X-Drupal-Cache: MISS
X-Content-Type-Options: nosniff
Content-Language: en
X-Generator: Drupal 7 (http://drupal.org)
Cache-Control: public, max-age=0
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Vary: Cookie,Accept-Encoding
Content-Encoding: gzip
Etag: "1449796084-1"
Last-Modified: Fri, 11 Dec 2015 01:08:04 GMT
X-Content-Type-Options: nosniff
Transfer-Encoding: chunked
Content-Type: text/html; charset=utf-8
roball’s picture

And, keeping the the always condition results in

X-Content-Type-Options: nosniff, nosniff

, right?

catch’s picture

Title: (followup) Mitigate the security risks that come from IE, Chrome and other browsers trying to sniff the mime type » Mitigate the security risks that come from IE, Chrome and other browsers trying to sniff the mime type
Version: 8.0.x-dev » 7.x-dev
Priority: Critical » Normal
Status: Needs work » Fixed

It's really hard to follow re-opened issues both in terms of history on d.o and also git commits logs.

I've opened #2633204: (followup) Mitigate the security risks that come from IE, Chrome and other browsers trying to sniff the mime type for the follow-up work. Moving this back to 7.x and fixed.

Fabianx’s picture

Priority: Normal » Critical

Restoring priority.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

calmforce’s picture

It looks like this feature completely broke my installation on openSUSE 11.4: For every js file I am getting now Chrome errors like "Refused to execute script from '<>/misc/drupal.js?o0epsk' because its MIME type ('text/x-js') is not executable, and strict MIME type checking is enabled."

I edited the file /etc/apache2/mime.types - commented out "text/x-js js" entry there and restarted Apache, but that did not help - I am still getting all these errors for js files. What do I have to do?

cilefen’s picture

The openSUSE reference suggests editing mod_mime-defaults.conf.

calmforce’s picture

Apparently, clearing Chrome caches was important for my change on the server to be recognized: it kept the old mime types way after I modified them on the server in the file /etc/apache2/mime.types and rebooted the server.

dolphinonmobile’s picture

Thanks for the patch on #48

afsch’s picture

I got similar MIME issue with Chrome trying to load any kind of files. Any patch posted here worked in my case, so I opted to remove this part of code 'X-Content-Type-Options' => 'nosniff', in includes/bootstrap.inc. I'm using Drupal 7.43

cilefen’s picture

@alexito By doing so you have reduced the security of Drupal.

David_Rothstein’s picture

Question, though - we are setting this header on uncached page requests, but not on cached ones - is that an oversight?

Looks like this is now being addressed at #2819535: x-content-type-options nosniff ignored for anonymous cached pages.