when processing a STS or PKP header fails, the console incorrectly always warns of an invalid header

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mathias, Assigned: Cykesiopka)

Tracking

35 Branch
mozilla42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150108202552

Steps to reproduce:

Added Strict-Transport-Security header to web page using Web.config file, so it applies to all content served. I used the value "max-age=31536000; includeSubdomains", as suggested on https://www.owasp.org/index.php/HTTP_Strict_Transport_Security. I also tried without the "includeSubdomains" and different values for max-age. All like described on MDN page (https://developer.mozilla.org/en-US/docs/Web/Security/HTTP_strict_transport_security). 

<!-- from Web.config -->
<add name="Strict-Transport-Security" value="max-age=31536000; includeSubdomains"/>



Actual results:

Header was sent, tested with "Live HTTP headers" plugin. And Firefox dev tools show Console message "The site specified an invalid Strict-Transport-Security header.[Learn More]".

https://localhost/

GET / HTTP/1.1
Host: localhost
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Cookie: AnonymousID=gid=ffa789a7-0870-47a6-b67b-04989e48384b; UserTrackingID=gid=b7985823-bf0e-4a6f-9d02-b89bdb725475; optimizelySegments=%7B%22206370730%22%3A%22ff%22%2C%22206333231%22%3A%22false%22%2C%22204948447%22%3A%22direct%22%7D; optimizelyEndUserId=oeu1421769850141r0.6186617974969267; optimizelyBuckets=%7B%7D; _ga=GA1.1.1497968550.1421769850; adv_targeting=gid=cd954c60-a34d-4aac-b65f-018d26821d5d; POPUPCHECK=1421944643493; ASP.NET_SessionId=42wvebtmajcsmnh5xejfdjyn; website#lang=de-CH; __AntiXsrfToken=03251177a7f44b41acbcc0452644eda7
Connection: keep-alive

HTTP/1.1 200 OK
Cache-Control: no-cache, no-store
Pragma: no-cache
Content-Type: text/html; charset=utf-8
Content-Encoding: gzip
Content-Language: de
Expires: -1
Vary: Accept-Encoding
Server: Microsoft-IIS/7.5
X-UA-Compatible: IE=edge
X-AspNet-Version: 4.0.30319
Set-Cookie: AnonymousID=gid=ffa789a7-0870-47a6-b67b-04989e48384b; expires=Fri, 22-Apr-2016 12:35:13 GMT; path=/
Set-Cookie: UserTrackingID=gid=b7985823-bf0e-4a6f-9d02-b89bdb725475; expires=Fri, 22-Apr-2016 12:35:14 GMT; path=/
Set-Cookie: adv_targeting=gid=cd954c60-a34d-4aac-b65f-018d26821d5d; expires=Fri, 22-Apr-2016 12:35:19 GMT; path=/
X-Powered-By: ASP.NET
P3P: policyref="http://www.comparis.ch/w3c/p3p.xml", CP="IND"
Strict-Transport-Security: max-age=31536000; includeSubdomains
X-XSS-Protection: 1; mode=block
X-Content-Type-Options: nosniff
Date: Thu, 22 Jan 2015 13:35:19 GMT
Content-Length: 22319


Expected results:

No message should be displayed for a valid header.

Updated

4 years ago
Blocks: 787133
Component: Untriaged → Security: PSM
Product: Firefox → Core
HPKP != HSTS :) It looks like you are trying to do this on localhost. Are you using a self-signed cert and getting certificate warnings when visiting the page?

I think we don't store HSTS headers in the case where there are cert warnings, but David would know for sure.
No longer blocks: 787133
Flags: needinfo?(dkeeler)
(Reporter)

Comment 2

4 years ago
Thanks for taking a look on this :-)

