Duplicate X-Content-Type-Options headers both with the value nosniff

I'm seeing this in 8.3.x because Drupal\system\Tests\Routing\RouterTest never passes locally. It gives this fail message:

Value 'nosniff,nosniff' is equal to value 'nosniff'.

This patch fixes the test, but not the problem.

Here's what it says in the test result:

x-content-type-options value: nosniff,nosniff which contains nosniff.

It applies to both 8.3.x and 8.4.x.

#2915417: 403 due to invalid security token has terrible DX (was: "CKEditor / X-Content-Type-Options") led me here.

BTW, I've had this same problem that Alex Pott describes in the IS, for years. It's bad for core DX.

Marked #2633204: (followup) Mitigate the security risks that come from IE, Chrome and other browsers trying to sniff the mime type as a duplicate.

Quoting https://httpd.apache.org/docs/current/mod/mod_headers.html#Header:

set

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

(emphasis mine)

This means this is simply a bug in Apache. We're not the only one to notice/experience this: https://serverfault.com/questions/837782/apache2-header-always-set-merge...

I tried changing it to

  Header always unset X-Content-Type-Options
  Header always set X-Content-Type-Options nosniff

But that sadly doesn't seem to work either. It seems that in case of 4xx responses, mod_headers seems to somehow fail to inspect response headers, and hence ends up simply appending?!

The only thing I could think of that could still work, is something like:

  Header always set X-Content-Type-Options nosniff "expr=%{SCRIPT_NAME} != 'index.php'"

i.e. only do this if the script name (i.e. after executing RewriteRule ^ index.php [L]) is NOT index.php, so that we only do this for static files, since in all other cases, \Drupal\Core\EventSubscriber\FinishResponseSubscriber will already set this header for us anyway!

Sadly, after more than an hour of debugging, I still can't get it to work. Debugging Apache is a painful mess.

Hopefully this helps the next person who looks at this, and hopefully they can bring it to completion…

In case it helps, here are my many sorry attempts to either make that idea of mine work, or to at least debug it:

#  Header always unset X-Content-Type-Options "expr=%{REQUEST_STATUS} == 403"
#    Header always set X-Content-Type-Options nosniff "expr=%{LA-U:REQUEST_FILENAME} == index.php"

#Header always set DBG %{SCRIPT_FILENAME}
#    Header always set YAR HAR "expr=%{SCRIPT_NAME} == 'index.php'"
#    Header always set YAR HAR "expr=%{SCRIPT_NAME} && %{SCRIPT_NAME} != 'index.php'"
Header always set YAR HAR "expr=!%{SCRIPT_NAME}"

   Header always set X-Content-Type-Options nosniff "expr=%{REQUEST_URI} != 'index.php'"

#  Header always set X-Content-Type-Options nosniff "expr=%{REQUEST_URI} =~ m#^index\.php$#"
#  Header always set X-Content-Type-Options nosniff "expr=%{REQUEST_URI} !~ m#^index\.php$#"

FWIW, https://httpd.apache.org/docs/current/expr.html is mostly useless. :(

Also had no luck with mod_headers, but as a workaround, I left the .htaccess as is and implemented an event subscriber to strip out the header added in PHP:

<?php

namespace Drupal\my_module\EventSubscriber;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;

/**
 * Response subscriber to handle finished responses.
 */
class FinishResponseSubscriber implements EventSubscriberInterface {

  /**
   * Sets extra headers on successful responses.
   *
   * @param \Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
   *   The event to process.
   */
  public function onRespond(FilterResponseEvent $event) {
    if (!$event->isMasterRequest()) {
      return;
    }

    $event->getResponse()->headers->remove('X-Content-Type-Options');
  }

  /**
   * Registers the methods in this class that should be listeners.
   *
   * @return array
   *   An array of event listener definitions.
   */
  public static function getSubscribedEvents() {
    $events[KernelEvents::RESPONSE][] = ['onRespond'];
    return $events;
  }

}

