Closed
Bug 1381755
Opened 7 years ago
Closed 7 years ago
Convert test browser_CTP_data_urls.js to comply with new data: URI inheritance model
Categories
(Core :: DOM: Security, enhancement, P2)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ckerschb
Blocks: 1337268
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Assignee | ||
Comment 1•7 years ago
|
||
Florian, we are updating the way the data: URI inheritance security model works within Firefox. By flipping the pref security.data_uri.unique_opaque_origin data: URIs become cross origin with the including context, hence the test browser_CTP_data_urls.js (which seems to originate from Bug 887773) starts failing.
It seems the test is testing that click to play from data: URIs should still be working, right? Before investing more time in trying to fix that test I wanted to make sure that this scenario actually should work once we change the data: URI inheritance model.
If yes, then I'll take a closer look on how to fix the problem. Potentially we can use SpecialPowers.wrap(...) of some sort...
Flags: needinfo?(florian)
Comment 2•7 years ago
|
||
I don't know anything about this, sorry. Forwarding the needinfo to felipe who may know (if not, you can try gfritzsche or bsmedberg).
Flags: needinfo?(florian) → needinfo?(felipc)
Comment 3•7 years ago
|
||
I'll redirect the question to Benjamin
Flags: needinfo?(felipc) → needinfo?(benjamin)
Comment 4•7 years ago
|
||
We shipped a pref that should have completely disabled plugins from data: URIs in bug 1335475, unless perhaps because it uses a principal check that didn't work?
I suspect that changing the behavior of the system to block plugins from data: URIs completely is quite reasonable, in which case this test should probably be removed but perhaps we should have a separate test to check that data: URIs can't activate plugins.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> We shipped a pref that should have completely disabled plugins from data:
> URIs in bug 1335475, unless perhaps because it uses a principal check that
> didn't work?
The pref plugins.http_https_only will only apply to data: URIs once we have flipped the pref security.data_uri.unique_opaque_origin. Currently in mozilla-central, our data: URIs still inherit the security context of the enclosing document. That means that the principal within PrincipalFlashClassification() [1] is always:
http://127.0.0.1:8888/browser/browser/base/content/test/plugins/plugin_data_url.html
Hence the test still passes currently on mozilla-central.
Now, when I flip the pref security.data_uri.unique_opaque_origin those data: URIs will load using a NullPrincipal and hence the NullPrincipal-check within PrincipalFlashClassification() [1] will return FlashClassification::Denied; causing the test to fail.
> I suspect that changing the behavior of the system to block plugins from
> data: URIs completely is quite reasonable, in which case this test should
> probably be removed but perhaps we should have a separate test to check that
> data: URIs can't activate plugins.
I think that allows us to remove that test. I am not entirely sure if I have to remove the testname also from the two *.json files. I would imagine yes, but also not entirely sure whether the testname ended up in the *.json using manual interaction or not.
[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#13257
Attachment #8887376 -
Attachment is obsolete: true
Attachment #8887634 -
Flags: review?(benjamin)
Comment 6•7 years ago
|
||
Comment on attachment 8887634 [details] [diff] [review]
bug_1381755_browser_ctp_data_url.patch
This is ok but not sufficient: please also add a test that plugin can't access Flash when you make this change.
Attachment #8887634 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 7•7 years ago
|
||
Hey Benjamin, I think this is what we want, right? This test calls PrincipalFlashClassification() three times:
* SystemPrincipal for the initial load (chrome://browser/content/browser.xul)
* codebaseprincipal for the second check (http://127.0.0.1:8888/browser/dom/plugins/test/mochitest/plugin_data_url_test.html)
* nullprincipal for the third check (data:text/html,foo)
Attachment #8888719 -
Flags: review?(benjamin)
Updated•7 years ago
|
Attachment #8888719 -
Flags: review?(benjamin) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44bd18fcb2e8
Updating the data: URI inheritance security model renders test browser_CTP_data_urls.js superfluous. r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/e86441746f45
Test that data: URIs can't activate plugins. r=bsmedberg
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44bd18fcb2e8
https://hg.mozilla.org/mozilla-central/rev/e86441746f45
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•