1082947 - Bugs in security state setting in nsMixedContentBlocker

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#589

* If the pref for blocking mixed display is set to false, and there is mixed display content that has loaded on the page, the nsIWebProgressListner flag STATE_LOADED_MIXED_DISPLAY_CONTENT is not set.

* If the pref for blocking mixed script is set to false, and there is mixed script content that has loaded on the page, the nsIWebProgressListner flag STATE_LOADED_MIXED_SCRIPT_CONTENT is not set.


http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#520

* If the pref to block mixed display content is set to true and the user overrides the blocking on a page, STATE_LOADED_MIXED_SCRIPT_CONTENT might not get set because we do:
rootDoc->SetHasMixedActiveContentLoaded(true);
Later, we return early if HasMixedActiveContentLoaded is true, so we never get a chance to update the state:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#545

* Also, if the pref to block mixed display is set to true and the user overrides the blocking on a page, we don't update the security state here (we use the existing state):
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#526
Note that when we do update this, we will have to check the active loaded/blocked state and set that flag appropriately to (the way we do for display content here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#553)


The only "big deal" issue in all of these is the last one, since we do need to make sure we appropriately set STATE_IS_SECURE vs STATE_IS_BROKEN.

Comment 1

[Tanvi Vyas[:tanvi]]

We should review the rest of the file for security state changes as well.

Comment 2

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #0)
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsMixedContentBlocker.cpp#589
> 
> * If the pref for blocking mixed display is set to false, and there is mixed
> display content that has loaded on the page, the nsIWebProgressListner flag
> STATE_LOADED_MIXED_DISPLAY_CONTENT is not set.
> 
> * If the pref for blocking mixed script is set to false, and there is mixed
> script content that has loaded on the page, the nsIWebProgressListner flag
> STATE_LOADED_MIXED_SCRIPT_CONTENT is not set.
> 
> 
This part is not true.  These flags do get set as part of http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp?rev=c7eccf7c09f1#56

Comment 3

[Tanvi Vyas[:tanvi]]

Attachment: Bug1082947-10-22-14.patch

Comment 4

[Tanvi Vyas[:tanvi]]

Comment on attachment 8509900 [details] [diff] [review]
Bug1082947-10-22-14.patch

Since the line numbers in the original post have changed, here is an explanation of what this patch fixes with links that won't change:

* If the pref to block mixed display content is set to true and the user overrides the blocking on a page, STATE_LOADED_MIXED_SCRIPT_CONTENT might not get set because we do:
rootDoc->SetHasMixedActiveContentLoaded(true); - http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp?rev=c7eccf7c09f1#523
Later, we return early if HasMixedActiveContentLoaded is true, so we never get a chance to update the state:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp?rev=c7eccf7c09f1#545

* Also, if the pref to block mixed display is set to true and the user overrides the blocking on a page, we don't update the security state here (we use the existing state):
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp?rev=c7eccf7c09f1#526
Note that when we do update this, we will have to check the active loaded/blocked state and set that flag appropriately to (the way we do for display content here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp?rev=c7eccf7c09f1#553)

Comment 5

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

Comment on attachment 8509900 [details] [diff] [review]
Bug1082947-10-22-14.patch

This really needs some tests.
 
>   // If the content is display content, and the pref says display content should be blocked, block it.
>   if (sBlockMixedDisplay && classification == eMixedDisplay) {
>     if (allowMixedContent) {
>       LogMixedContentMessage(classification, aContentLocation, rootDoc, eUserOverride);
>       *aDecision = nsIContentPolicy::ACCEPT;
>-      rootDoc->SetHasMixedActiveContentLoaded(true);
>-      if (!rootDoc->GetHasMixedDisplayContentLoaded() && NS_SUCCEEDED(stateRV)) {
>-        rootDoc->SetHasMixedDisplayContentLoaded(true);
>-        eventSink->OnSecurityChange(aRequestingContext, (State | nsIWebProgressListener::STATE_LOADED_MIXED_DISPLAY_CONTENT));
>+      // See if mixed display content has already loaded on the page or if the state needs to be updated here.
>+      // If mixed display hasn't loaded previously, then we need to call OnSecurityChange() to update the UI.
>+      if (rootDoc->GetHasMixedDisplayContentLoaded()) {
>+        return NS_OK;
>+      }
>+      rootDoc->SetHasMixedDisplayContentLoaded(true);
>+
>+      if (rootHasSecureConnection) {
>+        if (rootDoc->GetHasMixedActiveContentLoaded()) {
>+          // If mixed active content is loaded, make sure to include that in the state.
>+          eventSink->OnSecurityChange(aRequestingContext, (nsIWebProgressListener::STATE_IS_BROKEN | 
>+          nsIWebProgressListener::STATE_LOADED_MIXED_DISPLAY_CONTENT | 
>+          nsIWebProgressListener::STATE_LOADED_MIXED_ACTIVE_CONTENT));
Odd looking indentation. The states could be aligned.
Looks like the similar code inside } else if (sBlockMixedScript && classification == eMixedScript) {
uses same a bit odd indentation.

Comment 6

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

Remind me why we need to use nsIWebProgressListener::STATE_IS_BROKEN 
even if allowMixedContent is true. Question applies to eMixedDisplay and eMixedScript cases.

Comment 7

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

Hmm, I guess that is needed for the UI.

Comment 8

[Tanvi Vyas[:tanvi]]

Attachment: Bug1082947-10-23-14.patch

Cleaned up a bunch of indentation.  Carrying over r+.

Comment 9

[Tanvi Vyas[:tanvi]]

Attachment: Bug1082947-10-23-14.patch

And fixed some trailing whitespace.

Comment 10

[Tanvi Vyas[:tanvi]]

Steps:
Block mixed display content in about:config and go to https://people.mozilla.org/~tvyas/mixedboth2.html.  Disable protection.

Looking at the code in release, this is what should happen:
1) We load a mixed content image first
2) mcb will set HasMixedActiveContentLoaded to true without updating the security state to indicate that mixed active content has loaded
3) mcb will update the security state to indicate that mixed display content is loaded, but won't change it from STATE_IS_SECURE to STATE_IS_BROKEN (i.e. we keep the lock)
4) then we load a mixed content script
5) we have already HasMixedActiveContentLoaded to true, so security state shouldn’t get updated (i.e. we don't show a yellow triangle)

We should see a lock in the address bar.  Not a yellow triangle or even a grey one.  But we do see the yellow triangle.

Going through a debugger, I see that we go through 1-3 in mcb.  Then on the next call to mcb, I check the state and it's set to 545 = 0x221.  This indicates mixed display loaded, mixed active loaded, and state is broken.  Something else is checking the flags and updating the state for us - 
nsSecureBrowserUIImpl here http://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#263

Hence, I could write a mochitest for this, but it wouldn't fail without this patch.

Comment 11

[Tanvi Vyas[:tanvi]]

Spoke to Olli and he said this is okay without a test, given comment 10.  nsSecureBrowserUIImpl fixes the mistake in nsMixedContentBlocker, but we should still land this patch and fix nsMixedContentBrowserUIImpl's flawed logic in the case that the user has blocked mixed display content.

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

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: Bug1082947-03-19-15.patch

Updated patch that accounts for the move from /content/base/src/nsMixedContentBlocker to /dom/security/nsMixedContentBlocker.  Carrying over r+.

Comment 13

[Tanvi Vyas[:tanvi]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b340b048cd80

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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