Consider an http page that has an https iframe that loads mixed display content (for example, http://www.smbc-comics.com/index.php?id=3793 currently does this). Right now in nightly, this changes the globe icon into a lock-with-a-triangle icon, which is wrong for multiple reasons. In particular, though, it doesn't even make sense to warn the user about mixed display content when the top level load isn't secure. (FWIW, I also don't think we should show the indicator for mixed active content in cases like this, but I guess if blocking the load breaks the page, that's the only way to unbreak it.) See also bug 918237 which is about improving the console message in these cases.

Yes, the site identity box is also misleading in this case, telling the user the page isn't "fully secure", when it's not secure at all. Here are some examples: HTTP parent, Mixed display content loaded in HTTPS child - http://people.mozilla.org/~tvyas/http_parent_with_mixed_passive_https_child.html Grey yield sign and incorrect text in control center. HTTP parent, Mixed active content blocked in HTTPS child - http://people.mozilla.org/~tvyas/mixedgrandiframe.html HTTP parent, Mixed active content loaded in HTTPS child (after you disable protection!) - http://people.mozilla.org/~tvyas/mixedgrandiframe.html Notice the icon is a globe not a yellow yield sign and there is no confusing text in the control center.

Looks like as soon as the iframe changes its security state the browser UI sees STATE_IS_BROKEN. So should that be fixed in the platform?

Needinfo'ing myself to look at this more closely.

[Tracking Requested - why for this release]: Now that we are using a lock with a yellow triangle instead of a grey lock, this problem is worse. We have to fix this for Firefox 42.

I'm working on a patch for this.

This patch fixes the bug in nsMixedContentBlocker. But unfortunately, there are still other bugs. Was hoping to fix this completely before I'm out for a week. When you go to http://people.mozilla.org/~tvyas/http_parent_with_mixed_passive_https_child.html with the patch applied, you see the correct security icons (i.e. the globe instead of the gray lock with yellow triangle). But if you open a new tab and then go back to this tab, the security UI changes back to the gray lock with the yellow triangle. I'm not sure why and won't be able to investigate right now. But I'm almost 100% sure this has to do with nsSecureBrowserUIImpl. It might be this part of the file https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsSecureBrowserUIImpl.cpp#266 where we override the security state it detects with the one that Mixed Content Blocker detected (since MCB is more accurate than nsSecureBrowserUI). If nsSecureBrowserUIImpl has access to the uri/principal of the root docshell, it can check if the root is an HTTPS page. If it isn't, the "STATE_IS_BROKEN" flag shouldn't be set, but the others should continue to get set. We can still review and land this patch since it does help the situation and show the right icon at first. The wrong icon just shows up if you are swapping back and forth between tabs.

But this shouldn't land before we fix that other bug, since this alone would lead to rather odd inconsistent behavior, IMO.

Comment on attachment 8634472 [details] [diff] [review] Bug1182551-07-15-15.patch It might make sense at some point to reorganize the code a bit so that one would collect the right flags to some variable and then just call eventSink->OnSecurityChange in one place. >+ uint32_t State = nsIWebProgressListener::STATE_IS_BROKEN; camelCase. so either state, or defaultState or originalState or something. Some tests would be great, I guess once the other bug is fixed too.

To fix nsSecureBrowserUIImpl, I can take the docshell and get the current URI associaed with it. If the URI is https, set aSTATE to STATE_IS_BROKEN and then append the necessary mixed content states to it as we do here[1]. If the URI is not https, set aSTATE to STATE_IS_INSECURE and then append the necessary mixed content states to it. David, what do you think? Are you okay with this approach? [1] https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsSecureBrowserUIImpl.cpp#266

(In reply to Tanvi Vyas [:tanvi] from comment #9) > To fix nsSecureBrowserUIImpl, I can take the docshell and get the current > URI associaed with it. If the URI is https, set aSTATE to STATE_IS_BROKEN > and then append the necessary mixed content states to it as we do here[1]. > If the URI is not https, set aSTATE to STATE_IS_INSECURE and then append the > necessary mixed content states to it. > > David, what do you think? Are you okay with this approach? > > [1] > https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/ > nsSecureBrowserUIImpl.cpp#266 That might work. One interesting thing I discovered is that with this patch applied, it appears to work as expected in e10s. That is, if you open the link in comment 6, open a new tab, and then go back to the first tab, it shows the correct icon rather than the lock with triangle. That might be a good place to start debugging that issue.

Tracking for 42, see Comment 4. Would be good to get a fix for this in 42.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #10) > That might work. One interesting thing I discovered is that with this patch > applied, it appears to work as expected in e10s. That is, if you open the > link in comment 6, open a new tab, and then go back to the first tab, it > shows the correct icon rather than the lock with triangle. That might be a > good place to start debugging that issue. In e10s when you switch back to the original tab, MapExternaltoIntenral isn't called. while in non-e10s it is and it changes the security state. I haven't gone through and seen what calls MapExternaltoInternal in the non-e10s case to see what the discrepancy there is. Nevertheless, printing security states in both Mixed Content Blocker and nsSecureBrowserUIImpl, it is clear we are setting the wrong security state here for http pages and it needs to be fixed: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsSecureBrowserUIImpl.cpp#266

This fixes the wrong icon that shows up after you switch tabs. Instead of checking the URI of the root docShell, I decided to trust the lock lis_no_security state. Since "http" pages aren't the only ones we might consider STATE_IS_INSECURE. But the again, I'm not familiar how nsSecureBrowserUIImpl determines this state or how much we should rely on it. What do you think? Stick with lis_no_security or check the root docShell URI for http?

Comment on attachment 8641935 [details] [diff] [review] Bug1182551-nsSecureBrowserUIImpl.patch Review of attachment 8641935 [details] [diff] [review]: ----------------------------------------------------------------- This looks like the right thing to do here. Can we add a test?

Rebased. Added a few more mcb tags to mochitests. Changed the iframe src to point to a different domain than the top level page.

Comment on attachment 8646504 [details] [diff] [review] Bug1182551-tests-08-11-15.patch Review of attachment 8646504 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I had a few comments. I feel like this should be reviewed by a browser peer, though. ::: browser/base/content/test/general/browser_mixedContentFramesOnHttp.js @@ +2,5 @@ > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + * > + * Tests for Bug 1182551 - > + * HTTP pages with HTTPS mixed passive content frames have the wrong security Rather than stating what the bug was, I think it's better to state what the behavior should be and how this test ensures that (which you do later, so I would just remove this bit). @@ +6,5 @@ > + * HTTP pages with HTTPS mixed passive content frames have the wrong security > + * state; they should be STATE_IS_INSECURE and not STATE_IS_BROKEN. > + * Using STATE_IS_BROKEN results in showing a lock icon with a yellow yield > + * sign on an HTTP page. > + * nit: some trailing spaces in this comment block @@ +48,5 @@ > + SecStateTestsCompleted(); > +} > + > +// Compares the security state of the page with what is expected > +function isSecurityState(expectedState) { Looks like there's already a function like this in browser_mixedContentFromOnunload.js - can we factor it out? ::: dom/security/test/mixedcontentblocker/mochitest.ini @@ +12,5 @@ > file_main_bug803225_websocket_wsh.py > file_server.sjs > > [test_main.html] > +tags = mcb If all of the tests in this folder are mcb, I think you can add the tag for all of them once at the top of the file instead of once for each test.

Addressed keeler's comments and flagging Tim for review. Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=269bea575b0f

Comment on attachment 8647318 [details] [diff] [review] Bug1182551-tests-08-12-15.patch Review of attachment 8647318 [details] [diff] [review]: ----------------------------------------------------------------- In the future, we should convert those tests to use add_task() and BrowserTestUtils, and make them e10s-friendly (don't use contentWindow etc.). Maybe file a bug to update all mcb mochitest-browser tests at once when we find the time to do so? ::: browser/base/content/test/general/browser_mixedContentFramesOnHttp.js @@ +23,5 @@ > + > +function test() { > + waitForExplicitFinish(); > + SpecialPowers.pushPrefEnv({"set": [["security.mixed_content.block_active_content", true], > + ["security.mixed_content.block_display_content", false]]}, SecStateTests); Nit: we could indent this better: SpecialPowers.pushPrefEnv({"set": [ ["security.mixed_content.block_active_content", true], ["security.mixed_content.block_display_content", false] ]}, SecStateTests); @@ +32,5 @@ > + gTestBrowser = gBrowser.selectedBrowser; > + > + whenLoaded(gTestBrowser, SecStateTest1); > + let url = gHttpTestRoot + "file_mixedContentFramesOnHttp.html"; > + gTestBrowser.contentWindow.location = url; Think we should do: function SecStateTests() { let url = gHttpTestRoot + "file_mixedContentFramesOnHttp.html"; gBrowser.selectedTab = gBrowser.addTab(url); gTestBrowser = gBrowser.selectedBrowser; whenLoaded(gTestBrowser, SecStateTest1); }

Fixed nits. Carrying over r+. Try looks good so I'm going to land. (In reply to Tim Taubert [:ttaubert] (on PTO, back Aug 31st) from comment #20) > Comment on attachment 8647318 [details] [diff] [review] > Bug1182551-tests-08-12-15.patch > > Review of attachment 8647318 [details] [diff] [review]: > ----------------------------------------------------------------- > > In the future, we should convert those tests to use add_task() and > BrowserTestUtils, and make them e10s-friendly (don't use contentWindow > etc.). Maybe file a bug to update all mcb mochitest-browser tests at once > when we find the time to do so? > I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1194475. > Think we should do: > > function SecStateTests() { > let url = gHttpTestRoot + "file_mixedContentFramesOnHttp.html"; > gBrowser.selectedTab = gBrowser.addTab(url); > gTestBrowser = gBrowser.selectedBrowser; > whenLoaded(gTestBrowser, SecStateTest1); Followed by: gTestBrowser.contentWindow.location = url; > }

Changed "State" to "state" per: (In reply to Olli Pettay [:smaug] (vacation Aug 11 - Aug 18) from comment #8) > >+ uint32_t State = nsIWebProgressListener::STATE_IS_BROKEN; > camelCase. so either state, or defaultState or originalState or something. > Carrying over r+.

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d8f8d2d9c0b2c874738c1b8ad2ae51019defa3 changeset: e8d8f8d2d9c0b2c874738c1b8ad2ae51019defa3 user: Tanvi Vyas <tanvi@mozilla.com> date: Thu Aug 13 17:13:43 2015 -0700 description: Bug 1182551 - Don't set STATE_IS_BROKEN on HTTP pages when mixed content is allowed by default. r=smaug url: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8a664fd55b6d1faf6b088c90706d5c2e4a59ec changeset: 7d8a664fd55b6d1faf6b088c90706d5c2e4a59ec user: Tanvi Vyas <tanvi@mozilla.com> date: Thu Aug 13 17:13:48 2015 -0700 description: Bug 1182551 - Updating nsSecureBrowserUIImpl so that insecure pages with mixed content iframes don't get marked as broken. r=keeler url: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ab71814ee94ab9d872fdb8a7ef0fe3e31a23ad changeset: a0ab71814ee94ab9d872fdb8a7ef0fe3e31a23ad user: Tanvi Vyas <tanvi@mozilla.com> date: Thu Aug 13 17:13:51 2015 -0700 description: Bug 1182551 - HTTP top level page with HTTPS mixed passive frame should have STATE_IS_INSECURE. r=ttaubert

Comment on attachment 8641935 [details] [diff] [review] Bug1182551-nsSecureBrowserUIImpl.patch Approval Request Comment [Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=1175678 [User impact if declined]: In some cases, users will see a lock in the url bar for an HTTP page. [Describe test coverage new/current, TreeHerder]: mochitest included, tree looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c33c9be1ec2 [Risks and why]: Changes to nsSecureBrowserUIImpl and nsMixedContentBlocker. Risk is to the security UI. We can let this sit on nightly and then uplift if that helps reduce the risk. [String/UUID change made/needed]: None

Comment on attachment 8647742 [details] [diff] [review] Bug1182551-tests-08-13-15.patch Approval Request Comment [Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=1175678 [User impact if declined]: In some cases, users will see a lock in the url bar for an HTTP page. [Describe test coverage new/current, TreeHerder]: mochitest included, tree looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c33c9be1ec2 [Risks and why]: Changes to nsSecureBrowserUIImpl and nsMixedContentBlocker. Risk is to the security UI. We can let this sit on nightly and then uplift if that helps reduce the risk. [String/UUID change made/needed]: None

Comment on attachment 8647749 [details] [diff] [review] Bug1182551-mcb-08-13-15.patch Approval Request Comment [Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=1175678 [User impact if declined]: In some cases, users will see a lock in the url bar for an HTTP page. [Describe test coverage new/current, TreeHerder]: mochitest included, tree looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c33c9be1ec2 [Risks and why]: Changes to nsSecureBrowserUIImpl and nsMixedContentBlocker. Risk is to the security UI. We can let this sit on nightly and then uplift if that helps reduce the risk. [String/UUID change made/needed]: None

(In reply to Tanvi Vyas [:tanvi] from comment #26) > [Risks and why]: Changes to nsSecureBrowserUIImpl and nsMixedContentBlocker. > Risk is to the security UI. We can let this sit on nightly and then uplift > if that helps reduce the risk. Should say "We can let this sit on nightly for a couple of days and then uplift"

https://hg.mozilla.org/mozilla-central/rev/e8d8f8d2d9c0 https://hg.mozilla.org/mozilla-central/rev/7d8a664fd55b https://hg.mozilla.org/mozilla-central/rev/a0ab71814ee9

Comment on attachment 8641935 [details] [diff] [review] Bug1182551-nsSecureBrowserUIImpl.patch Has been in nighly for a few days, has tests, beginning of the aurora cycle, taking it.

Marking this as checkin-needed for aurora.

You don't need to set checkin-needed for uplifts. We have separate bug queries for them.

https://hg.mozilla.org/releases/mozilla-aurora/rev/360c90e4a9cc https://hg.mozilla.org/releases/mozilla-aurora/rev/1d051fc00be9 https://hg.mozilla.org/releases/mozilla-aurora/rev/e552f33beec4