Closed Bug 1314356 Opened 8 years ago Closed 7 years ago

Mixed Content warning still shown on cross-origin resources upgrade-insecure-requests

Categories

(Core :: DOM: Security, defect, P2)

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ericlaw1979, Assigned: ckerschb, NeedInfo)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.87 Safari/537.36

Steps to reproduce:

Visit https://whytls.com/test/httpimage.htm. Observe Mixed Content symbol correctly shows.
Visit https://whytls.com/test/httpimageUIR.php. Observe Mixed Content symbol incorrectly still shows despite the upgrade-insecure-request directive.
Visit https://whytls.com/test/httpimageUIRSO.php. Observe that the Mixed Content symbol disappears.




Expected results:

upgrade-insecure-requests should upgrade resource requests to HTTPS *before* analysis of mixed content on the page. (This is unlike HSTS, which deliberately doesn't: https://bugzilla.mozilla.org/show_bug.cgi?id=1306258#c2)
FWIW, it appears to be some sort of caching issue, because the green lock is shown on the original page (where it shouldn't be) if you navigate back to it after getting the green lock on the page with upgrade-insecure-request. 

If you double-refresh a page with the incorrect mixed-content indicators, they seem to get corrected.
What version are you observing these behaviors in?
(In reply to Tanvi Vyas [:tanvi] from comment #2)
> What version are you observing these behaviors in?
Flags: needinfo?(ericlaw1979)
This seems to work in Nightly (52), broken as described in 51 and earlier. Hard to say if this is "correct" because we've also landed HSTS priming and your test site uses HSTS as well.
bayden.com is also marked HSTS, which is making testing difficult.

We recently added HSTS priming which is on in Nightly and Aurora.  I had to turn security.mixed_content.use_hsts and security.mixed_content.send_hsts_priming to false in order to get the Mixed Content icon on https://whytls.com/test/httpimage.htm in Nightly 53.
Note that I can reproduce this in 53.0a1 32-bit (2016-11-18) in the default configuration, if I follow the repro steps in the order provided.
Flags: needinfo?(ericlaw1979)
Using Nightly 53 on a Fresh Profile with no changes to preferences, this is what I get
> 
> Visit https://whytls.com/test/httpimage.htm. Observe Mixed Content symbol
> correctly shows.
Green lock.  No mixed content.  Webconsole shows requests to:
GET https://whytls.com/test/httpimage.htm
GET https://bayden.com/images/banner.jpg 

Note that the image is served over HTTPS, probably because bayden.com is HSTS.

> Visit https://whytls.com/test/httpimageUIR.php. Observe Mixed Content symbol
> incorrectly still shows despite the upgrade-insecure-request directive.
I see the Mixed Content icon in the url bar (lock plus yellow triangle).  The webconsole says:
Loading mixed (insecure) display content “https://bayden.com/images/banner.jpg” on a secure page[Learn More]

Even though the image is loaded over HTTPS, the Mixed Content Blocker is still flagging this as Mixed Content.  This could be because of https://bugzilla.mozilla.org/show_bug.cgi?id=1275402.


> Visit https://whytls.com/test/httpimageUIRSO.php. Observe that the Mixed
> Content symbol disappears.
> 
Location bar shows the green lock.  Webconsole shows:
Content Security Policy: Upgrading insecure request ‘http://whytls.com/test/HiDpi.png’ to use ‘https’
GET https://whytls.com/test/HiDpi.png


I think the use of HSTS plus UIR is making this really hard to debug.  For the HTTP image load, can you use a non-HSTS site?  i.e. something other than bayden.com?

Kate, can you also take a look at this?
Flags: needinfo?(ericlaw1979)
Trying this again in the same Nightly 53 profile as I used in comment 7.  I have set these two about:config prefs to false and then restarted the browser:
security.mixed_content.send_hsts_priming
security.mixed_content.use_hsts


> Visit https://whytls.com/test/httpimage.htm. Observe Mixed Content symbol
> correctly shows.
Url bar shows mixed content icon (grey lock plus yellow triangle)

Webconsole says:
Loading mixed (insecure) display content “http://bayden.com/images/banner.jpg” on a secure page[Learn More]httpimage.htm
Loading mixed (insecure) display content “http://bayden.com/images/banner.jpg” on a secure page[Learn More]

Response headers in the Network Panel show:
Upgrade-Insecure-Requests: "1"

> Visit https://whytls.com/test/httpimageUIR.php. Observe Mixed Content symbol
> incorrectly still shows despite the upgrade-insecure-request directive.
I see the Mixed Content icon in the url bar (lock plus yellow triangle).

Webconsole says:
Loading mixed (insecure) display content “https://bayden.com/images/banner.jpg” on a secure page[Learn More]

Response headers in the Network Panel show:
Upgrade-Insecure-Requests: "1"



> Visit https://whytls.com/test/httpimageUIRSO.php. Observe that the Mixed
> Content symbol disappears.
> 
> 
Green lock.

Webconsole shows:
Content Security Policy: Upgrading insecure request ‘http://whytls.com/test/HiDpi.png’ to use ‘https’

GET https://whytls.com/test/HiDpi.png

Response headers in the Network Panel show:
Upgrade-Insecure-Requests: "1"
Kate, can you look into this?
Flags: needinfo?(kmckinley)
I can reproduce in all versions of FF from 49.0.2 onward.

The best way to test this for 51+ is to turn HSTS priming off (set security.mixed_content.use_hsts and security.mixed_content.send_hsts_priming) to false.

All of the following is on 49.0.2.

Using a clean profile, visit https://whytls.com/test/httpimage.htm (no-UIR). Note the grey lock with yellow triangle.

Click the link "With UIR". Note the grey lock with the yellow triangle. No network load takes place, but load is HTTPS.

Force reload the page (Cmd-Shift-R on Mac). Note the grey lock with yellow triangle. Load is HTTPS.

Wait a couple minutes. Force reload the page. Note the green lock. Load is HTTPS.

Wait a couple minutes. Click the link "Without UIR". Force reload the page. Note the grey lock with yellow triangle. Load is via HTTPS (HSTS upgrade, no priming).

Click the link "With UIR". Note the grey lock with the yellow triangle. No network load takes place, but load is HTTPS.

Reload the page (not force) with Cmd-R. Get a 304 (unchanged). Still grey lock with yellow triangle.

Having just been in the image cache code for priming, I'm not super surprised that this type of behavior is possible. It could also be AsyncOnRedirect
Flags: needinfo?(kmckinley)
Just for reference, what happens if it isn't UIR, but HSTS has been seen is that it falls through mixed-content blocking and is allowed because security.mixed_content.block_display is false, and the UI is updated. Then in nsHttpChannel::Connect, we do the HSTS upgrade. Since the UI has already been updated, nothing changes.

HSTS priming obscures this because when the site is HSTS and security.mixed_content.use_hsts is on, once we have HSTS from the site we also short-cut mixed-content blocking.

https://dxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp?q=imgloader&redirect_type=direct#549
(In reply to Kate McKinley [:kmckinley] from comment #11)
> Just for reference, what happens if it isn't UIR, but HSTS has been seen is
> that it falls through mixed-content blocking and is allowed because
> security.mixed_content.block_display is false, and the UI is updated. Then
> in nsHttpChannel::Connect, we do the HSTS upgrade. Since the UI has already
> been updated, nothing changes.
> 
> HSTS priming obscures this because when the site is HSTS and
> security.mixed_content.use_hsts is on, once we have HSTS from the site we
> also short-cut mixed-content blocking.
> 
> https://dxr.mozilla.org/mozilla-central/source/image/imgLoader.
> cpp?q=imgloader&redirect_type=direct#549

Are we using the wrong aLoadingContext when loading a cached image?  We should be using the current document that is trying to load the image.  If we are, then we should be able to find the UIR header and upgrade.

This needs further debugging.

Also, is this only an issue for images?  What happens if you try a script (does it get updated or blocked) or try <audio>/<video> (do you get the right icon)?
Is this just images? We do have some special image caching code for perf reasons. If this is due to the general HTTP then I'd expect script, for example, to behave the same way which is going to break pages in much worse ways. Need to test with something that gets actually blocked to see if the problem is with the blocker or just with the lock display.
Assignee: nobody → ckerschb
Priority: -- → P2
Whiteboard: [domsecurity-active]
Re Comment #7, I placed an alternate version here: https://whytls.com/test/httpimageNoHSTS.htm where the target resource does not reside on a server with a HSTS rule. The problem does not reproduce in that repro.

Here's a repro where we use SCRIPT instead of IMG:

(With HSTS)
  https://whytls.com/test/httpscript.htm

(Without)
  https://whytls.com/test/httpscriptnoHSTS.htm

These do not exhibit the same problem as the image repro, although script runs (unexpectedly?) for httpscript-- is this what HSTS-priming is all about?
Flags: needinfo?(ericlaw1979)
Eric, thanks for reporting; that is indeed a valid bug which can happen and very likely confuses Firefox users.

Here is what happens; please note that I set security.mixed_content.send_hsts_priming and security.mixed_content.use_hsts to false to make sure hsts priming is not messing with the results. Some things have already been mentioned in previous comments, but let me summarize what the problem is using the STR from comment 0:

> Visit https://whytls.com/test/httpimage.htm.
Observe Mixed Content symbol correctly shows because HSTS happens after mixed content, the image in the cache ends up being stored using a scheme of https:

> Visit https://whytls.com/test/httpimageUIR.php.
Old behavior: Observe Mixed Content symbol incorrectly still shows despite the upgrade-insecure-request directive. This problem occurs because the image encountered an insecure redirect http->https and hence we manually consult mixedContentBlocker again. Now, within mixedcontentblocker we only bail if the doc has upgrade-insecure-requests set (which would be the case) but in addition the load must be http:, which is not the case, because the load from the cache is already https: because of HSTS.

New behavior: If the cache encounters an image which had an insecure redirect and upgrade-insecure-requests is set on the doc, we just invalidate the cache and reload the image.

Tanvi, what do you think? Agreed?
Attachment #8823382 - Flags: review?(tanvi)
Status: NEW → ASSIGNED
Comment on attachment 8823382 [details] [diff] [review]
bug_1314356_mixed_content_and_uir.patch

Hey Dan, given that Tanvi is on leave I don't think she will get to this bug anytime soon, but I think we should fix that. Any chance you want to review this one?
Attachment #8823382 - Flags: review?(tanvi) → review?(dveditz)
Comment on attachment 8823382 [details] [diff] [review]
bug_1314356_mixed_content_and_uir.patch

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

::: image/imgLoader.cpp
@@ +592,5 @@
> +    if (docShell) {
> +      nsIDocument* document = docShell->GetDocument();
> +      if (document && document->GetUpgradeInsecureRequests(false)) {
> +        return false;
> +      }

If we don't get a docshell you carry on; is not having a docshell an error or an expected occurrence? Elsewhere in this routine we "return false" for every error. Maybe a good spot for assertions for a quick try run through our tests, or telemetry if you think it's rarer than that (that seems overkill though).
Attachment #8823382 - Flags: review?(dveditz) → review+
(In reply to Daniel Veditz [:dveditz] from comment #17)
> If we don't get a docshell you carry on; is not having a docshell an error
> or an expected occurrence? Elsewhere in this routine we "return false" for
> every error. Maybe a good spot for assertions for a quick try run through
> our tests, or telemetry if you think it's rarer than that (that seems
> overkill though).

I am not sure if we always have a docshell in that case. Let's see what try offers [1]. Anyway, I agree that we return 'false' in other cases, but not having a docshell also means that upgrade-insecure-requests can not be set on the document. To preserve parity I would rather keep it the way it is.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4560b3a2ccaf47d11222906711a0e0276e0dc31
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4560b3a2ccaf47d11222906711a0e0276e0dc31

It seems there is always a docshell. To avoid unexpected crashes however I think it's wise to keep the 'if (docshell)' part in the patch.
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59ebf8130c6d
Do not reuse insecure chached image when upgrade-insecure-requests is present. r=dveditz
https://hg.mozilla.org/mozilla-central/rev/59ebf8130c6d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: qe-verify+
(In reply to Eric from comment #0)
> Visit https://whytls.com/test/httpimageUIR.php. Observe Mixed Content symbol
> incorrectly still shows despite the upgrade-insecure-request directive.
I'm not able to reproduce on nightly 2016-11-01, OS X 10.12, even with security.mixed_content.send_hsts_priming and security.mixed_content.use_hsts set to FALSE.

Eric, could you please verify the issue is gone on https://archive.mozilla.org/pub/firefox/candidates/54.0b2-candidates/build1/ ?
Thanks.
Flags: needinfo?(ericlaw1979)
I wasn't able to reproduce the problem in Nightly 55.0a1 (2017-04-28) (32-bit).

I do notice that the console warning in step 1 is a little bit confusing:
   Loading mixed (insecure) display content “https://bayden.com/images/banner.jpg” on a secure page[Learn More]


(The markup in the page is "HTTP" which causes the message, but by the time it's displayed, it's already been upgraded to "HTTPS")
Flags: needinfo?(ericlaw1979)
(In reply to Eric from comment #23)
> I wasn't able to reproduce the problem in Nightly 55.0a1 (2017-04-28)
> (32-bit).
> 
> I do notice that the console warning in step 1 is a little bit confusing:
>    Loading mixed (insecure) display content
> “https://bayden.com/images/banner.jpg” on a secure page[Learn More]
> 
> 
> (The markup in the page is "HTTP" which causes the message, but by the time
> it's displayed, it's already been upgraded to "HTTPS")

Kate, any chance you could take another look at this one? Especially the console warning?
Flags: needinfo?(kmckinley)
Using 64 bit Nightly 55.0a1 from 2017-05-02 and a fresh profile, I see the first mixed-content warning as expected, but no others if I follow the steps provided above. I'm not sure what the current issues is.

On the first load of a mixed-content item that goes through HSTS priming, it is a known issue that the mixed-content report will happen prior to the decision to upgrade the request. For HSTS priming that is bug 1275402, but we also see problems updating the developers console under e10s with CORS (bug 1331730).
Flags: needinfo?(kmckinley)
(In reply to Paul Silaghi, QA [:pauly] from comment #22)
> (In reply to Eric from comment #0)
> > Visit https://whytls.com/test/httpimageUIR.php. Observe Mixed Content symbol
> > incorrectly still shows despite the upgrade-insecure-request directive.
> I'm not able to reproduce on nightly 2016-11-01, OS X 10.12, even with
> security.mixed_content.send_hsts_priming and security.mixed_content.use_hsts
> set to FALSE.
> 
> Eric, could you please verify the issue is gone on
> https://archive.mozilla.org/pub/firefox/candidates/54.0b2-candidates/build1/
> ?
> Thanks.

I could not reproduce on old Nightly from 2016-11-18 as well, macOS 10.12.5. Eric could you be so kind and verify on Fx 54.0 as well?

build - https://archive.mozilla.org/pub/firefox/candidates/54.0-candidates/build3/
Flags: needinfo?(ericlaw1979)
You need to log in before you can comment on or make changes to this bug.