Closed Bug 1335475 Opened 7 years ago Closed 7 years ago

Deny plugins from non-HTTP/HTTPS protocols

Categories

(Core Graveyard :: Plug-ins, defect, P3)

defect

Tracking

(firefox55 fixed)

VERIFIED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bytesized, Assigned: benjamin)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files, 4 obsolete files)

As referenced here [1], plugins should be denied from non-HTTP/HTTPS protocols.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1307604#c61
Should this be part of flash blocking (pref-ed off behind |plugins.flashBlock.enabled|) or not?
Flags: needinfo?(benjamin)
This bug is not about restricting Flash to HTTPS. It's about banning non-HTTP non-HTTPS (e.g. file:) protocols.
No longer blocks: https-everything
Summary: Deny plugins from non-HTTP(S) protocols → Deny plugins from non-HTTP/HTTPS protocols
No, this should be entirely independent. It's something that we should ship and release-note separately.
Flags: needinfo?(benjamin)
Priority: P2 → P3
Stefan, heads-up about this change. I'm not going to land this until 55 so this has some time to bake. This *will* break some Flash authoring tools which do Flash debugging against file: URIs, and that is an unfortunate but intended change. I don't think we need extensive testing of this; I've tested this manually on ftp and the automated tests here cover the following cases:

* plugins still work from HTTP
* plugins aren't available from file:
* plugins aren't available from chrome:
* plugins aren't available if a user loads about:blank directly in the browser
* plugins are available if a HTTP site loads about:blank in a subframe
* plugins are available if a HTTP site loads about:blank in a window
Flags: needinfo?(stefan.georgiev)
Comment on attachment 8843455 [details]
Bug 1335475 - Deny plugins from non-HTTP/HTTPS origins.

https://reviewboard.mozilla.org/r/117202/#review118860

::: commit-message-a8f45:1
(Diff revision 1)
> +Bug 1335475 - Deny plugins from non-HTTP/HTTPS origins. r?qdot r?ksteuber

Did you mean to commit a file with the commit message?
Attachment #8843455 - Flags: review?(kyle) → review+
Comment on attachment 8843455 [details]
Bug 1335475 - Deny plugins from non-HTTP/HTTPS origins.

https://reviewboard.mozilla.org/r/117202/#review118864

This seems fine to me, though I have suggested a few small changes.

::: dom/base/nsDocument.cpp:13050
(Diff revision 1)
>    rv = principal->GetURI(getter_AddRefs(classificationURI));
>    if (NS_FAILED(rv) || !classificationURI) {
>      return FlashClassification::Denied;
>    }
>  
> +  if (Preferences::GetBool("plugins.http_https_only", true)) {

Two things about this pref.

1) It does not seem to exist in modules/libpref/init/all.js which it seems like it should be.
2) The name of the pref seems to imply that it applies to all plugins when it only applies to Flash. On the other hand, though, Flash is now the only plugin left, so perhaps it is ok to use these terms interchangebly?

::: dom/plugins/test/mochitest/browser.ini:16
(Diff revision 1)
>  skip-if = (!e10s || os != "win")
>  [browser_tabswitchbetweenplugins.js]
>  skip-if = (!e10s || os != "win")
>  [browser_pluginscroll.js]
>  skip-if = (true || !e10s || os != "win") # Bug 1213631
> +[browser_bug1335475.js]

Could this test have a bit more descriptive of a name than bug1335475?
Attachment #8843455 - Flags: review?(ksteuber) → review+
Flags: needinfo?(stefan.georgiev)
Flags: needinfo?(stefan.georgiev)
> 1) It does not seem to exist in modules/libpref/init/all.js which it seems
> like it should be.

will fix.

> 2) The name of the pref seems to imply that it applies to all plugins when
> it only applies to Flash. On the other hand, though, Flash is now the only
> plugin left, so perhaps it is ok to use these terms interchangebly?

It probably is, but I'm also not sure this is correct. nsDocument::GetAllowPlugins calls ::DocumentFlashClassification without regard to whether this is the Flash plugin or any other plugin. So I think that actually the name "DocumentFlashClassification" is actually in practice "DocumentPluginClassification".

Did I get some flash check wrong?

> > +[browser_bug1335475.js]
> 
> Could this test have a bit more descriptive of a name than bug1335475?

I've seen people do both and not clear guidance on what to prefer. Is there a particular reason to prefer one or the other?
Flags: needinfo?(ksteuber)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #9)
> > 2) The name of the pref seems to imply that it applies to all plugins when
> > it only applies to Flash. On the other hand, though, Flash is now the only
> > plugin left, so perhaps it is ok to use these terms interchangebly?
> 
> It probably is, but I'm also not sure this is correct.
> nsDocument::GetAllowPlugins calls ::DocumentFlashClassification without
> regard to whether this is the Flash plugin or any other plugin. So I think
> that actually the name "DocumentFlashClassification" is actually in practice
> "DocumentPluginClassification".
> 
> Did I get some flash check wrong?