Note sure if this still is an issue. I tried to reproduce it with Apache 2.4.25, but when fetching the homepage of a 8.9.x instance, the X-Content-Type-Options: nosniff only appears once:

curl -I localhost:32801
HTTP/1.1 200 OK
Date: Thu, 31 Oct 2019 10:27:53 GMT
Server: Apache/2.4.25 (Debian)
Cache-Control: must-revalidate, no-cache, private
X-Drupal-Dynamic-Cache: MISS
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Vary:
X-Generator: Drupal 8 (https://www.drupal.org)
X-Drupal-Cache: MISS
Content-Type: text/html; charset=UTF-8

Note: the empty Vary-header is be covered in #3089143: FinishResponseSubscriber adds empty Vary-headers on non-cacheable responses.

Apache 2.4.7 added a setifempty action in the headers module. The following eliminates the problem for me on D7:

<IfModule mod_headers.c>
  # Disable content sniffing, since it's an attack vector.
  Header setifempty X-Content-Type-Options nosniff
</IfModule>

Not sure how to turn this into a general solution, but may be a workaround for some.

According to the Apache mod_headers documentation, the onsuccess (default if ommitted) and always conditions process separate internal tables of HTTP headers. And indeed using

Header unset X-Content-Type-Options
Header always set X-Content-Type-Options nosniff

also fixes the problem for me. Note the absence of always in the first line compared to comment #12 above. Patch against 7.x-dev enclosed.

Edit: Also, having always in both of these lines still does trigger the header duplication.

The solution in #19 is working for me in D8.8.1.

Is there a reason #20 is preferable to #19?

Yes, the reason is that it should work with Apache versions older than 2.4.7 (where setifempty was introduced), as early as 2.0.

Patch for Drupal 8.9.x. It probably applies to 9.0.x. as well.

#23 with scaffold file also updated.

I was able to reproduce the issue after manually compiling Apache 2.4.6+PHP 7.2, both visually (duplicated headers) as well as confirming the failing RouterTest::testFinishResponseSubscriber:

curl -I http://d8.ddev.site:8080/
HTTP/1.1 200 OK
Date: Sun, 16 Feb 2020 15:28:27 GMT
Server: Apache/2.4.6 (Unix) PHP/7.2.27
X-Content-Type-Options: nosniff
X-Powered-By: PHP/7.2.27
Cache-Control: must-revalidate, no-cache, private
X-Drupal-Dynamic-Cache: MISS
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Vary:
X-Generator: Drupal 8 (https://www.drupal.org)
X-Drupal-Cache: HIT
Content-Type: text/html; charset=UTF-8

After applying the patch from #25, the "X-Content-Type-Options" header is present and not duplicated for A. static error pages (404, /does-not-exist.jpg), B. Dynamic error pages (404, /foo-bar), C. Regular pages (<front>) and D. static assets (/INSTALL.txt):

curl -I http://d8.ddev.site:8080/
HTTP/1.1 200 OK
Date: Sun, 16 Feb 2020 15:29:34 GMT
Server: Apache/2.4.6 (Unix) PHP/7.2.27
X-Content-Type-Options: nosniff
X-Powered-By: PHP/7.2.27
Cache-Control: must-revalidate, no-cache, private
X-Drupal-Dynamic-Cache: MISS
X-UA-Compatible: IE=edge
Content-language: en
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Vary:
X-Generator: Drupal 8 (https://www.drupal.org)
X-Drupal-Cache: HIT
Content-Type: text/html; charset=UTF-8

+++ b/.htaccess
@@ -182,6 +182,8 @@ AddEncoding gzip svgz
+  # Unset header to ensure no header duplication.
+  Header unset X-Content-Type-Options

I think this comment needs to be clearer, since it does not answer the question "why unset+set to avoid duplicate headers instead of not setting it at all". We should refer to the fact that X-Content-Type-Options is set by FinishResponseSubscriber, as without that context, this comment and the following directives are confusing.

Other than that, I think this is RTBC.

Thinking some more about this, I think we should just up the minimum required Apache version for D8 to Apache >=2.4.7 (right now it is 2.x) and use the more elegant solution from #19. I doubt many people are still stuck on <= 2.4.6 (which is over 6 years old), and if they are, then they have a large number of vulnerabilities to worry about...

+1 on upping the minimum required Apache version. I also think this should be done for Drupal 7 as well (I already use the suggestion in #19 for Drupal 7).

Opened #3114079: Update minimum supported Apache version to >= 2.4.7 to see if upping the minimum version for Apache is acceptable.

Patch in #25 with updated comments attached.

I agree with upping the Apache version. Unless this happens quickly, I believe we should move ahead with the attached patch. We can always return and use setifempty later.

Can we use merge instead of set, i.e.

Header merge X-Content-Type-Options nosniff

merge
The response header is appended to any existing header of the same name, unless the value to be appended already appears in the header's comma-delimited list of values. When a new value is merged onto an existing header it is separated from the existing header with a comma. This is the HTTP standard way of giving a header multiple values. Values are compared in a case sensitive manner, and after all format specifiers have been processed. Values in double quotes are considered different from otherwise identical unquoted values.

As nosniff is the only valid value, this should add it if it's not already present, and ignore it if it is?

Although the docs don't explicitly say, in a quick test it does seem to add it if the header doesn't exist at all.

edit: #462950-44: Mitigate the security risks that come from IE, Chrome and other browsers trying to sniff the mime type mentions the same idea

I think always should still be there.

@Liam Morland is correct in #33, and RouterTest::onFinishResponseSubscriber() is still failing locally on Apache 2.4.6 with the patch from #32.

From the Apache docs:

The optional condition argument determines which internal table of responses headers this directive will operate against: onsuccess (default, can be omitted) or always. The difference between the two lists is that the headers contained in the latter are added to the response even on error, and persisted across internal redirects (for example, ErrorDocument handlers). Note also that repeating this directive with both conditions makes sense in some scenarios because always is not a superset of onsuccess with respect to existing headers:

Adding always to the changes in #32 makes RouterTest::onFinishResponseSubscriber() pass locally.

Although the docs don't explicitly say, in a quick test it does seem to add it if the header doesn't exist at all.

I can confirm that this is indeed the behaviour for merge even on an older Apache version like 2.4.6.

While using merge makes the RouterTest pass locally, applying the patch from #36 still returns duplicate headers when testing manually:

curl -I http://d8.ddev.site:8080/
HTTP/1.1 200 OK
Date: Wed, 19 Feb 2020 09:36:31 GMT
Server: Apache/2.4.6 (Unix) PHP/7.2.27
X-Content-Type-Options: nosniff
X-Powered-By: PHP/7.2.27
Cache-Control: must-revalidate, no-cache, private
X-Drupal-Dynamic-Cache: MISS
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Vary:
X-Generator: Drupal 8 (https://www.drupal.org)
X-Drupal-Cache: HIT
Content-Type: text/html; charset=UTF-8

This is not the case when I apply the patch from #30

I think the problem is that set and merge both don't notice that the header is already set, perhaps because it was set in PHP and not Apache. This strikes me as an Apache bug, but that shouldn't stop us from working around it here. Fortunately, unset works.

I'm proposing we stick with #30.

Since it is unlikely we can increase the minimum Apache version to >2.4.6 (see #3114079: Update minimum supported Apache version to >= 2.4.7) which would allow us to use Header setifempty, and since using Header merge does not (always) seem to prevent the duplicate headers, I agree with @Liam Morland in #38.

The patch in #30 ensures that RouterTest passes locally and resolves the duplicate header issue, so RTBC.

Patch #30 also applies to 9.0.x.

D7 version.

+++ b/.htaccess
@@ -150,6 +150,9 @@ RewriteBase /~lkmorlan/drupal-7
+  # This header is also set in FinishResponseSubscriber. Unset before adding to

Drupal 7 doesn't have a FinishResponseSubscriber. This looks like it should be drupal_page_header() instead.

Thanks

Patch from #30. Restoring status.

Child issue created for D7 port.

In #3114079: Update minimum supported Apache version to >= 2.4.7, it was decided that as of Drupal 9, Apache 2.4.7 would be required. Attached is a patch for D9 that uses merge.

D7 port is in #3116482: Duplicate X-Content-Type-Options headers both with the value nosniff [D7].

I ran some manual tests with both Apache 2.4.7 and Apache 2.4.41, here are my findings (✗ = duplicate X-Content-Type-Options header, ✓ = single X-Content-Type-Options header):

Conclusion: using merge does not fix the issue (see also comment #37 where similar manual testing yielded the same result.), using the unset/set technique does address the issue on all supported versions of Apache.

As per #39, we upgraded the minimum Apache version so we would be able to use setifempty rather than merge. I have not tested this variant in #48, but some very quick testing indicates that this does resolve the issue.

Sorry about that. Patch using setifempty for 9.0.x.

For 8.9.x, the patch in #45 is current and had acheived RTBC.

On my laptop show me this (D8.9.x php7.2):

D9.x php7.3:

• I am unable to reproduce the issue on Apache 2.4.38 (with or without the patch, there is no header duplication).
• When I test the most recent patch against Apache 2.4.7, I'm still seeing duplicate headers unfortunately.

@thalles: just to confirm, were your screenshots taken after applying the patch in #50?

It may be that newer Apache has fixed a bug. Header set should not have lead to a duplicated header anyway since it is supposed to set the value of the header, but in our case it has been behaving like add.

Are you saying that in Apache 2.4.7, the patch in #50 (setifempty) does not fix the problem but that the patch in #45 (unset & set) does?

I didn't apply any patches

It may be that newer Apache has fixed a bug.

The testing in #18 and 52 indicates that the problem does not occur with Apache 2.4.25 & 2.4.38. The headers in the original report were generated using Apache 2.4.23, so that would mean the behavior changed in Apache 2.4.25 (2.4.24 was never released). I will run some tests later today to confirm this.

Are you saying that in Apache 2.4.7, the patch in #50 (setifempty) does not fix the problem but that the patch in #45 (unset & set) does?

Correct.

Ignore my previous comment, I *am* able to reproduce the duplicate headers, even with the most recent version of Apache. I am not sure why the header duplication was not occuring during my previous tests, or in @thalles' test.

Regardless of version, the only solution that works for me is unset/set.

What is your drupal version?

Perhaps we should use unset and set for all versions.

This is the patch from #45 which uses unset and set. It had reached RTBC. Given the above, I propose that we use this for all versions of D8.

The patch in #59 is the patch from #30 which had reach RTBC in #39. Various tests reported above have found no better solution. Restoring status.

This might be minor-only since it involves changes to .htaccess. It's also something we mention in release notes.

Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

Reroll of #59.

This issue is about duplicate headers. The always is already there. Making a change to that should be a separate issue.

merge does something different. This header could be set to a different value, which means merge would leave us with both values, so set is what we want.

Reroll.

Unrelated fail

Created merge request with #65.

Re-rolled for 9.3.x.

This patch is as simple as can be and #64 responds to @alexpott's concern in #61, there seems nothing left to do here.

Would be nice to have test coverage for this fix. I guess we already do per #4, except DrupalCI doesn't include mod_headers. I opened #3226187: Enable mod_headers and mod_expires on Apache for that.

It probably makes sense to commit this without waiting on that. I'm holding off for the moment, but if I or someone else doesn't commit this by Wednesday, please add a comment here then to remind me.

Since this MR is using the pattern that's recommended in https://httpd.apache.org/docs/current/mod/mod_headers.html (see "To circumvent this limitation..." on that page), I'm thinking it might help people who read this code later if we add an inline comment that links to that.

Needs work for #76

I pushed a commit to the MR that addresses #76.

I don't know why it created a second merge request. Use !323.

Re-rolled for 9.4.x.

#76 was addressed, so back to RTBC.

Sorry for the clutter but the diff above won't apply because the .htaccess is not present when you build the project with composer. That file will be copied from the scaffold folder. So this patch only patches the scaffold file.

This still needs a change record and release note.

I added the release note to the summary, but I cannot add a change record. I will add a change record now.

I added a draft for the change record. I is my first, hope it is sufficient.

Thanks! I tweaked both the note and change record a little but they look good to me.

#85 Change record and Release note is available, setting back to RTBC

Asking from complete ignorance: Is there any web.config equivalent we should consider here for parity?

@xjm Great question. https://serverfault.com/a/904278 suggests it is both possible to do and to prevent duplicate headers, as in the .htaccess changes in this issue. However I think it would be necessary for someone running IIS to test this before we commit such a change.

Great, let's do that then. Thanks!

The web.config file is completely different. This should be done in a follow-up ticket.

Opened #3336628: Set X-Content-Type-Options header in IIS web.config to discuss #99 through #104.

If IIS doesn't have it set at all yet, then @catch, @longwave, and I agreed with a followup.

However, please make note that in general, any issues for .htaccess should consider web.config in the same issue scope. In this case, probably the previous issue should have. We've had frequent issues over the years (both public hardenings and private security issues) where the web.config has not been well-maintained and has not gotten the same fixes, so we always need to evaluate it @Liam Morland. It doesn't get excluded just because it's in a different file, and how issues are scoped is a release management decision.

  # https://httpd.apache.org/docs/current/mod/mod_headers.html.
  Header onsuccess unset X-Content-Type-Options
  Header always set X-Content-Type-Options nosniff

Since we're unsetting all values for the header but only resetting nosniff, I checked to verify that this was the only possible value for it. Per https://fetch.spec.whatwg.org/#x-content-type-options-header:

1. Let values be the result of getting, decoding, and splitting `X-Content-Type-Options` from list.
2. If values is null, then return false.
3. If values[0] is an ASCII case-insensitive match for "nosniff", then return true.
4. Return false.

So at least in the present spec, this solution is OK.

Adding credit for @alexpott for code review and the initial issue report, @Liam Morland, @alex-b, and @Mile23 for work on the patch, @longwave for review and work on the CR and release note, @JoshaHubbers for work on the CR and release note, @effulgentsia for review and work on the patch, @Wim Leers for review and issue triage, and @mr.baileys for review and manual testing. I did not credit @idebr for merging HEAD with no other changes.

I was wondering what happened to the earlier attempts at test coverage, but it seems #3226187: Enable mod_headers and mod_expires on Apache needs to be addressed for that.

Release notes should always tell the user:

1. What it was before
2. What changed
3. Who might be affected
4. What they should do as a result

The release note was missing the second two, so I added a sentence for that. I similarly expanded the content of the CR with background information on why this header is used in the first place, and put in a code snippet to show exactly what site owners should update. Finally, I linked the CR from the release note.

Committed and pushed to 10.1.x. Thanks!

Added the release note to the draft release notes doc.

I appreciate all the work on this issue. Thanks to everyone involved.

However, please make note that in general, any issues for .htaccess should consider web.config in the same issue scope. In this case, probably the previous issue should have. We've had frequent issues over the years (both public hardenings and private security issues) where the web.config has not been well-maintained and has not gotten the same fixes, so we always need to evaluate it @Liam Morland. It doesn't get excluded just because it's in a different file, and how issues are scoped is a release management decision.

From the security team perspective, I support this paragraph from comment #106. We regularly will get reports "htaccess provides X security benefit that is missing in web.config" and then are in the awkward situation of deciding whether to do a security advisory and release for a fix that affects relatively few people.

I think we should consider it a bug if web.config and htaccess' behavior is different.

Published the CR.

Is this commit eligible for backporting as it is a bugfix?

Changes to .htaccess are usually only made in minor releases unless they are critical. This is not a critical bug - the duplicate header might cause warnings, but it has no actual impact on anything - so to me this is not eligible for backport.