Closed Bug 1190623 Opened 9 years ago Closed 6 years ago

Add a pref to treat mixed OBJECT_SUBREQUESTS as active and block them.

Categories

(Core :: DOM: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: tanvi, Assigned: jkt)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [domsecurity-backlog])

Attachments

(3 files)

Assume you are on an https page with a plugin.  The request for the object is also https.  Once the user agent has retrieved the plugin source, the plugin takes over and makes additional subrequests for more content.  These subrequests may or may not be over https.

Assume they are over http.  The plugin subrequests may be requesting resources that fall in the blockable mixed content category (ex: javascript) or they may be requesetiing resources that fall into the optionally blockable mixed ocntent category (ex: images).  The key point here is that those requests are being made by the plugin software installed on the users computer, not by the browser itself and being proxied through the browser.  Hence, the browser doesn't know which mixed content category to put each of these subrequests in.

Thus far, Firefox and Chrome have considered these object subrequests as optionally blockable content.  That means it is actually possible for a MITM to intercept an http script request and compromise an https page that has a plugin running in it.  We have a bug on file to try and differentiate the subrequest, but there is no good technical solution - https://bugzilla.mozilla.org/show_bug.cgi?id=836352.

Chrome has recently proposed putting all of these object subrequests blockable by default:
https://codereview.chromium.org/1265953002, https://codereview.chromium.org/1260163005
From my understanding, they are going to experiment and see how it goes.

We should collect telemetry on how often we have mixed content object subrequests and see if we too can move these from optionally blockable to blockable.  This means that we will block images along with script for mixed content requests made by plugins.

This bug is just to determine if this is feasible.  We can make a followup to actually do it; which is a 2 line code change.
Is it acceptable to just have it on Nightly for a while coupled with a blog post to raise some awareness to see if anything serious breaks? Any additional sandboxing of plugins seems extremely warranted.
Depends on: 1244116
Whiteboard: [domsecurity-backlog]
Summary: Determine if we can make OBJECT_SUBREQUESTS blockable mixed content → Add a pref to block OBJECT_SUBREQUESTS
Summary: Add a pref to block OBJECT_SUBREQUESTS → Add a pref to treat mixed OBJECT_SUBREQUESTS as active and block them.
I lost track of this.  Should we bump this back up?  Adding the pref to the codebase and enabling it in nightly would take less then a few hours.
Flags: needinfo?(tanvi)
I think we should do this sooner than later, as part of the HTTP Bad / HTTPS Everything projects.
Flags: needinfo?(tanvi)
Priority: -- → P2
(In reply to Jonathan Kingston [:jkt] from comment #5)
> Created attachment 8927600 [details]
> Bug 1190623 - Adding a pref to consider object sub requests as active
> 
> Review commit: https://reviewboard.mozilla.org/r/198892/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/198892/

Looks pretty good Jonathan.  You will probably need to update tests; hopefully you won't need to create a new one, but see what is there today to start with.  If you are taking this bug, can you assign yourself?  Thanks!
This code I think works and I have a test case written in haxe which was the only open source converter for flash I can find. However there is a crash happening in Ubuntu Linux at the moment with flash loads. I will check back later in the week to see if there has been a fix.
Assignee: nobody → jkt
Attached file testflash.zip
This zip file contains everything to test the patch, our testing infra won't run flash tests reliably and the mixed content blocking is already well tested anyway.

In the README.md is instructions of how to spin up two severs "client-server" and "server" which allow you to browse 127.0.0.1:8080 and check with dev tools what is being blocked.

With the pref false you can see that the "127.0.0.1:8081/" gets loaded where as it won't when loading with it true.
Attachment #8927600 - Flags: review?(tanvi)
Attachment #8927600 - Flags: review?(ckerschb)
Comment on attachment 8927600 [details]
Bug 1190623 - Adding a pref to consider object sub requests as active

https://reviewboard.mozilla.org/r/198892/#review206882

In gernal, that all looks good to me. Just fix my nits and flag me again and we have that landed in no time.

::: browser/app/profile/firefox.js:1328
(Diff revision 2)
> +// Block sub requests that happen within an object
> +#if defined(NIGHTLY_BUILD)
> +pref("security.mixed_content.block_object_subrequest", true);
> +#else
> +pref("security.mixed_content.block_object_subrequest", false);
> +#endif

Please add the new pref here:
https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2569-2571

and I think we can for now also ship it in early beta, so please use: EARLY_BETA_OR_EARLIER

::: dom/security/nsMixedContentBlocker.cpp:600
(Diff revision 2)
> +      classification = eMixedDisplay;
> +      break;
>      case TYPE_OBJECT_SUBREQUEST:
> +      if (!sBlockMixedObjectSubrequest) {
> -      classification = eMixedDisplay;
> +        classification = eMixedDisplay;
> +      }

I don't like that implicit fall through, but I guess that's something we should fix another day.
Attachment #8927600 - Flags: review?(ckerschb)
Comment on attachment 8927600 [details]
Bug 1190623 - Adding a pref to consider object sub requests as active

https://reviewboard.mozilla.org/r/198892/#review208438

thanks. r=me

::: dom/security/nsMixedContentBlocker.cpp:605
(Diff revision 3)
>      case TYPE_OBJECT_SUBREQUEST:
> +      if (!sBlockMixedObjectSubrequest) {
> -      classification = eMixedDisplay;
> +        classification = eMixedDisplay;
> +      } else {
> +        classification = eMixedScript;
> +      }

One last nit, since you are already flipping it around:

case TYPE_OBJECT_SUBREQUEST:
  if (sBlockMixedObjectSubrequest) {
    classification = eMixedScript;
  } else {
    classification = eMixedDisplay;
  }
  
no need to check for the inverse :-)
Attachment #8927600 - Flags: review?(ckerschb) → review+
Comment on attachment 8927600 [details]
Bug 1190623 - Adding a pref to consider object sub requests as active