I originally meant for |nsIDocument::DocumentFlashClassification| to be used for Flash only. The reason that it is used in a more generic way in |nsDocument::GetAllowPlugins| is because you asked for it to be used that way [1].

Like I said, perhaps it is now ok to use "Flash" and "Plugins" interchangeably. I think it is a little confusing, but I will leave it up to you to decide.

> 
> > > +[browser_bug1335475.js]
> > 
> > Could this test have a bit more descriptive of a name than bug1335475?
> 
> I've seen people do both and not clear guidance on what to prefer. Is there
> a particular reason to prefer one or the other?

I'm afraid the only evidence I can offer for my opinion on test naming is that :Mossop once told me that descriptive test names are preferred over test names that are bug numbers. If you think it is clear enough this way though, feel free to leave it as it is.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1323064#c4
Flags: needinfo?(ksteuber)
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Comment on attachment 8844509 [details]
Bug 1335475 - Switch browser_tab_dragdrop.js to load a plugin from HTTP instead of a no-privilege data: URI which doesn't work any more. Also re-enable this test on Linux now that we don't have windowed-mode plugins any more, so this won't assert (undo bu

https://reviewboard.mozilla.org/r/117916/#review119642

::: browser/base/content/test/general/browser.ini:18
(Diff revision 1)
>    browser_bug970746.xhtml
>    browser_fxa_web_channel.html
>    browser_registerProtocolHandler_notification.html
>    browser_star_hsts.sjs
>    browser_tab_dragdrop2_frame1.xul
> +  browser_tab_dragdrop_embed.html

I don't see this file in the patch nor in mozilla-central.
Attachment #8844509 - Flags: review?(jaws) → review-
Comment on attachment 8844509 [details]
Bug 1335475 - Switch browser_tab_dragdrop.js to load a plugin from HTTP instead of a no-privilege data: URI which doesn't work any more. Also re-enable this test on Linux now that we don't have windowed-mode plugins any more, so this won't assert (undo bu

https://reviewboard.mozilla.org/r/117916/#review119676
Attachment #8844509 - Flags: review?(jaws) → review+
Comment on attachment 8845413 [details]
Bug 1335475 - Move test_refresh_navigator_plugins to a plain mochitest.

https://reviewboard.mozilla.org/r/118604/#review120578

Solid patch, and the test is more readible too to boot. Thanks bsmedberg!

::: commit-message-8b20e:1
(Diff revision 1)
> +Bug 1335475 - Move test_refresh_navigator_plugins to a plain mochitest. Note that it currently is broken/disabled in e10s mode. That's covered by bug 1090576, but this patch doesn't make it worse because chrome mochitests run in local mode always anyway. r?mconley

Everything after the first "mochitest.", minus the review flag, can probably go onto the next line(s).
Attachment #8845413 - Flags: review?(mconley) → review+
Comment on attachment 8845412 [details]
Bug 1335475 - Fix test_chrome_over_plugin_window to not load plugins from a data: URI,

https://reviewboard.mozilla.org/r/118602/#review120636
Attachment #8845412 - Flags: review?(dvander) → review+
Comment on attachment 8845414 [details]
Bug 1335475 - Remove test about loading Java from file: origins because it's no longer relevant several times over.

https://reviewboard.mozilla.org/r/118606/#review120662

::: commit-message-e6715:1
(Diff revision 1)
> +Bug 1335475 - Remove test about loading Java from file: origins because it's no longer relevant several times over. r?qdot

Looks like you commited a commit-message file again? Or is this something I don't know about mozreview?
Attachment #8845414 - Flags: review?(kyle) → review+
Comment on attachment 8845414 [details]
Bug 1335475 - Remove test about loading Java from file: origins because it's no longer relevant several times over.

https://reviewboard.mozilla.org/r/118606/#review120662

> Looks like you commited a commit-message file again? Or is this something I don't know about mozreview?

Ok, just saw the dev-platform email about this. Nevermind.
Comment on attachment 8845415 [details]
Bug 1335475 - Load reftests that use plugins from HTTP since file: doesn't allow plugins any more,

https://reviewboard.mozilla.org/r/118608/#review120674

How did you verify that this was the complete set of tests that needed to be switched to HTTP?

r=dbaron assuming that the way you did that was appropriately thorough and covered all platforms... though it's info that would be useful to have in the commit message for the reviewer (and potentially others looking at it later)
Attachment #8845415 - Flags: review?(dbaron) → review+
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0838d45e9b6
Deny plugins from non-HTTP/HTTPS origins. r=bytesized,qdot
https://hg.mozilla.org/integration/autoland/rev/19bf052b3949
Switch browser_tab_dragdrop.js to load a plugin from HTTP instead of a no-privilege data: URI which doesn't work any more. Also re-enable this test on Linux now that we don't have windowed-mode plugins any more, so this won't assert (undo bug 1331320). r=jaws
https://hg.mozilla.org/integration/autoland/rev/732eb7783912
Fix test_chrome_over_plugin_window to not load plugins from a data: URI, r=dvander
https://hg.mozilla.org/integration/autoland/rev/19c91edc6549
Move test_refresh_navigator_plugins to a plain mochitest.r=mconley
https://hg.mozilla.org/integration/autoland/rev/9900d421e24e
Remove test about loading Java from file: origins because it's no longer relevant several times over. r=qdot
https://hg.mozilla.org/integration/autoland/rev/ab966fb76f78
Reftest harness needs to check for the test plugin without using navigator.plugins. Load reftests that use plugins from HTTP since file: doesn't allow plugins any more, r=dbaron
Backed out for failing e.g. dom/plugins/test/crashtests/539897-1.html:

