838396 - Not setting hasMixedDisplayContentLoaded and hasMixedDisplayContentBlocked flag in nsMixedContentBlocker.cpp

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Attachment: Missed setting hasMixedDisplayContentBlocked

We are missing a call to SetHasMixedDisplayContentBlocked in nsMixedContentBlocker.cpp.  Missing this shouldn't really cause any issues today, since we aren't doing much with this flag and it doesn't have a UI impact today.  But, it is supposed to be set when mixed display content is blocked, so here is a patch that does so.

Comment 1

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

Comment on attachment 710445 [details] [diff] [review]
Missed setting hasMixedDisplayContentBlocked

Can we have a test for this (so that we catch possible regressions)-

Comment 3

[Tanvi Vyas[:tanvi]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b5262e61b9af

I need to add a test.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Tanvi Vyas [:tanvi] from comment #0)
> We are missing a call to SetHasMixedDisplayContentBlocked in
> nsMixedContentBlocker.cpp.  Missing this shouldn't really cause any issues
> today, since we aren't doing much with this flag and it doesn't have a UI
> impact today.  But, it is supposed to be set when mixed display content is
> blocked, so here is a patch that does so.

This flag is now used in nsSecureBrowserUIImpl to help calculate the UI state. The easiest way to test this may be to have a test if an <img src=x.png>, where the server returns a 404 response for x.png. In that case, I nsMixedContentBlocker will rightly consider the page  to have mixed content, bug the old nsISecureBrowserUIImpl calculations will, I think, wrongly consider the page to be mixed-content free.

Comment 5

[Tanvi Vyas[:tanvi]]

We forget to set the hasMixedDispalyContentBlocked the following case:

* User has explicitly enabled mixed display content blocking in about:config by setting security.mixed_content.block_display_content to true.
* User visits a page with mixed display content
* The mixed display content load is rejected by nsMixedContetnBlocker.cpp:462, but we never set the hasMixedDisplayContentBlocked flag.

We need a rootDoc->SetHasMixedDisplayContentBlocked(true); at line 465.  The patch does this (but might not apply cleanly because of change to the file).

You can test this by checking the value of hasMixedDisplayContentBlocked, which is exposed in this idl:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#510


Also, this bug has an implication I didn't realize before.  We are under-reporting telemetry for pages with mixed display content because we forgot to set this flag.  However, the percentage of under-reporting is VERY low since it's only for users who gone to about:config and set the pref to block mixed display.  If I had to guess, I would say this would be on the order of 100 users.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#1452

Comment 6

[Tanvi Vyas[:tanvi]]

While looking at this more carefully, I realized we also forget to set hasMixedDisplayContentLoaded() in the following case:

* User has explicitly enabled mixed display content blocking in about:config by setting security.mixed_content.block_display_content to true.
* User visits a page with BOTH mixed active & mixed display content.  The user clicks the shield and "disables protection".
* The mixed display content load is hence accepted by nsMixedContetnBlocker.cpp:456, but we never set the hasMixedDisplayContentLoaded flag.


Fix:
We need a rootDoc->SetHasMixedDisplayContentLoaded(true); at line 459.

Implications: As in the previous case, there are no UI implications to missing this flag.  Thi is because this is a case where there is both mixed display and mixed active content.  Hence, the UI would look the same as if there were just mixed active content anyway (yellow triangle).

Again, there are telemetry implications where we are under-reporting.  But this situation is even more rare than in the previous comment, so I"m not expecting any spikes in the mixed display content telemetry.

Test:
To test this, we can check the value of hasMixedDisplayContentLoaded after the user "disables protection on this page".  This is exposed in this idl:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#503

Comment 7

[Ivan Alagenchev :ialagenchev]

Attachment: 838396.patch

I am uploading a WIP patch, since I am running into two issues that I can't seem to figure out on my own. The first one is that these checks are failing for some reason after clicking on the doorhanger to allow mixed content:
+  is(browser.docShell.hasMixedDisplayContentLoaded, true, "hasMixedDisplayContentLoaded flag has not been set");
+  is(browser.docShell.hasMixedActiveContentLoaded, true, "hasMixedActiveContentLoaded flag has not been set");

I stepped through the code in nsMixedContentBlocker and the flags are getting set correctly. The checks before clicking on the doorhanger work fine too. 

The second issue is that when putting the tests in browser/devtools/webconsole/test and running them from there, they run fine. When I try to put them in browser/base/content/test then nothing really happens. I've added both files to the Makefile.in and did mach build.

Can I please get some quick pointers about what's going on here? I asked about the first issue on #developers without success.
Thanks a lot!

Comment 8

[Boris Zbarsky [:bzbarsky]]

So this happens:

>+  notification.secondaryActions[0].callback();

and that starts a new (async) load, right?

But the flags are checked immediately, before that load completes.  Is that the basic problem?

I have no idea what's going on with the test dir issue.

Comment 9

[Ivan Alagenchev :ialagenchev]

(In reply to Boris Zbarsky [:bz] from comment #8)
> So this happens:
> 
> >+  notification.secondaryActions[0].callback();
> 
> and that starts a new (async) load, right?
> 
> But the flags are checked immediately, before that load completes.  Is that
> the basic problem?
Oh yes! I knew it was something simple I was missing. 

> I have no idea what's going on with the test dir issue.
I'll try to ask around on #developers again.

Thanks bz!

Comment 10

[Ivan Alagenchev :ialagenchev]

Attachment: 838396.patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 796334 [details] [diff] [review]
838396.patch

r=me

Comment 12

[Ivan Alagenchev :ialagenchev]

Comment on attachment 796334 [details] [diff] [review]
838396.patch

Tanvi asked me to request a review from smaug too, since he's familiar with the MCB code.

Comment 13

[Ivan Alagenchev :ialagenchev]

Attachment: 838396.patch

Comment 14

[Ivan Alagenchev :ialagenchev]

Attachment: 838396.patch

Comment 15

[Ivan Alagenchev :ialagenchev]

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=4bea3029c17b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e615c2a302

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e3e615c2a302