Looks good!  Ship it!
Attachment #8927600 - Flags: review?(tanvi) → review+
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(jkt)
Fixed sorry.
Flags: needinfo?(jkt)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1ba25bdb8755
Add a pref to consider object sub requests as active. r=tanvi, r=ckerschb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1ba25bdb8755
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1425828
This issue seems to lead to some video sites (or live video streaming site) flash video can not be played. 

For example: https://v.douyu.com/author/XrZwYelr5wbK

click to enable flash and you will see an error message from the website.

Before this bug fixed version is normal. the later version works fine if changed security.mixed_content.block_object_subrequest in about:config

Whether this change will be applied in the 59 as default? Chrome works normal. If this is because of the site's problem, I think I can help to contact the site to modify.
:yxu Our current timeline for enabling this by default is Firefox 60, however I don't even get a blocked mixed active icon with this site (https://v.douyu.com/author/XrZwYelr5wbK) which is odd. I do however get an intermittent crash on refresh of the page. Can you confirm you get the same?
Flags: needinfo?(yxu)
Attached image Playing error
My screenshot shows that. First of all, without changing the prefs and click the flash element to play, after a while the video will display a playing error message. Then modify the prefs, refresh the page, the video can play normally.
Flags: needinfo?(yxu)
Thanks for the debugging. I don't see this working even with Firefox stable however this might be a regional or Linux issue (I see the second screenshot in Firefox release).

I was able to get a full tab crash however this was what I was replicating on Linux before I did the work so that isn't a concern with this patch. This likely is a sandbox issue perhaps.

I think as mentioned with the comcast Bug 1425828 we should take an advocacy stance first before deciding what to do next.
Flags: needinfo?(yxu)
(In reply to Jonathan Kingston [:jkt] from comment #22)
> Thanks for the debugging. I don't see this working even with Firefox stable
> however this might be a regional or Linux issue (I see the second screenshot
> in Firefox release).
> 
> I was able to get a full tab crash however this was what I was replicating
> on Linux before I did the work so that isn't a concern with this patch. This
> likely is a sandbox issue perhaps.
> 
> I think as mentioned with the comcast Bug 1425828 we should take an advocacy
> stance first before deciding what to do next.

The release version will not play failure. I mean, this change has already been pushed to nightly, so the latest nightly version will show the playing error.

I also support promoting this change, but because of chrome normal performance, the users maybe consider this is a browser problem. We'd better confirm that whether under the same standard, firefox and chrome has inconsistent performance.
Flags: needinfo?(yxu)
I have documented this by adding a note to our experimental features pages for the moment:

https://developer.mozilla.org/en-US/Firefox/Experimental_features#Security

I will add this to the Fx release notes/Plugin information when it gets enabled in release.

In the meantime, let me know if that note reads OK. Thanks!
Flags: needinfo?(jkt)
It might be simpler to state:

*Block plain text requests from Flash plugins on encrypted pages*
A pref has been added to treat mixed OBJECT_SUBREQUESTS as active content to mitigate MitM attacks due to plugins loading "risky" content. See bug 1190623 for more details.
Flags: needinfo?(jkt)
(In reply to Jonathan Kingston [:jkt] from comment #25)
> It might be simpler to state:
> 
> *Block plain text requests from Flash plugins on encrypted pages*
> A pref has been added to treat mixed OBJECT_SUBREQUESTS as active content to
> mitigate MitM attacks due to plugins loading "risky" content. See bug
> 1190623 for more details.

Much better - updated, thanks!
There is a live test available now:

https://mixed-object-loader.badsite.io/  (which attempts to load from http://http.badsite.io/)
Depends on: 1438448
Depends on: 1441794
Depends on: 1441084
Depends on: 1445951
See Also: → 1473808
See Also: 1473808
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: