0x0C is not whitespace for X-Content-Type-Options

RESOLVED FIXED in Firefox 65

Status

()

P2
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: annevk, Assigned: bagder)

Tracking

unspecified
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [necko-triaged] )

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
See https://github.com/web-platform-tests/wpt/pull/13559 for tests and https://github.com/whatwg/fetch/pull/818 for the new parser definition.
(Reporter)

Comment 1

5 months ago
(In general 0x0C shouldn't be considered whitespace when parsing HTTP header values. Not sure yet to what extent this problem is widespread.)
Dragana -- can you have a look and let me know what priority you think this is? Should we tackle this soon, or next quarter?
Flags: needinfo?(dd.mozilla)
Whiteboard: [necko-triaged]
(Assignee)

Comment 3

5 months ago
Maybe just converting them to space before stripping them?

diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
index f0c86a3ab00e..8a86f1a1ebcd 100644
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -1471,10 +1471,11 @@ ProcessXCTO(nsIURI* aURI, nsHttpResponseHead* aResponseHead, nsILoadInfo* aLoadI
     if (idx > 0) {
       contentTypeOptionsHeader = Substring(contentTypeOptionsHeader, 0, idx);
     }
     // b) let's trim all surrounding whitespace
     //    e.g. "   NoSniFF   " -> "NoSniFF"
+    contentTypeOptionsHeader.ReplaceChar('\x0c', ' ');
     contentTypeOptionsHeader.StripWhitespace();
     // c) let's compare the header (ignoring case)
     //    e.g. "NoSniFF" -> "nosniff"
     //    if it's not 'nosniff' then there is nothing to do here
     if (!contentTypeOptionsHeader.EqualsIgnoreCase("nosniff")) {
(Assignee)

Comment 4

5 months ago
I chatted with :annevk on IRC and I had the logic turned the wrong way. StripWhitespace() strips too much. Here's a take instead uses TrimHTTPWhitespace(). I have not tested this, only verified that it builds!

diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
index 8877037af95a..7414c97435e7 100644
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -1481,11 +1481,11 @@ ProcessXCTO(nsIURI* aURI, nsHttpResponseHead* aResponseHead, nsILoadInfo* aLoadI
     if (idx > 0) {
       contentTypeOptionsHeader = Substring(contentTypeOptionsHeader, 0, idx);
     }
     // b) let's trim all surrounding whitespace
     //    e.g. "   NoSniFF   " -> "NoSniFF"
-    contentTypeOptionsHeader.StripWhitespace();
+    nsHttp::TrimHTTPWhitespace(contentTypeOptionsHeader, contentTypeOptionsHeader);
     // c) let's compare the header (ignoring case)
     //    e.g. "NoSniFF" -> "nosniff"
     //    if it's not 'nosniff' then there is nothing to do here
     if (!contentTypeOptionsHeader.EqualsIgnoreCase("nosniff")) {
         // since we are getting here, the XCTO header was sent;
Sounds like we have a volunteer.
Assignee: nobody → daniel
Whiteboard: [necko-triaged] → [necko-triaged]
(Assignee)

Comment 6

5 months ago
The nsCSstring's StripWhitespace() method is not very HTTP "friendly".
(Assignee)

Updated

5 months ago
Blocks: 1505752

Comment 7

5 months ago
Pushed by daniel@haxx.se:
https://hg.mozilla.org/integration/autoland/rev/c9f524cff7df
do not strip 0x0c from X-Content-Type-Options header fields r=dragana

Comment 8

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9f524cff7df
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: needinfo?(dd.mozilla)
You need to log in before you can comment on or make changes to this bug.