x-content-type-options nosniff ignored for anonymous cached pages

Hi,

Anyone could please review the attached patch I believe It should resolve the missing "x-content-type-options" and append it to header, when the Drupal default cache is enabled.

Thanks,
Sumeet

This patch passed https://www.drupal.org/pift-ci-job/507915 . Thanks!

-- Attached patch should not be used, please read on | Marked as needs review because I'm curious what the testbot says about this --

Although the patch applies cleanly it doesn't seem to fix the issue. In my scenario I'm running on nginx and unable to set the header outside of Drupal.

I can't figure out what the correct solution would be exactly but I believe your fix is only partial. Your fix does allow the header to be cached so it's output on later page visits. However the header itself never makes it into the cache so it isn't actually sent to the browser once the page is loaded from cache.

The drupal_page_header() function calls drupal_send_headers() which directly sends the headers. When the page gets stored in the cache though the headers are loaded using drupal_get_http_header() which loads it from the static variable named drupal_http_headers. This static variable is an array that's only populated through the use of drupal_add_http_header().

The quickest solution would be to call drupal_add_http_header() instead of drupal_send_headers() from drupal_page_header() as the former will call the latter.

A core maintainer may have to weigh in as to what the side effects of this are though because I'm not entirely familiar with the system.

I have a patch attached and hidden that implements the above train of thought. The reason I've hidden it is that it has the side effect of caching all 3 default headers. This causes the cache-control to be stuck at forcing the browser to revalidate at every turn.

A better solution is to call `drupal_add_http_header()` for `X-Content-Type_Options` explicitly but I'm unsure where that should happen.

Explaining the problem in an issue queue post sometimes helps you figure things out.

The attached patch addresses the above issue. The above issue is caused by default_headers passed to drupal_send_headers not being able to overwrite headers explicitly set by drupal_add_http_header. Rightfully so. I think the nosniff header is important enough to be opt-out and thus I have promoted it to a header that's always set in drupal_page_header by calling drupal_add_http_header only for that specific header.

A user can disable the header by setting a different value when calling drupal_add_http_header elsewhere in the codebase. This along with the fix from SumeetJaggi causes the header to be properly sent for anonymous cached pages.

An alternative to the attached patch of promoting the X-Content-Type-Options could be to add it as another default header before line 1574 in drupal_serve_page_from_cache in bootstrap.inc (which would still require SumeetJAggi's addition).

After putting this down last night I have come to an additional insight. I can't exactly figure out how browsers cache headers but after reading RFC 2616, section 10.3.5 as mentioned on line 1549 of bootstrap.inc I have come to the realisation that the X-Content-Type-Options is probably cached as part of the page (in the way the browser treats the page the first time and thus upon every next request).

With this in mind I think it is correct that the "X-Content-Type-Options" header is not sent for requests that can be served from the browser cache. These terminate in a 304 Not Modified response at line 1577 of bootstrap.inc.

The remaining issue is then that if a request is not served from browser cache (not terminating at line 1579 of bootstrap.inc) but still served from Drupal cache the X-Content-Types-Options is not sent. This is fixed when promoting the header to use drupal_add_http_header instead of setting it as default header.

Attached is a patch only implementing the first half of #10 where the header is set using drupal_add_http_header so it's correctly cached on Drupal's side and sent when serving pages from the Drupal cache. It omits the addition of X-Content-Type-Options on line 1552 so that the header is not resent when the page is to be served from the browser cache (as the browser has previously determined how to treat the resource).

In practice I'm not sure there's a need to set this header on cached page requests (does Drupal ever serve a user-uploaded file via the page cache?) but for consistency it definitely makes sense.

(Apparently I actually noticed this problem when committing the original patch, but I don't think I understood exactly why it happened :)

The explanation in this issue and the latest patch make sense on a first glance.... Do we know if this issue affects Drupal 8 also?

This is not an issue on Drupal 9, and it's fair enough that it shouldn't affect cached pages, but it seems to make commercial penetration testing providers generate spurious complaints about Drupal 7 sites (don't ask) so a fix would be nice :-)

I've refactored the patch so the header is set to a default value in drupal_serve_page_from_cache() (just as it is in drupal_page_header() and can be overridden as needed via hook_boot() so now you have two fixes to choose from. (sorry! :-)

Added this issue to #3216978: [meta] Priorities for 2021-12-01 release of Drupal 7 for tracking, but that doesn't guarantee if/when it'll be reviewed and committed.

Tested patch #14 just right now and it worked for me.
Marking this issue as RTBC.

D9 adds this header in \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond():

    // 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.
    // https://www.owasp.org/index.php/List_of_useful_HTTP_headers
    $response->headers->set('X-Content-Type-Options', 'nosniff', FALSE);
    $response->headers->set('X-Frame-Options', 'SAMEORIGIN', FALSE);

https://git.drupalcode.org/project/drupal/-/blob/9.4.0-alpha1/core/lib/D...

Is there any reason why D7 shouldn't also emit this header in the same place as X-Frame-Options (and Permissions-Policy and others) e.g.:

https://git.drupalcode.org/project/drupal/-/blob/7.89/includes/common.in...

A quick manual test suggests that approach seems to work.

I would also like to see a test added for this ideally; perhaps just a couple of lines added to an existing test for a response from cache. However, if the testbot runs apache, am I right in thinking the header will always be added by .htaccess anyway?

Writing a test ended up being a bit convoluted, but this approach seems to work okay.

The test adds a callback to system_test.module that retrieves an object from cache_page and returns any headers stored with it.

This means we can verify that Drupal itself emitted the header, rather than apache via .htaccess

Passes locally with the patch adding the header in common.inc but let's see if the testbot agrees.

Ooops - I've put page_cache in a couple of places and that should be cache_page.

Shouldn't affect the test as it's just in comments etc.. will fix later if we keep this test.

Let's try those again.

Oops and this time without hacky Xdebug hacks.

Very naughty, setting your own patch to RTBC.

:) yes, fair enough.

I'm not sure we're following #3093258: [policy, no patch] Proposed Drupal 7 contributor / maintainer workflow strictly, but the general idea is that the maintainers will review issues we've tagged as "Pending Drupal 7 commit". So the Framework Manager (@Fabianx) will review any patches I've worked on before they're committed. On that basis I sometimes bend the rules and mark patches I've worked on as RTBC before the final review.

More eyes are always welcome though, so if anyone else wants to review (and hopefully RTBC) this.. that'd be great. The next release is due in a few weeks (#3259739: [meta] Priorities for 2022-06-01 release of Drupal 7).

We should consider whether this is worth a Change Record as it may change the headers output by some sites.

e.g. if a site was using a menu callback to serve something like JS or CSS without setting the content type appropriately, this could possibly break that functionality.

Sorry about that, I meant to test this straight after my whine and then things happened.

I've applied the patch from #23 to my site and it works as advertised, thank you :-)

RTBC! - Looks good to me, approved. Thanks all!

Added a CR.

Thank you everyone!