https://hg.mozilla.org/integration/autoland/rev/5269e230d77e73ce9fea400c446742d7c88680ad
https://hg.mozilla.org/integration/autoland/rev/5be714033fbd7b76ec3018cd125309f7de799b32
https://hg.mozilla.org/integration/autoland/rev/e162d597d4d361c90d1fed0facf59b6079c9ec8e
https://hg.mozilla.org/integration/autoland/rev/577c3261adf8ed01e462b1e2c7f2e54277e42ab4
https://hg.mozilla.org/integration/autoland/rev/ed96183557d896c6b0ffdf0d0d74c2068dd53ca9
https://hg.mozilla.org/integration/autoland/rev/337a2eb6aa5c80defedd32cd3e63c3258f492e23

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ab966fb76f781459def4babad8c41651820936b9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=83124386&repo=autoland

[task 2017-03-10T21:53:05.801019Z] 21:53:05     INFO - REFTEST TEST-START | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/539897-1.html
[task 2017-03-10T21:53:05.802370Z] 21:53:05     INFO - REFTEST TEST-LOAD | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/539897-1.html | 555 / 3171 (17%)
[task 2017-03-10T21:53:05.809867Z] 21:53:05     INFO - ++DOMWINDOW == 49 (0x7f7be8152c00) [pid = 1100] [serial = 1414] [outer = 0x7f7beca07400]
[task 2017-03-10T21:53:05.834042Z] 21:53:05     INFO - JavaScript error: file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/539897-1.html, line 7: TypeError: plugin.reinitWidget is not a function
[task 2017-03-10T21:53:05.861509Z] 21:53:05     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/539897-1.html | plugin should not crash item 1

There is more:
REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/540114-1.html | plugin should not crash item 1
REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/598862.html | load failed: timed out waiting for reftest-wait to be removed

This also makes some servo reftests fail, please check the push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ab966fb76f781459def4babad8c41651820936b9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Flags: needinfo?(benjamin)
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/897169cb6bec
Switch browser_tab_dragdrop.js to load a plugin from HTTP instead of a no-privilege data: URI which doesn't work any more. Also re-enable this test on Linux now that we don't have windowed-mode plugins any more, so this won't assert (undo bug 1331320). r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/336d4724cc59
Fix test_chrome_over_plugin_window to not load plugins from a data: URI, r=dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f2b99c9577
Move test_refresh_navigator_plugins to a plain mochitest.r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/08748e75a1ff
Reftest harness needs to check for the test plugin without using navigator.plugins. r=dbaron
Attachment #8844509 - Attachment is obsolete: true
Attachment #8845412 - Attachment is obsolete: true
Attachment #8845413 - Attachment is obsolete: true
Attachment #8845414 - Attachment is obsolete: true
Try build, everything looks green, planning on autolanding this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e3c5e2f37679e15ad3ec049b8313388887eaa24&selectedJob=99203846
Flags: needinfo?(benjamin)
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17a2effb01e2
Load reftests that use plugins from HTTP since file: doesn't allow plugins any more, r=dbaron
https://hg.mozilla.org/integration/autoland/rev/8068fb1cc45e
Deny plugins from non-HTTP/HTTPS origins. r=bytesized,qdot
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/17a2effb01e2
https://hg.mozilla.org/mozilla-central/rev/8068fb1cc45e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
QA has verified this bug.

This was tested on: Windows 10 x64, Windows 7 x86, Ubuntu 16.04 x64, OS X 10.12 (Sierra) with 32 and 64 bits Nightly builds. 
The test covered the following scenarios:
Flash does not load from: URI, PDF, FTP, FTPS, file:// 
Flash does load from: HTTP and HTTPS

If you have any questions regarding the testing, please let me know.
Status: RESOLVED → VERIFIED
Flags: needinfo?(stefan.georgiev)
See Also: → 1376341
We had a functionality where a local file had Flash content embedded within. After upgrading to FF55, this functionality is broken. I no long see flash content data/charts after upgrade. Also, I noticed that the api navigator.plugins do not return 'Shockwave Flash' any more. Could you please confirm whether these are expected behavior due to this bug fix.

Thanks,
Subba
Flags: needinfo?(jmathies)
Yes, that is expected behavior as a result of the changes made here.
Flags: needinfo?(jmathies)
Depends on: 1412118
See Also: → 1550940
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: