Closed Bug 834836 Opened 11 years ago Closed 11 years ago

Turn on pref to block mixed active content

Categories

(Firefox :: Security, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
firefox23 + verified

People

(Reporter: tanvi, Assigned: tanvi)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, sec-want, site-compat)

Attachments

(5 files, 6 obsolete files)

1.33 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
12.79 KB, patch
tanvi
: review+
Details | Diff | Splinter Review
850 bytes, patch
briansmith
: review+
Details | Diff | Splinter Review
12.06 KB, patch
tanvi
: review+
Details | Diff | Splinter Review
2.02 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Set "security.mixed_content.block_active_content" to true by default (instead of false). 

Trying to do this on try shows lots of oranges because of tests that expect the mixed content to load (mixed content tests, onsecurity change tests, csp test times out, local storage test, web console mixed content entry test, etc).  This bug is to update these tests and turn the pref on.

https://tbpl.mozilla.org/?tree=Try&rev=7ff3f12543ac
Depends on: 822366, 822367, 822371
Assignee: nobody → tanvi
Comment on attachment 706531 [details] [diff] [review]
Set "security.mixed_content.block_active_content" to true

Review of attachment 706531 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz on this patch when you're ready to land (i.e. regressions/broken tests are fixed)
Depends on: 837075, 836951, 836630, 781018
Target Milestone: --- → Firefox 21
Depends on: 837351
Attachment #712016 - Flags: review?(dtownsend+bugmail)
Next I have to update all the actual mixed content tests.
Comment on attachment 712016 [details]
Updated browser_discovery test since it does it's own mixed content handling v1

Spoke to Dave on IRC and decided that the 2 insecure content tests in browser_discovery should be run with the flag set to false and the flag set to true.  Will update the patch.
Attachment #712016 - Flags: review?(dtownsend+bugmail)
Comment on attachment 712013 [details] [diff] [review]
Updated CSP test so that it doesn't break when we block mixed active content v1

Review of attachment 712013 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  r=me on this test tweak.
Attachment #712013 - Flags: review?(sstamm) → review+
Attachment #712011 - Flags: review?(bugs) → review+
Comment on attachment 712011 [details] [diff] [review]
Updated /dom tests so that they don't break when we block mixed active content v1

This patch actually also includes a change to a /browser/devtools test.  So asking additional review from Mihai for just the browser/devtools/webconsole/test/browser_webconsole_bug_737873_mixedcontent.js test.  

Sorry for missing this and not splitting this into separate patches earlier!
Attachment #712011 - Flags: review?(mihai.sucan)
Work in progress patch for the mixed content tests.  I can't seem to run these locally (not sure why??), but have pushed to try - https://tbpl.mozilla.org/?tree=Try&rev=b24426a48ce6

There is one tests that causes some problems when I try to set and then unset the block active and block display tests:
https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/mixedcontent/

This test navigates from one page to another, and hence finish() isn't called on the first page, and the original blockDisplay and blockActive prefs aren't available on the last page.  The value of the prefs actually doesn't matter for that test and hence we can update mixedContentTest.js to ignore it.  Right now, I use the fact that it is the only test that sets bypassNavigationTests to true to determine this.  I can instead use a new flag that is specific to this test if that is preferable.

Feedback? to bsmith and dkeeler.
Attachment #712642 - Flags: feedback?(dkeeler)
Attachment #712642 - Flags: feedback?(bsmith)
Attachment #706531 - Flags: review?(dveditz)
Comment on attachment 712642 [details] [diff] [review]
Update mixed content tests so that they don't break when we block mixed active content v1

Review of attachment 712642 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like this just caches the values of the mixed content prefs, so if that's all that's needed, then I think this is fine.

::: security/manager/ssl/tests/mochitest/mixedcontent/mixedContentTest.js
@@ +27,5 @@
>  
>  // Internal variables
>  var _windowCount = 0;
> +var blockDisplay;
> +var blockActive;

