Closed Bug 1324406 Opened 3 years ago Closed 2 years ago

Treat 'data:' documents as unique, opaque origins

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 4 open bugs)

Details

(Keywords: addon-compat, dev-doc-complete, Whiteboard: [domsecurity-active])

Attachments

(1 file)

Change the handling of 'data:' URLs to which user agents
navigate. Rather than inheriting the origin of the settings object
responsible for the navigation, they will be treated as unique,
opaque origins.

This aligns the spec with the behavior found in Chrome, Safari,
Opera, and Edge.

See also:
https://github.com/whatwg/html/commit/00769464e80149368672b894b50881134da4602f
https://github.com/whatwg/html/issues/1753
Depends on: 1302399
Olli did a test run in November to see how much tests fail once you do the change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28735d0f2e5516c5a6d1f7805a065a6edbd8f28b
Duplicate of bug 1018872?
(In reply to Anne (:annevk) from comment #2)
> Duplicate of bug 1018872?

Indeed, I would like to keep this new bug anyway since the spec has changed and hence we could have this bug to track implementation effort.
See Also: → 1018872
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
We are going to track work also within this etherpad:
  https://public.etherpad-mozilla.org/p/datauri
Depends on: 1328860
Depends on: 1337186
Depends on: 1337268
Depends on: 1337269
Depends on: 1337270
Depends on: 1337271
Depends on: 1337272
Just so we're clear on the test bugs: the problem is not just tests that _fail_ with the pref flipped.  It's also tests that stop testing what they should be testing completely and always pass instead.
Some tests can be repaired by replacing data with actual resources, as they rely on the resource being in the same origin.
Other tests use data URIs for images or scripts, in which case the functionality shouldn't be hampered through the change.

What scenario do you have in mind, exactly?
Flags: needinfo?(bzbarsky)
Well, say we had a test that did:

  <iframe sandbox src="data:whatever">

and was then checking that the "sandbox" attribute actually made the load different-origin.  Then we make data: always different-origin.  Then we break the behavior of the "sandbox" attribute so it doesn't work anymore and don't notice.

Yes, I know we probably have tests for @sandbox that don't use data:, so this _specific_ scenario is ok.

What we should probably do if we want to do this right is MOZ_CRASH if data: is being loaded in a docshell, and then go through all the test failures resulting from _that_.
Flags: needinfo?(bzbarsky)
Note that to provide motivation for sites to switch to HTTPS there's been a push to have all new features require a secure context, whether the features are in any way security related or not. If we prevent data: URI documents inheriting the security context'edness of the opener we will find data: URI pages increasingly hobbled in ways we may not foresee yet. Then again, once we've killed off plain-HTTP I guess the concept of Secure Contexts can die and we can unhobble them. ;)
No longer depends on: 1337186
Depends on: 1345593
Depends on: 1357386
Depends on: 1366973
This probably needs a mention somewhere on MDN.
Keywords: dev-doc-needed
No longer depends on: 1357386
Duplicate of this bug: 1370792
Should we treat data:image as cross origin? as I am working in one of the tests, I found that Chrome treats data:image/ as the same origin
But if we turn on the pref in Firefox, it will throw Security Error.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1368895#c2
> Should we treat data:image as cross origin?

No.  Nor should you block data: fonts.  And data: stylesheets should probably be considered same-origin too.  Not sure what the state is for data: workers.  Some of these things already use one of our *_DATA_INHERITS security flags, but I expect the current implementation of the pref completely breaks those across the board, which is wrong.

Note that the specs may or may not properly spec all of this, depending.  File spec bugs as needed.
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #12)
> > Should we treat data:image as cross origin?
> 
> No.  Nor should you block data: fonts.  And data: stylesheets should
> probably be considered same-origin too.  Not sure what the state is for
> data: workers.  
> 
Thanks, I'll file bugs.

One question, what if we do
<iframe src="data:image/png..."></iframe>
What should this be?

Per spec,
"If the Document was generated from a data: URL

    A unique opaque origin assigned when the Document is created.
"

it should be cross origin, right?
The document should be cross-origin, yes, with the pref flipped.  The image in that document should be same-origin with that document.
That is, the handling of data: basically depends on the loading _context_, not the MIME type of the response.  If you do:

  <iframe src="data:text/css,stuff"></iframe>

That should likewise not be same-origin with the parent, with the pref flipped.
> Not sure what the state is for data: workers.

Its treated as an opaque null principal origin.
Right, but it's still allowed to load, yes?
Depends on: 1377244
Depends on: 1377523
Depends on: 1377748
Depends on: 1377861
Duplicate of this bug: 1378875
Depends on: 1380249
Depends on: 1380631
Depends on: 1380641
Depends on: 1380717
Depends on: 1380752
Depends on: 1380765
Depends on: 1381761
Depends on: 1382815
Depends on: 1383649
Depends on: 1383672
Depends on: 1383732
Depends on: 1384048
Depends on: 1384427
Depends on: 1384500
Depends on: 1385223
Depends on: 1385240
Depends on: 1385805
Depends on: 1386183
Depends on: 1387997
Depends on: 1388003
Depends on: 1388027
Depends on: 1388028
Depends on: 1388140
Depends on: 1388141
Depends on: 1388142
Just got an idea and not sure if has been discussed:

For those fixes which requires replacing data:uri requests, can we just
have a "echo server" which just parse/echo what's carried in the data:uri?

For example, for
 
<iframe src="data:text/html,<iframe></iframe>">

instead of creating a new file and replacing with that file,

we can just change it to

<iframe src="https://mochi.test/echo/" + encode("data:text/html,<iframe></iframe>"))

and the echo sever would respond data based on what the data:uri specified.

I can imagine people who are fixing this kind of test cases have been suffering
from creating new files and modifying .ini files.
That might be okay for mochitests. I don't think it's good enough for web-platform-tests. But also, simple cases like that can be written as <iframe srcdoc="<iframe></iframe>">.
(In reply to Anne (:annevk) from comment #20)
> That might be okay for mochitests. I don't think it's good enough for
> web-platform-tests. But also, simple cases like that can be written as
> <iframe srcdoc="<iframe></iframe>">.

I am rather for rewriting those tests if needed. Also for future changes within our codebase and testing infrastructure we might run ourselves into a corner where it's hard to get out of. Adding such changes were content dynamically gets modified is hard to follow and reason about. I think it's better to keep it as simple as possible.
(In reply to Christoph Kerschbaumer [:ckerschb, back on Aug 16] from comment #21)
> (In reply to Anne (:annevk) from comment #20)
> > That might be okay for mochitests. I don't think it's good enough for
> > web-platform-tests. But also, simple cases like that can be written as
> > <iframe srcdoc="<iframe></iframe>">.
> 
> I am rather for rewriting those tests if needed. Also for future changes
> within our codebase and testing infrastructure we might run ourselves into a
> corner where it's hard to get out of. Adding such changes were content
> dynamically gets modified is hard to follow and reason about. I think it's
> better to keep it as simple as possible.

I think my comment was not completely clear. To be precise, we should add new files and rewrite one test at a time instead of dynamically changing content on a simulated server.
Blocks: 1366973
No longer depends on: 1366973
Depends on: 1387684
No longer depends on: 1388141
Blocks: dataxss
Depends on: 1392006
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7ca5a6b268ccc2559cf5bf0246965631caf05e84

Carsten, we are about to flip the pref |security.data_uri.unique_opaque_origin| and push to inbound within the next couple days and hence we also pushed to TRY a couple times over the last few days. The link within this comment (see above) contains a few errors, but we *think* all of those errors are intermittents. Any chance you could take a look at the provided link and let us know if we forgot to update a test? It seems that wpt10 on OS X Stylo opt/debug fails permanently. Do you think we can still consider that to be intermittent? Thanks.
Flags: needinfo?(cbook)
Smaug, I think we are ready to flip the pref (assuming all of the errors we encounter on TRY can be attributed to be intermittents). Do we need an additional reviewer to sign off on this or are we good? What do you think?
Attachment #8899265 - Flags: review?(bugs)
Depends on: 1392095
Priority: P3 → P1
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Intent-to-ship to dev.platform would be good.
ah, it is there.
Comment on attachment 8899265 [details] [diff] [review]
bug_1324406_treat_data_documents_as_unique_opaque_origins.patch

Are there tests to ensure we have same data: handling also in workers as other browsers?
Attachment #8899265 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #32)
> Are there tests to ensure we have same data: handling also in workers as
> other browsers?

I browsed through the workers wpt-tests; none of them are disabled (see grep for data: underneath).

@Ben, are there any other worker tests we should make sure to run correctly before we flip the pref so that data: URIs are treated cross origin with the including context?

@Anne, do you happen to know if there are any other tests we need to make sure we run correctly? Otherwise I think we are ready to go.

ckerschb@ckerschbdesktop:~/source/mc/testing/web-platform/tests/workers$ grep -r "data:" .
./data-url-shared.html:    var w = new SharedWorker(`data:${mime_type},onconnect = function(e) { port = e.ports[0]; ${worker_code}}`);
./data-url-shared.html:// 'data:' workers are cross-origin
./data-url-shared.html:// 'data:' workers have opaque origin
./constructors/Worker/same-origin.html:  var worker = new Worker('data:,postMessage(1);');
./constructors/SharedWorker/same-origin.html:  var worker = new SharedWorker('data:,onconnect = function(e) { e.ports[0].postMessage(1); }', '');
./interfaces/WorkerUtils/importScripts/006.html:  importScripts('data:text/javascript,x=1',
./interfaces/WorkerUtils/importScripts/006.html:                'data:text/javascript,throw 2',
./interfaces/WorkerUtils/importScripts/006.html:                'data:text/javascript,z=3');
./interfaces/WorkerUtils/importScripts/005.html:  importScripts('data:text/javascript,x={',
./interfaces/WorkerUtils/importScripts/005.html:                'data:text/javascript,}');
./interfaces/WorkerUtils/importScripts/009.html:importScripts('data:text/javascript,function run() { for(var i = 0; i < 1000; ++i) { if (i == 500) log(true); } return 1; }');
./interfaces/WorkerUtils/importScripts/007.html:importScripts('data:text/javascript,postMessage(1)');
./interfaces/WorkerUtils/importScripts/003.html:  importScripts('data:text/javascript,x+="b"',
./interfaces/WorkerUtils/importScripts/003.html:                'data:text/javascript,x+="c"');
./interfaces/WorkerUtils/importScripts/004.html:  importScripts('data:text/javascript,x+="first script successful. "',
./interfaces/WorkerUtils/importScripts/004.html:                'data:text/javascript,x+="FAIL (second script). "; for(;) break;', // doesn't compile
./interfaces/WorkerUtils/importScripts/004.html:                'data:text/javascript,x+="FAIL (third script)"');
./interfaces/WorkerUtils/importScripts/002.worker.js:    importScripts('data:text/javascript,ran=true','http://foo bar');
./interfaces/WorkerUtils/importScripts/008.html:importScripts('data:text/javascript,function run() { log(true) }');
./semantics/structured-clone/worker-common.js:        if (check_ImageData(input, {width:1, height:1, data:[0,0,0,0]})) {
./semantics/structured-clone/worker-common.js:        if (check_ImageData(input, {width:1, height:1, data:[100, 101, 102, 103]})) {
./semantics/structured-clone/worker-common.js:            check_ImageData(input[0], {width:1, height:1, data:[0,0,0,0]})) {
./semantics/structured-clone/worker-common.js:            check_ImageData(input[0], {width:1, height:1, data:[100, 101, 102, 103]})) {
./semantics/structured-clone/worker-common.js:              check_ImageData(input['x'], {width:1, height:1, data:[0,0,0,0]})) {
./semantics/structured-clone/worker-common.js:              check_ImageData(input['x'], {width:1, height:1, data:[100, 101, 102, 103]})) {
./semantics/structured-clone/worker-common.js:        if (check_ImageBitmap(input, {width:1, height:1, data:[0, 0, 0, 0]})) {
./semantics/structured-clone/worker-common.js:        if (check_ImageBitmap(input, {width:1, height:1, data:[100, 101, 102, 103]})) {
./semantics/structured-clone/worker-common.js:            check_ImageBitmap(input[0], {width:1, height:1, data:[0, 0, 0, 0]})) {
./semantics/structured-clone/worker-common.js:            check_ImageBitmap(input[0], {width:1, height:1, data:[100, 101, 102, 103]})) {
./semantics/structured-clone/worker-common.js:              check_ImageBitmap(input['x'], {width:1, height:1, data:[0, 0, 0, 0]})) {
./semantics/structured-clone/worker-common.js:              check_ImageBitmap(input['x'], {width:1, height:1, data:[100, 101, 102, 103]})) {
./semantics/navigation/001-1.html:<a href='data:text/html,<title>foo</title><script>onload=function(){window.parent.postMessage({title: window.document.title}, "*")}</script>'>link</a>
./data-url.html:    var w = new Worker(`data:${mime_type},${worker_code}`);
./data-url.html:    var w = new Worker(`data:${mime_type},${worker_code};postMessage("PASS")`);
./data-url.html:// 'data:' workers are cross-origin
./data-url.html:// 'data:' workers have opaque origin
Flags: needinfo?(bkelly)
Flags: needinfo?(annevk)
In web-platform-tests there's these:

  workers/data-url-shared.html
  workers/data-url.html

Those were written as part of https://github.com/whatwg/html/pull/1782 which points to various issues where we agreed on data: URL handling across the board and across the various browsers (insofar they participated). Since it was in 2016 I don't think browser bugs were necessarily filed, we only started doing that consistently recently.
Flags: needinfo?(annevk)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #26)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=7ca5a6b268ccc2559cf5bf0246965631caf05e84
> 
> Carsten, we are about to flip the pref
> |security.data_uri.unique_opaque_origin| and push to inbound within the next
> couple days and hence we also pushed to TRY a couple times over the last few
> days. The link within this comment (see above) contains a few errors, but we
> *think* all of those errors are intermittents. Any chance you could take a
> look at the provided link and let us know if we forgot to update a test? It
> seems that wpt10 on OS X Stylo opt/debug fails permanently. Do you think we
> can still consider that to be intermittent? Thanks.

Checked with Aryx on IRC, he looked at this TRY push [1] and said that |Windows 7 debug (c2)| looks suspicious. We were already investigating that failure within Bug 1390434 but concluded that even without our changes that test seems to fail, see [2].


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ca5a6b268ccc2559cf5bf0246965631caf05e84
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1390434#c10
Blocks: 1365145
No longer depends on: 1365145
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #26)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=7ca5a6b268ccc2559cf5bf0246965631caf05e84
> 
> Carsten, we are about to flip the pref
> |security.data_uri.unique_opaque_origin| and push to inbound within the next
> couple days and hence we also pushed to TRY a couple times over the last few
> days. The link within this comment (see above) contains a few errors, but we
> *think* all of those errors are intermittents. Any chance you could take a
> look at the provided link and let us know if we forgot to update a test? It
> seems that wpt10 on OS X Stylo opt/debug fails permanently. Do you think we
> can still consider that to be intermittent? Thanks.

Carsten, in the end that ni? is not needed anymore. I checked with Aryx and we have pushed to TRY several times. This is ready!
Flags: needinfo?(cbook)
We are still investigating Bug 1392006, but we think we shouldn't wait with the pref flip. We will go ahead and push that change within this bug to inbound, but keep investigating Bug 1392006 at the same time.
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d712f190ba
Treat 'data:' documents as unique, opaque origins. r=smaug
Backed out for failing talos tpaint test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/498614f0f0658046ca51dd7feb9df90a7d5d4da6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e8d712f190baf68efa3d0ad45424a3092d1531ec&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=125324164&repo=mozilla-inbound

14:50:00     INFO -  TEST-START | tpaint
14:50:00     INFO -  Initialising browser for tpaint test...
14:50:00     INFO -  Application command: /builds/slave/test/build/application/firefox/firefox  http://localhost:36765/getInfo.html -profile /tmp/tmpy0JRJg/profile
14:50:00     INFO -  TEST-INFO | started process 15394 (/builds/slave/test/build/application/firefox/firefox  http://localhost:36765/getInfo.html)
14:50:05     INFO -  TEST-INFO | 15394: exit 0
14:50:05     INFO -  Browser initialized.
14:50:05     INFO -  Running cycle 1/1 for tpaint test...
14:50:05     INFO -  TEST-INFO | started process 15570 (/builds/slave/test/build/application/firefox/firefox -profile /tmp/tmpy0JRJg/profile -tp file:/builds/slave/test/build/tests/talos/talos/tests/tpaint/tpaint.manifest.develop -tpchrome -tpmozafterpaint -tpnoisy -tpcycles 1 -tppagecycles 20)
14:50:06     INFO -  PID 15570 |
14:50:06     INFO -  PID 15570 | (/builds/slave/test/build/application/firefox/firefox:15620): Pango-WARNING **: error opening config file '/home/cltbld/.pangorc': Permission denied
14:50:06     INFO -  PID 15570 |
14:50:07     INFO -  PID 15570 |
14:50:07     INFO -  PID 15570 | (/builds/slave/test/build/application/firefox/firefox:15675): Pango-WARNING **: error opening config file '/home/cltbld/.pangorc': Permission denied
14:50:07     INFO -  PID 15570 |
14:50:07     INFO -  PID 15570 | __metrics	Screen width/height:1600/1200
14:50:07     INFO -  PID 15570 | 	colorDepth:24
14:50:07     INFO -  PID 15570 | 	Browser inner width/height: 1024/768
14:50:07     INFO -  PID 15570 | __metrics
14:50:16     INFO -  PID 15570 | RSS: Main: 177057792
14:50:16     INFO -  PID 15570 |
14:55:05     INFO -  Timeout waiting for test completion; killing browser...
14:55:05     INFO -  Terminating psutil.Process(pid=15570, name='firefox')
14:55:05     INFO -  PID 15570 | ExceptionHandler::GenerateDump cloned child 15745
14:55:05     INFO -  PID 15570 | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
14:55:05     INFO -  PID 15570 | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
14:55:05     INFO -  TEST-UNEXPECTED-ERROR | tpaint | timeout
14:55:05    ERROR -  Traceback (most recent call last):
14:55:05     INFO -    File "/builds/slave/test/build/tests/talos/talos/run_tests.py", line 279, in run_tests
14:55:05     INFO -      talos_results.add(mytest.runTest(browser_config, test))
14:55:05     INFO -    File "/builds/slave/test/build/tests/talos/talos/ttest.py", line 61, in runTest
14:55:05     INFO -      return self._runTest(browser_config, test_config, setup)
14:55:05     INFO -    File "/builds/slave/test/build/tests/talos/talos/ttest.py", line 201, in _runTest
14:55:05     INFO -      if counter_management else None),
14:55:05     INFO -    File "/builds/slave/test/build/tests/talos/talos/talos_process.py", line 127, in run_browser
14:55:05     INFO -      raise TalosError("timeout")
14:55:05     INFO -  TalosError: timeout
Flags: needinfo?(ckerschb)
Depends on: 1393350
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #41)
> Backed out for failing talos tpaint test:

Thanks - I filed Bug 1393350 to get that test updated.
Flags: needinfo?(ckerschb)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd36476dd8b3
Treat 'data:' documents as unique, opaque origins. r=smaug
https://hg.mozilla.org/mozilla-central/rev/cd36476dd8b3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Given that this behaviour matches other browsers, it shouldn't break sites IIUC. But extensions are apparently affected, as reported in Bug 1392006.
Duplicate of this bug: 1398073
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #46)
> To document this, I've added a note to the Data URLs page
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs
> and a note to the Fx57 rel notes:
> https://developer.mozilla.org/en-US/Firefox/Releases/57#Other
> Do these look OK?

Thanks for adding these.

In my opinion, this should fit in the Security section (in the release notes).
This change fixed a vulnerability in Firefox.
Before we treated data: URL scheme as a unique opaque origin, attackers might be able
to launch cross-site scripting (XSS) attacks using data: URL scheme.
Flags: needinfo?(cmills)
(In reply to Ethan Tseng [:ethan] - 57 Regression Engineering Support from comment #48)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #46)
> > To document this, I've added a note to the Data URLs page
> > https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs
> > and a note to the Fx57 rel notes:
> > https://developer.mozilla.org/en-US/Firefox/Releases/57#Other
> > Do these look OK?
> 
> Thanks for adding these.
> 
> In my opinion, this should fit in the Security section (in the release
> notes).
> This change fixed a vulnerability in Firefox.
> Before we treated data: URL scheme as a unique opaque origin, attackers
> might be able
> to launch cross-site scripting (XSS) attacks using data: URL scheme.

No worries - that makes sense. I've moved it.
Flags: needinfo?(cmills)
Duplicate of this bug: 1016491
Duplicate of this bug: 656823
Blocks: 1475831
Blocks: 1552168
You need to log in before you can comment on or make changes to this bug.