1074642 - (CVE-2017-7789) Firefox ignores Strict-Transport-Security when two more STS headers are sent from server

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Muneaki Nishimura]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.124 Safari/537.36

Steps to reproduce:

1. Prepare a web server sending two more STS headers via valid https connection.
2. Access to the page from Firefox.


Actual results:

HSTS is not enabled.

Reproduction environment is here.
1) Launch following URL for receiving two STS headers.
https://alice.csrf.jp/hsts/
2) Launch following link, then HSTS is not enabled.
http://alice.csrf.jp/



Expected results:

Section 8.1 of HSTS spec (RFC 6797) requires following process to the UA.

If a UA receives more than one STS header field in an HTTP
response message over secure transport, then the UA MUST process
only the first such header field.

It seems that Firefox violates the mandatory spec RFC requires.

If one STS header is set by web app. and the other one is set by web server or some sort of network equipment, then, HSTS is not enabled on Firefox.

Comment 1

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

When visiting the url in 1 with FX 32.0.3 on OS X 10.9.5 the console shows the following errors for HTST

10:20:09.030 The site specified an invalid Strict-Transport-Security header.[Learn More] hsts
10:20:09.038 The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol. hsts

HTTP/1.1 200 OK
Date: Tue, 30 Sep 2014 14:25:07 GMT
Server: Apache
Last-Modified: Tue, 30 Sep 2014 05:18:33 GMT
Etag: "198-5044183badc40-gzip"
Accept-Ranges: bytes
Vary: Accept-Encoding
Content-Encoding: gzip
Strict-Transport-Security: max-age=3600, max-age=3600
Content-Length: 287
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive
Content-Type: text/html

$ curl -I -L alice.csrf.jp/hsts/

HTTP/1.1 200 OK
Date: Tue, 30 Sep 2014 14:39:41 GMT
Server: Apache
Last-Modified: Tue, 30 Sep 2014 05:18:33 GMT
ETag: "198-5044183badc40"
Accept-Ranges: bytes
Content-Length: 408
Vary: Accept-Encoding
Strict-Transport-Security: max-age=3600
Strict-Transport-Security: max-age=3600
Content-Type: text/html

Comment 3

[Daniel Veditz [:dveditz]]

The error firefox shows on the console is that "The site specified an invalid Strict-Transport-Security header". So we're rejecting it because we think it's invalid rather than because there are two of them.

Since proxies are allowed to coalesce headers we should probably expect that syntax.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1112

Looks like we don't do anything to properly handle combined duplicate headers (where two instances of a header are comma-joined).  IIRC, the HSTS spec says "ignore all but the first header", which means to strip everything after a comma when parsing the header value.  We should make sure we conform to whatever the HPKP spec says about this too.

Should be an easy fix to do like we did for CSP but maybe ignore subsequent values instead of using them all (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2735).

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

My understanding is that necko concatenates headers with the same name as a comma-separated list. However, from RFC 2616 section 4.2[0]:

"Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded."

Since the field-value for HSTS is not defined as a comma-separated list and since doing this does change the semantics of the message (by resulting in an invalid HSTS header), this concatenation shouldn't be happening.

[0] http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Looks like this will probably require two changes if we want to stop the merging in Necko:

1. Add HSTS to the list of headers that are singletons (not-merged)
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.h#122

2. Verify nsHttpHeaderArray::SetHeaderFromNet() does the right thing, via some HSTS test:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.cpp#55

mcmanus: does this sound right?

Comment 7

[Patrick McManus [:mcmanus]]

that's not a valid http response wrt 6797.. so I'm not sure this is even an outright bug. 7230 clarifies that the sender cannot send 2 headers with the same name if they cannot be combined into a comma-list syntax and STS isn't defined that way. (it also mentions that happens with set-cookie all the time :))

so what necko is doing (the combining) is totally legal - clarified by 7230. (we're a receiver, not a sender in this case) http://tools.ietf.org/html/rfc7230#section-3.2

So just be aware that some other intermediary might do the combination even if necko doesn't - by the same logic. (yes its https, but reverse proxies and the like are still presnet) - so having the parser drop things at the comma seems like something you should do in addition to having gecko not combine things. either that or throw an error.

you can prevent merging in necko and drop the non-first ones by adding it to the IsSingletonHeader list - sid is right.

don't add it to the SuspectDuplicateHeader list unless you want to throw an error when this happens.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

David, we are triaging DOM::Security bugs, it seems this bug was misclassified. I changed the compnent to Security. Is this bug something that should be on your radar? I am not sure what we need to do about this bug and how important it is to get it fixed. Just wanted to run it by you.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the ping. This is more of a polish bug - Gecko is behaving correctly according to the various specifications (see comment 5 and comment 7). We could attempt to detect this and give a more helpful error message in the console.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1074642 - When multiple HSTS headers are received, only consider the first.

This implements a plaintext reading of RFC 6797, which says to only consider the first, however it slightly conflicts with RFC 7230, which says that sending multiple headers which can't be merged is illegal (except for a specific whitelist which HSTS isn't in). Chrome also implements HSTS using RFC 6797's description of the parsing algorithm.