Maybe call these _savedBlockDisplayPref, _savedBlockActivePref? (underscore for consistency, "saved...Pref" to be a bit more clear about what they're used for)
Also, I would initialize them to null since they get specifically compared with null later.
Attachment #712642 - Flags: feedback?(dkeeler) → feedback+
Comment on attachment 706531 [details] [diff] [review]
Set "security.mixed_content.block_active_content" to true

Review of attachment 706531 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/src/init/all.js
@@ +1384,5 @@
>  pref("security.csp.enable", true);
>  pref("security.csp.debug", false);
>  
>  // Mixed content blocking
> +pref("security.mixed_content.block_active_content", true);

This will cause mixed active content to be blocked in B2G, Fennec, and Thunderbird too, where there is no UI to unblock it. Is that the desired intent?
Comment on attachment 706531 [details] [diff] [review]
Set "security.mixed_content.block_active_content" to true

(In reply to Brian Smith (:bsmith) from comment #11)
> Comment on attachment 706531 [details] [diff] [review]
> Set "security.mixed_content.block_active_content" to true
> 
> Review of attachment 706531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpref/src/init/all.js
> @@ +1384,5 @@
> >  pref("security.csp.enable", true);
> >  pref("security.csp.debug", false);
> >  
> >  // Mixed content blocking
> > +pref("security.mixed_content.block_active_content", true);
> 
> This will cause mixed active content to be blocked in B2G, Fennec, and
> Thunderbird too, where there is no UI to unblock it. Is that the desired
> intent?

No.  Thanks for catching this.  I need to move this to firefox.js
Attachment #706531 - Flags: review?(dveditz)
Depends on: 840388
Comment on attachment 712016 [details]
Updated browser_discovery test since it does it's own mixed content handling v1

We need to update the extensions code to detect when Mixed Content is blocked.  This is now being handled in bug 840388.  Obsoleting this patch; the test shouldn't require any changes after all.
Attachment #712016 - Attachment is obsolete: true
Attachment #712016 - Attachment is patch: false
Comment on attachment 712011 [details] [diff] [review]
Updated /dom tests so that they don't break when we block mixed active content v1

Patch looks good. At the end you can use clearUserPref() to reset back to the default value, instead of setBoolPref(). Also, instead of having a new endTest() function you could registerCleanupFunction(function() { ... clearUserPref() ... }). This is just for future reference - you can land this patch as-is.

Thanks for keeping me in the loop withe web console-related changes.
Attachment #712011 - Flags: review?(mihai.sucan) → review+
Depends on: 840641
(In reply to David Keeler (:keeler) from comment #10)
> Comment on attachment 712642 [details] [diff] [review]
> Update mixed content tests so that they don't break when we block mixed
> active content v1
> 
> Review of attachment 712642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like this just caches the values of the mixed content prefs, so if
> that's all that's needed, then I think this is fine.
> 
> ::: security/manager/ssl/tests/mochitest/mixedcontent/mixedContentTest.js
> @@ +27,5 @@
> >  
> >  // Internal variables
> >  var _windowCount = 0;
> > +var blockDisplay;
> > +var blockActive;
> 
> Maybe call these _savedBlockDisplayPref, _savedBlockActivePref? (underscore
> for consistency, "saved...Pref" to be a bit more clear about what they're
> used for)
> Also, I would initialize them to null since they get specifically compared
> with null later.

Looks like I use origBlockActive in other tests, so I will switch the variable name to origBlockActive in all of the tests in this bug too.
Carrying over r+ from Mihai and Olli.

Changed variable names from blockActive and blockDisplay to origBlockActive and origBlockDisplay.
Attachment #712011 - Attachment is obsolete: true
Attachment #713061 - Flags: review+
Update 9 /mixedcontent tests that depend on mixed active content to load in order to test the security state of a page.

security/manager/ssl/tests/mochitest/mixedcontent/test_bug329869.html
security/manager/ssl/tests/mochitest/mixedcontent/test_documentWrite2.html
security/manager/ssl/tests/mochitest/mixedcontent/test_dynDelayedUnsecureXHR.html
security/manager/ssl/tests/mochitest/mixedcontent/test_dynUnsecureIframeRedirect.html
security/manager/ssl/tests/mochitest/mixedcontent/test_unsecureCSS.html
security/manager/ssl/tests/mochitest/mixedcontent/test_unsecureIframe.html
security/manager/ssl/tests/mochitest/mixedcontent/test_unsecureIframe2.html
security/manager/ssl/tests/mochitest/mixedcontent/test_unsecureIframeRedirect.html
security/manager/ssl/tests/mochitest/mixedcontent/test_unsecurePictureInIframe.html
security/manager/ssl/tests/mochitest/mixedcontent/test_unsecureRedirect.html
Attachment #712642 - Attachment is obsolete: true
Attachment #712642 - Flags: feedback?(bsmith)
Attachment #713063 - Flags: review?(bsmith)
Attachment #706531 - Attachment is obsolete: true
Attachment #713069 - Flags: review?(dveditz)
Pushed all of these to try, along with the patch to bug 840388 that handles mixed content on the about:addons page.

https://tbpl.mozilla.org/?tree=Try&rev=ac63fceb7e30
Comment on attachment 713063 [details] [diff] [review]
Update mixed content tests so that they don't break when we block mixed active content v2

Tanvi, I think this is the wrong patch. All this stuff is in dom/ and devtools.
Attachment #713063 - Flags: review?(bsmith)
(In reply to Brian Smith (:bsmith) from comment #20)
> Comment on attachment 713063 [details] [diff] [review]
> Update mixed content tests so that they don't break when we block mixed
> active content v2
> 
> Tanvi, I think this is the wrong patch. All this stuff is in dom/ and
> devtools.

Sorry, added the wrong patch.  Updated with the correct patch.
Attachment #713132 - Flags: review?(bsmith)
Attachment #713063 - Attachment is obsolete: true
Comment on attachment 713132 [details] [diff] [review]
Update mixed content tests so that they don't break when we block mixed active content v3

Review of attachment 713132 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/mochitest/mixedcontent/mixedContentTest.js
@@ +69,5 @@
>  
> +    if (hasMixedActiveContent)
> +    {
> +      origBlockActive = SpecialPowers.getBoolPref("security.mixed_content.block_active_content")
> +      SpecialPowers.setBoolPref("security.mixed_content.block_active_content", false);

SpecialPowers.pushPrefEnv?

@@ +96,5 @@
>        {
>          if (testCleanUp)
>            testCleanUp();
> +        if (hasMixedActiveContent)
> +          SpecialPowers.setBoolPref("security.mixed_content.block_active_content", origBlockActive);

SpecialPowers.popPrefEnv?
Attachment #713132 - Flags: review?(bsmith) → review+
Updated with bsmith's review comments.  Carrying over r+ and did another push to try, but limited and only re-testing mochitest-5 where the mixed content tests exist:

https://tbpl.mozilla.org/?tree=Try&rev=2cd8832b34c4
Attachment #713132 - Attachment is obsolete: true
Attachment #713208 - Flags: review+
Comment on attachment 713069 [details] [diff] [review]
Set "security.mixed_content.block_active_content" to true for desktop Firefox

Stealing review from Dan, as Dan already r+d enabling this before.
Attachment #713069 - Flags: review?(dveditz) → review+
Needed a new push to try anyway; cancelling the previous ones: https://tbpl.mozilla.org/?tree=Try&rev=f0a382ea2080
Is the intent to land this before the uplift, or at the beginning of the Firefox 22 cycle?

There has been a lot of churn in this feature to get it working, and things like bug 840641 make me think it's not had any significant usage on Nightly (by users manually turning it on). Ergo we don't know if this is likely to cause breakage and need more fixes (which would be undesirable to be doing directly in Aurora).

I don't think this should squeak in at the end of a cycle, it should get as much of a full Nightly cycle as possible.
Moving target milestone to FF22 because there are still some dependency bugs that need to be fixed.
Target Milestone: Firefox 21 → Firefox 22
Pushed to try, along with the patches in Bug 840388 and Bug 836951:
https://tbpl.mozilla.org/?tree=Try&rev=3b9e02950972
I took the patches from this bug, bug 836951, bug 840388, and bug 841850 except for
the patch that actually switches the security.mixed_content.block_active_content to true and I pushed them to try.  This try looks good: https://tbpl.mozilla.org/?tree=Try&rev=27e901d5fbe9


I also pushed to try with all of these patches + the pref that switches block_active_content to true.  In this result, there are password manager tests that are failing - https://tbpl.mozilla.org/?tree=Try&rev=d078bae998d3.
/tests/toolkit/components/passwordmgr/test/test_prompt.html 
/tests/toolkit/components/passwordmgr/test/test_notifications_popup.html 

Turning the pref on didn't make these fail before.  But both tests have been recently changed.  I need to dig into what is causing the failures.  Locally,
(In reply to Tanvi Vyas [:tanvi] from comment #29)
> Turning the pref on didn't make these fail before.  But both tests have been
> recently changed.  I need to dig into what is causing the failures.  Locally,

...

Locally, test_notifications_popup.html completes without a timeout.  But for test_prompt.html, I do get the same error that try is observing:
TypeError: notification is undefined at http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/notification_common.js:43
See Also: → 806737
The reason that a failure in test http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_prompt.html?force=1 did not show up before was because the test was disabled and then re-enabled as part of bug 806737.

The failure is caused by a situation where you have a form with a passwords input field and mixed content.  For example, try entering a bogus username and password here: 
https://people.mozilla.com/~tvyas/javascript_post_https.html
When you enter them, you get a doorhanger popup asking if you want Firefox to remember the password.

Now try going to https://people.mozilla.com/~tvyas/javascript_post_https_mixed.html and entering a bogus username and password.  After you hit submit, you will get two icons next to the location bar.  One from the password manager to save the password, and another from the mixed content blocker.  The password manager notification doorhanger is displayed for a split second and then disappears.  I'm not sure why yet.

Justin, Jared - do you have any ideas what is going on here and why the passwords manager doorhanger disappears? The code to show the doorhanger is here http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#828 and shows that the notification should persist for 10 seconds.

I had tested stacked notifications with the click to play notification and the mixed content doorhanger (https://people.mozilla.com/~tvyas/mixedctp.html).  But both of these are dismissed by default.  I hadn't tested it with a page where one doorhanger wants to show up automatically and another doesn't.  Or in the case where both doorhangers want to show up automatically.  What is the correct behavior in these cases?

Thanks!
Flags: needinfo?(dolske)
This may be an issue with the notification code in general.  The same issue that is occurring with the mixed content blocker notification occurs with the click to play notification.  Go to https://people.mozilla.com/~tvyas/passwordctp.html and enter a bogus username and password.  You will see the remember password doorhanger appear for a split second and then disappear.
Yes, you'll either need to fix the tests so they don't trigger the mixed-content notification (and therefore pass), or we'll need to get PopupNotifications.jsm to be smarter about how multiple notifications are handled.
Flags: needinfo?(dolske)
(In reply to Justin Dolske [:Dolske] from comment #33)
> Yes, you'll either need to fix the tests so they don't trigger the
> mixed-content notification (and therefore pass), or we'll need to get
> PopupNotifications.jsm to be smarter about how multiple notifications are
> handled.

We probably need to do both.  I will file a bug to make PopupNotifications.jsm smarter.  I think the bug is somewhere in the code here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/PopupNotifications.jsm#554.
Filed bug 854740 to make PopupNotifications.jsm smarter.  Now I will work on updating the password manager tests so that they don't get multiple notifications.
The try logs show:
1:20     INFO -  166403 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | handleLoad running for test 1003
16:51:20     INFO -  166404 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | Time is Sat, 23 Mar 2013 23:51:20 GMT
16:51:20     INFO -  166405 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | is popup panel open? false
16:51:20     INFO -  166406 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | Found 2 popup notifications.
16:51:20     INFO -  166407 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | #0: mixed-content-blocked
16:51:20     INFO -  166408 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | #1: password-change
16:51:20     INFO -  166409 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | Found 0 notification bars.
16:51:20     INFO -  166410 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | handleDialog was invoked
16:51:20     INFO -  166411 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | Checking for successful authentication
16:51:20     INFO -  166412 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | Checking for echoed username
16:51:20     INFO -  166413 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | Checking for echoed password
16:51:20     INFO -  166414 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | Looking for password-change popup notification
16:51:20     INFO -  166415 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | got popup notification
16:51:20     INFO -  166416 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_prompt.html | Looking for action at index 0
16:51:20     INFO -  166417 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_prompt.html | at least one notification displayed


https://tbpl.mozilla.org/php/getParsedLog.php?id=21023785&tree=Try

Note that is says Found 2 popup notifications.
 #0: mixed-content-blocked
 #1: password-change

This test is an http test and all its subframes are also http, so there is no reason for the mixed-content-blocked popupnotification tos how up.  Running the test locally passes and I don't see any mixed content notifications or doorhangers.
http://hg.mozilla.org/integration/mozilla-inbound/rev/9b46d7f33f26
http://hg.mozilla.org/integration/mozilla-inbound/rev/d2eefc0da377
http://hg.mozilla.org/integration/mozilla-inbound/rev/ae4bb9b986cf

Pushed the mochitest updates to inbound.  Have NOT pushed the patch that enables mixed active content blocking.  We can't do that yet, because it causes the following mochitests to fail:
/tests/toolkit/components/passwordmgr/test/test_prompt.html 
/tests/toolkit/components/passwordmgr/test/test_notifications_popup.html 

Updating the dependency list.  The only blocker remaining to enable the pref is the above password manager tests OR Bug 854740 (not sure yet which of the two are the root of the problem).
No longer depends on: 781018, 837351
Whiteboard: [leave open]
I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22

Also, please update relevant docs if any.
Keywords: dev-doc-needed
(In reply to Kohei Yoshino from comment #39)
> I've added this bug to the compatibility doc. Please correct the info if
> wrong.
> https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22
> 
> Also, please update relevant docs if any.

We don't know yet if the preference to block mixed active content will be on by default in Firefox 22.  That part of this bug has not landed because of 2 password manager mochitests that are failing.
We're in the last few days of the 22 cycle -- I'd have the same concerns about landing at the end of the cycle as I did 6 weeks ago in comment 26.
Commented out this change from the compat doc.
In bug 855275, Gavin is working on the PopupNotifications bugs that cause the two password manager tests to fail.  Once that bug is complete, we should be able to turn the pref on.

Unless of course more mochitests are created/enabled in the meantime and they happen to fail with mixed content blocking turned on.  Let's hope we can finish this before that happens again ;)
Depends on: 855275
Found another test that breaks when mixed content blocking is turned on: 
https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/test_browserElement_inproc_SecurityChange.html?force=1

It's looking for a broken security state but blocking mixed content prevents that.  Need to update the test.
(In reply to Tanvi Vyas [:tanvi] from comment #45)
> Found another test that breaks when mixed content blocking is turned on: 
> https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/
> test_browserElement_inproc_SecurityChange.html?force=1
> 
> It's looking for a broken security state but blocking mixed content prevents
> that.  Need to update the test.

Test updated.  r? to smaug (who reviewed the original mochitests in bug 763694.
Attachment #733033 - Flags: review?(bugs)
Attachment #733033 - Flags: review?(bugs) → review+
Pushed to inbound!!!
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f1779767e3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/60b08f643863

With this push, Mixed Active Content will be blocked by default, with an option to un-block through a doorhanger.
Landed in FF 23.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: Firefox 22 → Firefox 23
I've added this bug to the site compat doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23

Also, please update relevant docs if any.
Keywords: site-compat
(In reply to Kohei Yoshino from comment #50)
> I've added this bug to the site compat doc. Please correct the info if wrong.
> https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23
> 
> Also, please update relevant docs if any.

There are a couple of corrections, but I don't seem to be able to correct the page.  I keep getting permission denied after logging in, editing, and trying to save.

The title should be:
Non-SSL active content on SSL pages is blocked by default

And the first sentence should read:
Firefox 18 introduced preferences to block loading content from non-SSL (http) sites on SSL (https) pages.
I made the changes you suggested in comment 51.

Can you file a bug about the devmo login/permission problem you're experiencing?
Thanks for your correcting. Tanvi: you have to enable HTTP referer on MDN to save your changes. I have an add-on installed to send referer only when I save. MDN issues a Cookie called "csrftoken" but it also refers HTTP referer to avoid CSRF attacks. Very annoying security practice, IMO.
(In reply to Kohei Yoshino from comment #53)
> Thanks for your correcting. Tanvi: you have to enable HTTP referer on MDN to
> save your changes. I have an add-on installed to send referer only when I
> save. MDN issues a Cookie called "csrftoken" but it also refers HTTP referer
> to avoid CSRF attacks. Very annoying security practice, IMO.

Thanks for the info. I filed bug 861237.
Depends on: 864787
Keywords: sec-want
Depends on: 902350
I confirm the fix is verified on Windows 7 x64 and Mac OS 10.7.5
Status: RESOLVED → VERIFIED
Depends on: 933085
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: