Closed Bug 1082947 Opened 5 years ago Closed 5 years ago

Bugs in security state setting in nsMixedContentBlocker

Categories

(Core :: Security, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: tanvi, Assigned: reuseme2600)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
We should review the rest of the file for security state changes as well.
(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
Attached patch Bug1082947-10-22-14.patch (obsolete) — Splinter Review
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)
Attachment #8509900 - Flags: review?(bugs)
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.
Attachment #8509900 - Flags: review?(bugs) → review+
Remind me why we need to use nsIWebProgressListener::STATE_IS_BROKEN 
even if allowMixedContent is true. Question applies to eMixedDisplay and eMixedScript cases.
Hmm, I guess that is needed for the UI.
Attached patch Bug1082947-10-23-14.patch (obsolete) — Splinter Review
Cleaned up a bunch of indentation.  Carrying over r+.
Attachment #8509900 - Attachment is obsolete: true
Attachment #8510527 - Flags: review+
Attached patch Bug1082947-10-23-14.patch (obsolete) — Splinter Review
And fixed some trailing whitespace.
Attachment #8510527 - Attachment is obsolete: true
Attachment #8510559 - Flags: review+
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.
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
Updated patch that accounts for the move from /content/base/src/nsMixedContentBlocker to /dom/security/nsMixedContentBlocker.  Carrying over r+.
Attachment #8510559 - Attachment is obsolete: true
Attachment #8582486 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b340b048cd80
Assignee: nobody → tvyas
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.