Review commit: https://reviewboard.mozilla.org/r/132066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132066/

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

I've uploaded a review that implements the plaintext RFC 6797 algorithm for handling multiple HSTS headers (namely: drop all but the first); however this needs a unit test! I'd be appreciating if someone could point me to the right place to add a C++ unit test.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Hmmm - I'm probably not the best person to ask about necko unit tests (I imagine :mcmanus would be, although this looks relevant: https://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_duplicate_headers.js ). If you wanted an integration test for this, I would add a new test to https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/stricttransportsecurity (or just extend one of the preexisting ones).

Comment 14

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8860042 [details]
Bug 1074642 - When multiple HSTS headers are received, only consider the first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132066/diff/1-2/

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8860042 [details]
Bug 1074642 - When multiple HSTS headers are received, only consider the first.

https://reviewboard.mozilla.org/r/132066/#review135050

I'm not a necko peer, so I'm not very familiar with this code. In any case, looks good to me from a code perspective, for what it's worth.
From a philosophical point of view, I'm still on the fence about changing this behavior. I feel like if a server is sending multiple HSTS headers, something is wrong and we don't wait to aid in it shooting itself in the foot. For example, say a server is initially configured to send "includeSubdomains", but then does some processing and figures out that it doesn't actually want to include subdomains because some of them don't support https. So, it adds a new header thinking it will overwrite the first one. Firefox sees both headers, discards the second one, and thinks that the domain's subdomains should be considered HSTS as well. Now the site is broken because some resources on subdomains can't be fetched because they're being upgraded to https when they shouldn't be. I think if multiple headers that have the same value are seen it would be fine to discard the copies, but not so much if there's conflicting information. Thoughts?

::: commit-message-27311:3
(Diff revision 2)
> +Bug 1074642 - When multiple HSTS headers are received, only consider the first.
> +
> +This implements a plaintext reading of RFC 6797, which says to only consider the first, however it slightly conflicts with RFC 7230, which says that sending multiple headers which can't be merged is illegal (except for a specific whitelist which HSTS isn't in). Chrome also implements HSTS using RFC 6797's description of the parsing algorithm.

I think it's customary to limit commit message line length to something like 80-ish characters (although often the first line goes over, it seems)

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 8860042 [details]
Bug 1074642 - When multiple HSTS headers are received, only consider the first.

https://reviewboard.mozilla.org/r/132066/#review135138

I understand this makes it more spec compliant - but the spec is really not good here. If this is a singleton and there are multiple copies of the header possible you're looking at a potential security hole if something can influence the header order (via smuggling or something else) if you do anything other than reject it.

the normal approach for duplicated singletons is to allow it iff they are all the same (which seems to be a common and harmless failure mode) and otherwise throw CORRUPTED_CONTENT in the case of Content-Length, though in this case I think ignoring the hsts directive would be fine.

but I'm going to r+ it cause the code seems ok and this is more of a decision for the security team on how tight they want to comply. I don't think I would do it if it were up to me.

Comment 17

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8860042 [details]
Bug 1074642 - When multiple HSTS headers are received, only consider the first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132066/diff/2-3/

Comment 18

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8860042 [details]
Bug 1074642 - When multiple HSTS headers are received, only consider the first.

https://reviewboard.mozilla.org/r/132066/#review135050

Right now such a site would already be broken in Chrome and Safari, so I don't think Firefox being a holdout on this behavior really helps developers not footgun themselves, it just makes it harder for them to debug, or risk losing the security wins of HSTS for Firefox users.

I don't have super strong feelings on whether 6797 is good or bad, but I think agreeing on one behavior for The Web (tm) which matches the RFC is the best thing for developers, because it makes debugging easier.

> I think it's customary to limit commit message line length to something like 80-ish characters (although often the first line goes over, it seems)

Fixed.

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8860042 [details]
Bug 1074642 - When multiple HSTS headers are received, only consider the first.

https://reviewboard.mozilla.org/r/132066/#review135350

Ok - I agree we should do this.

Comment 20

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8860042 [details]
Bug 1074642 - When multiple HSTS headers are received, only consider the first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132066/diff/3-4/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f8ae1c5e282
When multiple HSTS headers are received, only consider the first. r=keeler,mcmanus

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f8ae1c5e282