Deny plugins from non-HTTP/HTTPS protocols

VERIFIED FIXED in Firefox 55

Status

()

Core
Plug-ins
P3
normal
VERIFIED FIXED
5 months ago
a month ago

People

(Reporter: bytesized, Assigned: bsmedberg)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla55
dev-doc-complete, site-compat
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

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

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1307604#c61
(Reporter)

Comment 1

5 months ago
Should this be part of flash blocking (pref-ed off behind |plugins.flashBlock.enabled|) or not?
Flags: needinfo?(benjamin)
Blocks: 1335586

Updated

5 months ago
Keywords: dev-doc-needed, site-compat
This bug is not about restricting Flash to HTTPS. It's about banning non-HTTP non-HTTPS (e.g. file:) protocols.
No longer blocks: 1335586
Summary: Deny plugins from non-HTTP(S) protocols → Deny plugins from non-HTTP/HTTPS protocols
(Assignee)

Comment 3

5 months ago
No, this should be entirely independent. It's something that we should ship and release-note separately.
Flags: needinfo?(benjamin)
Priority: P2 → P3
(Reporter)

Updated

5 months ago
No longer blocks: 1277346, 1335232, 1323064, 1333483
Here's the preliminary site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/flash-will-be-loaded-only-from-https-sites/
Comment hidden (mozreview-request)
(Assignee)

Comment 6

4 months ago
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+
(Reporter)

Comment 8

4 months ago
mozreview-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+

Updated

4 months ago
Flags: needinfo?(stefan.georgiev)

Updated

4 months ago
Flags: needinfo?(stefan.georgiev)
(Assignee)

Comment 9

4 months ago
> 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)
(Reporter)

Comment 10

4 months ago
(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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → benjamin
Status: NEW → ASSIGNED

Comment 14

4 months ago
mozreview-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/#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 hidden (mozreview-request)

Comment 16

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

Comment 25

4 months ago
mozreview-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 26

4 months ago
mozreview-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 29

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

Comment 35

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

Updated

3 months ago
Duplicate of this bug: 504255

Comment 44

2 months ago
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
Blocks: 1361798
Keywords: leave-open

Comment 45

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/897169cb6bec
https://hg.mozilla.org/mozilla-central/rev/336d4724cc59
https://hg.mozilla.org/mozilla-central/rev/f9f2b99c9577
https://hg.mozilla.org/mozilla-central/rev/08748e75a1ff
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8844509 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8845412 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8845413 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8845414 - Attachment is obsolete: true
(Assignee)

Comment 48

a month ago
Try build, everything looks green, planning on autolanding this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e3c5e2f37679e15ad3ec049b8313388887eaa24&selectedJob=99203846
Flags: needinfo?(benjamin)

Comment 49

a month ago
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
(Assignee)

Updated

a month ago
Keywords: leave-open

Comment 50

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/17a2effb01e2
https://hg.mozilla.org/mozilla-central/rev/8068fb1cc45e
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
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)
I have documented this by adding a note here:

https://developer.mozilla.org/en-US/docs/Plugins

And to the Fx55 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/55#Security
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.