The issue occurs for localhost and for our dev environment. And you are right, we use self-signed certificates for both environments. But when I understand you right, the issue would not occur on our live system with a "real" certificate.
Monica is correct - the underlying transport must be secure (i.e. no warnings or certificate error overrides) for a host to be noted as an HSTS host. The error message in the console is misleading, though. It looks like we display that error message regardless of why processing the header failed (i.e. it's currently assumed that the header was invalid, but it could also be the case that the transport was insecure (or, indeed, any number of other reasons)).
Flags: needinfo?(dkeeler)
Summary: After adding seemingly valid Strict-Transport-Security header, Console message states "invalid STS header" → when processing a Strict-Transport-Security header fails, the console incorrectly always warns of an invalid STS header
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1178251
(Assignee)

Comment 5

3 years ago
I've been working on some WIPs to make the web console print out more specific errors, so assigning to myself for now.
Assignee: nobody → cykesiopka.bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Comment 6

3 years ago
Created attachment 8640340 [details]
MozReview Request: Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.

Bug 1124649 - Add specific error messages for various types of HSTS header failures.
(Assignee)

Comment 7

3 years ago
Comment on attachment 8640340 [details]
MozReview Request: Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.

Still working on the remaining test cases, but feedback on the approach taken here would be nice.
Attachment #8640340 - Flags: feedback?(dkeeler)
https://reviewboard.mozilla.org/r/14311/#review12979

This looks great! Were you going to add strings for the HPKP errors too? I think it would be good to do that at the same time.

::: netwerk/protocol/http/nsHttpChannel.cpp:1183
(Diff revision 1)
> - * Process a single security header. Only two types are suported HSTS and HPKP.
> + * Process a single security header. Only two types are supported HSTS and HPKP.

As long as we're here, we might as well change 'are supported' to 'are supported:'
Attachment #8640340 - Flags: feedback?(dkeeler) → feedback+
(Assignee)

Comment 9

3 years ago
https://reviewboard.mozilla.org/r/14311/#review12979

I currently have the HPKP changes in a separate patch, but I guess it does make sense to just do it all at once.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 903006
(Assignee)

Comment 11

3 years ago
Comment on attachment 8640340 [details]
MozReview Request: Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.

Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.
Attachment #8640340 - Attachment description: MozReview Request: Bug 1124649 - Add specific error messages for various types of HSTS header failures. → MozReview Request: Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.
Attachment #8640340 - Flags: feedback+
(Assignee)

Comment 12

3 years ago
Created attachment 8642342 [details]
MozReview Request: Bug 1124649 - Add additional invalid STS header web console tests.

Bug 1124649 - Add additional invalid STS header web console tests.
(Assignee)

Comment 13

3 years ago
Comment on attachment 8642342 [details]
MozReview Request: Bug 1124649 - Add additional invalid STS header web console tests.

Hi jryans,

Do the changes here look reasonable? I plan to replicate the same sort of structure for the PKP tests.

Feel free to forward to someone else if you're busy.

Thanks!
Attachment #8642342 - Flags: feedback?(jryans)
Comment on attachment 8642342 [details]
MozReview Request: Bug 1124649 - Add additional invalid STS header web console tests.

Seems reasonable to me!
Attachment #8642342 - Flags: feedback?(jryans) → feedback+
(Assignee)

Updated

3 years ago
Summary: when processing a Strict-Transport-Security header fails, the console incorrectly always warns of an invalid STS header → when processing a STS or PKP header fails, the console incorrectly always warns of an invalid header
(Assignee)

Comment 15

3 years ago
Comment on attachment 8640340 [details]
MozReview Request: Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.

Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.
Attachment #8640340 - Flags: review?(jryans)
Attachment #8640340 - Flags: review?(hurley)
Attachment #8640340 - Flags: review?(dkeeler)
(Assignee)

Comment 16

3 years ago
Comment on attachment 8642342 [details]
MozReview Request: Bug 1124649 - Add additional invalid STS header web console tests.

Bug 1124649 - Add additional invalid STS header web console tests.
Attachment #8642342 - Flags: feedback+ → review?(jryans)
(Assignee)

Comment 17

3 years ago
Created attachment 8642811 [details]
MozReview Request: Bug 1124649 - Add invalid PKP header web console tests.

Bug 1124649 - Add invalid PKP header web console tests.
Attachment #8642811 - Flags: review?(jryans)
Comment on attachment 8640340 [details]
MozReview Request: Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.

I'm not a reviewer for DOM / network changes, but the error text seems reasonable to me.
Attachment #8640340 - Flags: review?(jryans) → feedback+
Comment on attachment 8642342 [details]
MozReview Request: Bug 1124649 - Add additional invalid STS header web console tests.

https://reviewboard.mozilla.org/r/14713/#review13401

Ship It!
Attachment #8642342 - Flags: review?(jryans) → review+
Attachment #8642811 - Flags: review?(jryans) → review+
Comment on attachment 8642811 [details]
MozReview Request: Bug 1124649 - Add invalid PKP header web console tests.

https://reviewboard.mozilla.org/r/14849/#review13409

It looks like for STS, we add a "Learn More" link to the console output[1].

It would be nice to link to such a page for PKP errors too, perhaps [this page][2].

[1]: https://dxr.allizom.org/mozilla-central/rev/abc56d57f6e1aebade48949fb557d26eae555df8/browser/devtools/webconsole/webconsole.js#1679
[2]: https://developer.mozilla.org/en-US/docs/Web/Security/Public_Key_Pinning
Attachment #8640340 - Flags: review?(hurley) → review+
Comment on attachment 8640340 [details]
MozReview Request: Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.

https://reviewboard.mozilla.org/r/14311/#review13461

Just 2 little stylistic nits, please fix them before landing. Otherwise, the bits under netwerk/ look good to me.

::: netwerk/protocol/http/nsHttpChannel.cpp:1151
(Diff revision 3)
> +void

mark this "static void"

::: netwerk/protocol/http/nsHttpChannel.cpp:1185
(Diff revision 3)
> +void

mark this "static void" as well
Comment on attachment 8640340 [details]
MozReview Request: Bug 1124649 - Add specific error messages for various types of STS and PKP header failures.

https://reviewboard.mozilla.org/r/14311/#review13473

This is awesome. Just a minor nit.

::: netwerk/protocol/http/nsHttpChannel.cpp:1152
(Diff revision 3)
> +GetSTSConsoleErrorTag(nsAString& consoleErrorTag, uint32_t failureResult)

Slight nit: consoleErrorTag is an output parameter, right? I would expect it to be listed last in the arguments.

::: netwerk/protocol/http/nsHttpChannel.cpp:1186
(Diff revision 3)
> +GetPKPConsoleErrorTag(nsAString& consoleErrorTag, uint32_t failureResult)

Same here.
Attachment #8640340 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 23

3 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #20)
> It would be nice to link to such a page for PKP errors too, perhaps [this
> page][2].
> 
> [1]:
> https://dxr.allizom.org/mozilla-central/rev/
> abc56d57f6e1aebade48949fb557d26eae555df8/browser/devtools/webconsole/
> webconsole.js#1679
> [2]: https://developer.mozilla.org/en-US/docs/Web/Security/Public_Key_Pinning

Sounds reasonable - I filed Bug 1191335 for this.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1084994
(Assignee)

Comment 25

3 years ago
Created attachment 8643720 [details] [diff] [review]
bug1124649_part1_add-specific-err-msgs_v3.patch

+ Address nits
Attachment #8640340 - Attachment is obsolete: true
Attachment #8643720 - Flags: review+
(Assignee)

Comment 26

3 years ago
Created attachment 8643721 [details] [diff] [review]
bug1124649_part2_add-sts-webconsole-msg-tests_v2.patch
Attachment #8642342 - Attachment is obsolete: true
Attachment #8643721 - Flags: review+
(Assignee)

Comment 27

3 years ago
Created attachment 8643724 [details] [diff] [review]
bug1124649_part3_add-pkp-webconsole-msg-tests_v1.patch
Attachment #8642811 - Attachment is obsolete: true
Attachment #8643724 - Flags: review+
(Assignee)

Comment 28

3 years ago
Thanks for the reviews!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2d294f2f5df
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fe549ef1f881
https://hg.mozilla.org/mozilla-central/rev/b3d049d0dbe8
https://hg.mozilla.org/mozilla-central/rev/f1808548c6f3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.