1183563 - CSP upgrade-insecure-requests directive intermittent failures

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Michael[tm] Smith [:sideshowbarker]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150714174846

Steps to reproduce:

In Nightly or or trunk build:

1. Navigate to a document that has mixed content and that sends an upgrade-insecure-requests header; e.g., https://www.w3.org/TR/wai-aria/states_and_properties and notice that you see the full green lock icon in the address bar, as expected.

2. Reload or force-reload the document once or twice or a few times and notice that at some point the full green lock gets replaced by gray-lock-with-caution indicator.

3. Open the console and notice that a "Loading mixed (insecure) display content "https://www.w3.org/Icons/w3c_home" on a secure page." message has been logged.

4. Close the console and reload or force-reload the document once or twice or a few times, and notice that the full green lock re-appears

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Michael[tm] Smith from comment #0)
> User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0)
> Gecko/20100101 Firefox/42.0
> Build ID: 20150714174846
> 
> Steps to reproduce:
> 
> In Nightly or or trunk build:
> 
> 1. Navigate to a document that has mixed content and that sends an
> upgrade-insecure-requests header; e.g.,
> https://www.w3.org/TR/wai-aria/states_and_properties and notice that you see
> the full green lock icon in the address bar, as expected.
> 

Hi Michael, thanks for filing the bug, but unfortunately I can't reproduce at the moment. It seems that the page [1] does not send a CSP header, also when inspecting the webpage it does not make use of meta csp either.

For example, the reason this web page fails [2] is because Firefox does not support meta csp as of now (see bug 663570, but we are working on that).

Either way, am I missing something or do you have another testpage available so I can inspect and fix the issue?

[1] https://www.w3.org/TR/wai-aria/states_and_properties
[2] https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/index.html

Comment 2

[Michael[tm] Smith [:sideshowbarker]]

Attachment: csp-issue.png

Comment 3

[Michael[tm] Smith [:sideshowbarker]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Hi Michael, thanks for filing the bug, but unfortunately I can't reproduce
> at the moment. It seems that the page [1] does not send a CSP header

In fact it does, as far as I can tell. Please see https://bug1183563.bugzilla.mozilla.org/attachment.cgi?id=8633615

> [1] https://www.w3.org/TR/wai-aria/states_and_properties

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Michael[tm] Smith from comment #3)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> > Hi Michael, thanks for filing the bug, but unfortunately I can't reproduce
> > at the moment. It seems that the page [1] does not send a CSP header
> 
> In fact it does, as far as I can tell. Please see
> https://bug1183563.bugzilla.mozilla.org/attachment.cgi?id=8633615

Interesting, let me investigate why I am not getting the CSP header. I will investigate. Thanks for the quick turnaround time.

Comment 5

[Tanvi Vyas[:tanvi]]

When I try https://www.w3.org/TR/wai-aria/states_and_properties I am redirected with a 307 Temporary Redirect to the HTTP version of the page http://www.w3.org/TR/wai-aria/states_and_properties

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Screen Shot 2015-07-15 at 11.21.43 AM.png

I still can't reproduce on [1] because of the temporary redirect (307). However, I applied the meta csp patches from bug 663570 and visited [2] which also makes use of the directive upgrade-insecure.

The screenshot shows 2 messages in the console:
* The first message is for the initial load of the page which indicates that the image load was upgraded from http to https.
* The second message appears after refreshing the page. INTERESTINGLY it shows
> Loading mixed (insecure) display content "https://googlechrome.github.io/samples/images/touch/chrome-touch-icon-192x192.png" on a secure page
Please note the *https* scheme in the message.

The same message appears for the reported problem in this patch. Looking at the intital screenshot provided [3], the console messages also says:
> Loading mixed (insecure) display content "https:

It seems that the directive upgrade-insecure requests itself works correctly, but the console message is displayed incorrectly. I have to further investigate the issue.

[1] https://www.w3.org/TR/wai-aria/states_and_properties
[2] https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/index.html
[3] https://bugzilla.mozilla.org/attachment.cgi?id=8633615

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1183563_csp_image_cache.patch

We shouldn't set mHadInsecureRedirect in case the CSP directive upgrade-insecure-requests is used. In such a case no data is fetched from the network over http. The problem was that imgRequest::OnRedirectVerifyCallback() was not aware of that internal redirect from http to https.

I discussed the problem with Tanvi over Vidyo and we think a similar problem appears for HSTS. ALTHOUGH not from a security perspective, but we are logging a message to the console that Firefox is loading mixed content with an *HTTPS* scheme which might confuse users, hence I filed bug 1184337 to investigate.

Comment 8

[Tanvi Vyas[:tanvi]]

Comment on attachment 8634411 [details] [diff] [review]
bug_1183563_csp_image_cache.patch

Our image cache hack for Mixed Content Blocker is turning into a bigger hack.  Images should really go through the standard Necko cache to avoid these bugs.

r+.

Providing some more information here.

Assume https://a.com has upgrade-insecure-requests set.  It has an image sourced from https://b.com/pic.jpg.  https://b.com redirects to http://c.com.  Per this patch, we don't set the mHadInsecureRedirect flag because the network request will go out to https://c.com because of upgrade-insecure-requests.  If https://c.com fails, no image will load.

Now assume https://d.com embeds the same image from https://b.com/pic.jpg but doesn't use the upgrade directive.  We will get a cache hit and check the mHadInsecureRequest flag.  It will be false since no HTTPS request was made to get the resource.  Hence, we will load the resource without any mixed content warnings.  (If it wasn't from the cache, it would have gone from https://b.com/pic.jpg to http://c.com and degraded the UI.)

Note this is the opposite of how we behave with HSTS.  Even if a load would never make an HTTP request because the HSTS header is set, we will still block or warn in Mixed Content Blocker.  See bug https://bugzilla.mozilla.org/show_bug.cgi?id=838395 for some details.  There have been many requests to change this policy and we might do so, but can't until the work to move security checks to AsyncOpen2 is complete (https://bugzilla.mozilla.org/show_bug.cgi?id=1006881).

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to Tanvi Vyas out until 7-24 [:tanvi] from comment #8)
> Our image cache hack for Mixed Content Blocker is turning into a bigger
> hack.  Images should really go through the standard Necko cache to avoid
> these bugs.

Some progress has been made on this; there was a discussion between the Necko, JS, and ImageLib teams at Whistler about adding some features to the Necko cache that would make this possible.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8634411 [details] [diff] [review]
bug_1183563_csp_image_cache.patch

Review of attachment 8634411 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. On the scale of hacks I've seen in Gecko code, this really isn't so bad. =)

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Seth Fowler [:seth] from comment #10)
> Comment on attachment 8634411 [details] [diff] [review]
> bug_1183563_csp_image_cache.patch
> 
> Review of attachment 8634411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. On the scale of hacks I've seen in Gecko code, this really
> isn't so bad. =)

Thanks Seth!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/360bb187e1a1

Comment 13

[Michael[tm] Smith [:sideshowbarker]]

(In reply to Tanvi Vyas out until 7-24 [:tanvi] from comment #5)
> When I try https://www.w3.org/TR/wai-aria/states_and_properties I am
> redirected with a 307 Temporary Redirect to the HTTP version of the page
> http://www.w3.org/TR/wai-aria/states_and_properties

Ah, yeah. As far as testing with that particular URL, I guess to get it over TLS right now, it's necesssary to update the system /etc/hosts as outlined in https://lists.w3.org/Archives/Public/public-webappsec/2015Jul/0099.html

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/360bb187e1a1