1182551 - don't show the mixed display content indicator if the top level load is http://

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 1

[Tanvi Vyas[:tanvi]]

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.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 3

[Tanvi Vyas[:tanvi]]

Needinfo'ing myself to look at this more closely.

Comment 4

[Tanvi Vyas[:tanvi]]

[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.

Comment 5

[Tanvi Vyas[:tanvi]]

I'm working on a patch for this.

Comment 6

[Tanvi Vyas[:tanvi]]

Attachment: Bug1182551-07-15-15.patch

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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Tanvi Vyas[:tanvi]]

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

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(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.

Comment 11

[Kate Glazko]

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

Comment 12

[Tanvi Vyas[:tanvi]]

(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

Comment 13

[Tanvi Vyas[:tanvi]]

Attachment: Bug1182551-nsSecureBrowserUIImpl.patch

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 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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?

Comment 15

[Tanvi Vyas[:tanvi]]

Attachment: Bug1182551-tests-08-10-15.patch

Comment 16

[Tanvi Vyas[:tanvi]]

Attachment: Bug1182551-tests-08-11-15.patch

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 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 18

[Tanvi Vyas[:tanvi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c33c9be1ec2

Comment 19

[Tanvi Vyas[:tanvi]]

Attachment: Bug1182551-tests-08-12-15.patch

Addressed keeler's comments and flagging Tim for review.

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=269bea575b0f

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

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);
}

Comment 21

[Tanvi Vyas[:tanvi]]

Attachment: Bug1182551-tests-08-13-15.patch

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;

> }

Comment 22

[Tanvi Vyas[:tanvi]]

Attachment: Bug1182551-mcb-08-13-15.patch

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+.

Comment 23

[Tanvi Vyas[:tanvi]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d8f8d2d9c0b2c874738c1b8ad2ae51019defa3
changeset:  e8d8f8d2d9c0b2c874738c1b8ad2ae51019defa3
user:       Tanvi Vyas <[e-mail]>
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 <[e-mail]>
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 <[e-mail]>
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 24

[Tanvi Vyas[:tanvi]]

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 25

[Tanvi Vyas[:tanvi]]

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 26

[Tanvi Vyas[:tanvi]]

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

Comment 27

[Tanvi Vyas[:tanvi]]

(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"

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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 29

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 30

[Tanvi Vyas[:tanvi]]

Marking this as checkin-needed for aurora.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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