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

RESOLVED FIXED in Firefox 59

Status

()

P2
normal
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: tanvi, Assigned: jkt)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-needed})

unspecified
mozilla59
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [domsecurity-backlog])

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
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.

Comment 1

3 years ago
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.
(Reporter)

Updated

3 years ago
Depends on: 1244116
Whiteboard: [domsecurity-backlog]
Summary: Determine if we can make OBJECT_SUBREQUESTS blockable mixed content → Add a pref to block OBJECT_SUBREQUESTS
(Reporter)

Updated

2 years ago
Summary: Add a pref to block OBJECT_SUBREQUESTS → Add a pref to treat mixed OBJECT_SUBREQUESTS as active and block them.
(Reporter)

Comment 3

2 years ago
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)
(Reporter)

Comment 4

a year ago
I think we should do this sooner than later, as part of the HTTP Bad / HTTPS Everything projects.
Blocks: 1335586
Flags: needinfo?(tanvi)
Priority: -- → P2
Comment hidden (mozreview-request)
(Reporter)

Comment 6

10 months ago
(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!
(Assignee)

Comment 7

10 months ago
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
Comment hidden (mozreview-request)
(Assignee)

Comment 9

10 months ago
Created attachment 8929485 [details]
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.
(Assignee)

Updated

10 months ago
Attachment #8927600 - Flags: review?(tanvi)
Attachment #8927600 - Flags: review?(ckerschb)

Comment 10

10 months ago
mozreview-review
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 hidden (mozreview-request)

Comment 12

10 months ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 14

10 months ago
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+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(jkt)
(Assignee)

Comment 16

10 months ago
Fixed sorry.
Flags: needinfo?(jkt)

Comment 17

10 months ago
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

Comment 18

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ba25bdb8755
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Updated

9 months ago
Keywords: dev-doc-needed

Updated

9 months ago
Depends on: 1425828

Comment 19

9 months ago
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.
(Assignee)

Comment 20

9 months ago
: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)

Comment 21

9 months ago
Created attachment 8937932 [details]
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)
(Assignee)

Comment 22

9 months ago
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)

Comment 23

9 months ago
(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)
(Assignee)

Comment 25

8 months ago
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!

Comment 27

8 months ago
There is a live test available now:

https://mixed-object-loader.badsite.io/  (which attempts to load from http://http.badsite.io/)
(Assignee)

Updated

7 months ago
Depends on: 1438448
(Assignee)

Updated

7 months ago
Depends on: 1441794
(Assignee)

Updated

6 months ago
Depends on: 1441084
(Assignee)

Updated

6 months ago
Depends on: 1445951
See Also: → bug 1473808
You need to log in before you can comment on or make changes to this bug.