incorrect parsing of multiple HSTS headers

[floatingatoll]

from @globau at mozilla/http-observatory-website#169:

currently toolbox3.iinet.net.au returns two STS headers:

Strict-Transport-Security: max-age=31536000
Strict-Transport-Security: max-age=31536000; includeSubDomains

the observatory folds those into a single header, reporting as:

Strict-Transport-Security: max-age=31536000, max-age=31536000; includeSubDomains

https://bugzilla.mozilla.org/show_bug.cgi?id=1074642

http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2 has:

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

while the response is malformed and should be reported as such by the observatory, it shouldn't be folding multiple HSTS headers into a single item.

in terms of validation only the first should be considered.

https://hg.mozilla.org/integration/autoland/rev/0f8ae1c5e282

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.

ps. i have reported the multiple header issue to the site owner, along with a heads-up about their symantec issued cert.

[april]

I can't really fix this one, because this is happening very deep inside requests/httplib. Even if I could, I would rather it simply say that it's invalid so people figure out that they're doing something they shouldn't be doing and fix it.

[floatingatoll]

If anyone is interested in patching this in requests, this link will save considerable time in investigation (found via urllib3/urllib3#534):

https://github.com/urllib3/urllib3/pull/534/files#diff-01955f24bc4d0d621454698a584ab854R290

In essence, there are two fixes that could be proposed:

1. Replace k.lower != 'set-cookie' with k.lower not in [ 'set-cookie', 'strict-transport-security' ] and accompanying syntax glue. This does not fully implement RFC 7230's specific whitelist, but does fix the HSTS issue specifically.

2. Replace k.lower != 'set-cookie' with k.lower in [ 'whitelist', 'whitelist', 'whitelist' ] and accompanying syntax glue. This would fully implement RFC 7230's specific whitelist, but is more work than simply fixing STS alone.

[globau]

i raised this issue with urllib3, and was told the parsing we want is possible:

Headers in urllib3 can return the values as a list or as a comma separated string. The behaviour you're seeing is an artefact of old behaviour that was kept for backwards-compatibility. You likely want headers.getall('strict-transport-security') which should return a list that looks like [max-"age=31536000", "max-age=31536000; includeSubDomains"]

Sorry, the method is actually getlist (see also: https://github.com/urllib3/urllib3/blob/f8d1c787d9b02a70d66ddbde9c99061d9073d54a/src/urllib3/_collections.py#L251)

Oh and there are other backwards-compatible shims too: https://github.com/urllib3/urllib3/blob/f8d1c787d9b02a70d66ddbde9c99061d9073d54a/src/urllib3/_collections.py#L263..L269