If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

data:image, data:css/, and fonts from data: URI should be same origin

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM: Security
P2
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: allstars, Assigned: allstars)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 attachments)

(Assignee)

Description

3 months ago
As mentioned by bz in https://bugzilla.mozilla.org/show_bug.cgi?id=1324406#c12
data:image and data:css should be considered as the same origin.

From https://fetch.spec.whatwg.org/#concept-main-fetch
4.1.12, 
request’s current url’s scheme is "data"
        Set request’s response tainting to "basic".

https://html.spec.whatwg.org/multipage/urls-and-fetching.html#terminology-3
2.5.1
A response whose type is "basic", "cors", or "default" is CORS-same-origin. [FETCH]
(Assignee)

Updated

3 months ago
Blocks: 1373515
(Assignee)

Updated

3 months ago
Summary: data:image and data:css/ should be same origin → data:image, data:css/, and fonts from data: URI should be same origin
Whiteboard: [domsecurity-active]
(Assignee)

Updated

3 months ago
Status: NEW → ASSIGNED
Priority: -- → P2
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1374252
(Assignee)

Comment 2

3 months ago
Created attachment 8879870 [details] [diff] [review]
Part 1: data:image, data:css, and data:fonts should be same origin
(Assignee)

Comment 3

3 months ago
Created attachment 8879871 [details] [diff] [review]
Part 2: Revert Bug 1364367
(Assignee)

Comment 4

3 months ago
Created attachment 8879874 [details] [diff] [review]
Part 3: Revert Bug 1363634
(Assignee)

Updated

3 months ago
Attachment #8879870 - Flags: review?(bugs)
(Assignee)

Updated

3 months ago
Attachment #8879871 - Flags: review?(ckerschb)
(Assignee)

Updated

3 months ago
Attachment #8879874 - Flags: review?(ckerschb)

Comment 5

3 months ago
Could you explain this a bit. Why only document navigations?

And 
     inherit = nsContentUtils::ChannelShouldInheritPrincipal(
       aPrincipalToInherit,
       aURI,
       true, // aInheritForAboutBlank
-      isSrcdoc);
+      isSrcdoc) && !isURIUniqueOrigin ;

Wouldn't it be faster to have
!isURIUniqueOrigin && nsContentUtils::ChannelShouldInheritPrincipal(...

Comment 6

3 months ago
Fetch spec is so super hard to follow that could you perhaps tell where it says the limitation is navigation only.

Comment 7

3 months ago
Comment on attachment 8879870 [details] [diff] [review]
Part 1: data:image, data:css, and data:fonts should be same origin

This a bit unusual r-, but could you explain why navigations, and only them need special casing?
(and tweak inherit assignment per my earlier comment.)
Attachment #8879870 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #7)
> This a bit unusual r-, but could you explain why navigations, and only them
> need special casing?
> (and tweak inherit assignment per my earlier comment.)

Olli, comment [1] and the following comments explain why.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1324406#c12
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> (In reply to Olli Pettay [:smaug] from comment #7)
> > This a bit unusual r-, but could you explain why navigations, and only them
> > need special casing?
> > (and tweak inherit assignment per my earlier comment.)
> 
> Olli, comment [1] and the following comments explain why.
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1324406#c12

E.g. if you have a toplevel page A, that loads a data: iframe and that data: iframe loads a data:image, then the iframe should be cross origin with the toplevel page, but the iframe should be same origin with the image.
Flags: needinfo?(bugs)
(Assignee)

Comment 10

3 months ago
(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8879870 [details] [diff] [review]
> Part 1: data:image, data:css, and data:fonts should be same origin
> 
> This a bit unusual r-, but could you explain why navigations, and only them
> need special casing?
> (and tweak inherit assignment per my earlier comment.)

Sorry, as bz suggested we should decice to inherit by the _load_context_, 
so first I reverted what :ckerschb did in nsDataHandler::GetProtocolFlags from bug 1328860. 
otherwise I have to modify all the code that calling ChannelShouldInheritPrincipal

and for subresource, they use the flag  nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS or nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS 
 
image: http://searchfox.org/mozilla-central/source/image/imgLoader.cpp#747
css: http://searchfox.org/mozilla-central/source/layout/style/Loader.cpp#1619
font: http://searchfox.org/mozilla-central/source/layout/style/FontFaceSet.cpp#1391

and they will inherit the principal from 
http://searchfox.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#374

for document, it uses nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10961

However it will have the flag nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL in 
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10975
therefore I add the code there to make the 'inherit' is false if it's data URI and the pref is flipped.
Comment on attachment 8879871 [details] [diff] [review]
Part 2: Revert Bug 1364367

Review of attachment 8879871 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8879871 - Flags: review?(ckerschb) → review+
Comment on attachment 8879874 [details] [diff] [review]
Part 3: Revert Bug 1363634

Review of attachment 8879874 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, r=me
Attachment #8879874 - Flags: review?(ckerschb) → review+
(Assignee)

Comment 13

3 months ago
For font-face
https://drafts.csswg.org/css-fonts-3/#font-fetching-requirements

/* data url's with no redirects are treated as same origin */
src: url("data:application/font-woff;base64,...");


For image
https://html.spec.whatwg.org/multipage/images.html#updating-the-image-data
Step 12
⌛ Fetch request. Let this instance of the fetching algorithm be associated with image request.

This will go to Fetch spec then.


For <link rel="stylesheet" href="data:text/css" ...>
https://html.spec.whatwg.org/multipage/semantics.html#obtaining-a-resource-from-a-link-element
Step 10
Fetch request.

This will also go to Fetch spec then.
(Assignee)

Comment 14

3 months ago
For script
https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-classic-script
Step 3:
If the caller specified custom steps to perform the fetch, perform them on request, with the is top-level flag set. Return from this algorithm, and when the custom perform the fetch steps complete with response response, run the remaining steps.

Otherwise, fetch request. Return from this algorithm, and run the remaining steps as part of the fetch's process response for the response response.


This will again go to Fetch spec.

Comment 15

3 months ago
So should I review something here?
Flags: needinfo?(bugs)
Yoshi, I guess we should incorporate the suggestions from comment 5 and then summarize why we are doing it. I see you have already provided information in various comments. Let's summarize it for smaug again so it's easy for him to get the big picture. Thanks!
Flags: needinfo?(allstars.chh)
(Assignee)

Comment 17

2 months ago
Hi smaug, 
I posted my comment 13 and comment 14, and then I pinged you in the work week and you said that you will check the spec again.
http://logs.glob.uno/?c=mozilla#developers&s=29+Jun+2017&e=29+Jun+2017&h=allstarschh#c1620176

If I need to provide more citations for the spec please let me know
Thanks
Flags: needinfo?(allstars.chh) → needinfo?(bugs)
(Assignee)

Comment 18

2 months ago
(In reply to Yoshi Huang [:allstars.chh] from comment #17)
> http://logs.glob.uno/
> ?c=mozilla#developers&s=29+Jun+2017&e=29+Jun+2017&h=allstarschh#c1620176
> 
The link seems not working, I paste the irc log here
17:08	allstarschh	smaug, anything I need to do in bugzilla.mozilla.org/show_bug.cgi?id=1373513 ?
17:08	allstarschh	smaug, I posted the spec in my latest comments
17:08	smaug	allstarschh: I guess I'll need to read and understand Fetch spec
17:09	smaug	why we in different cases handle data: differently
17:10	allstarschh	smaug, you mean like why Chrome people treating data: document is unique origin in the first place?
17:10	smaug	allstarschh: I mean I need to understand what Fetch spec is saying
17:10	smaug	and how different callers pass data to its algorithms
17:11	smaug	given that it is Fetch spec, nothing is simple ;)
17:11	smaug	I mean, I need to understand how from html spec we end up executing some part of fetch so that data: is treated in some specific way
17:12	allstarschh	smaug, okay, totally agree, :)

Comment 19

2 months ago
Am I supposed to now review something here?
Flags: needinfo?(bugs)
(Assignee)

Updated

2 months ago
Attachment #8879870 - Flags: review- → review?(bugs)

Comment 20

2 months ago
So workers need opaque origin.

Comment 21

2 months ago
Comment on attachment 8879870 [details] [diff] [review]
Part 1: data:image, data:css, and data:fonts should be same origin

Please file a bug to fix workers, if there isn't such yet.
Attachment #8879870 - Flags: review?(bugs) → review+

Comment 22

2 months ago
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3467531e2747
Part 1: data:image, data:css, and data:fonts should be same origin. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/38db3cf25d69
Part 2: Revert Bug 1364367. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7704471f1be7
Part 3: Revert Bug 1363634. r=ckerschb
(Assignee)

Comment 23

2 months ago
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8879870 [details] [diff] [review]
> Part 1: data:image, data:css, and data:fonts should be same origin
> 
> Please file a bug to fix workers, if there isn't such yet.

We already treat Worker from data: URL as an unique opaque origin, 
the web-platform test http://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/webappapis/the-windoworworkerglobalscope-mixin/Worker_Self_Origin.html#34

We already pass the test even without flip the pref.

Comment 24

2 months ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/38db3cf25d69
https://hg.mozilla.org/mozilla-central/rev/7704471f1be7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 25

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3467531e2747
I don't think these patches are quite right.  For example, they change <object data="data:text/html,stuff"> back to inheriting the origin, no?

Also, do we have tests that ensure that data: scripts don't get their errors sanitized?  This patch would fix that, of course, but we want to make sure we don't regress it.

Finally, shouldn't the image/font/style tests be web platform tests, if they're based on the spec?
Flags: needinfo?(allstars.chh)
(Assignee)

Comment 27

2 months ago
(In reply to Boris Zbarsky [:bz] from comment #26)
> I don't think these patches are quite right.  For example, they change
> <object data="data:text/html,stuff"> back to inheriting the origin, no?
> 
Thanks for pointing this out, filed Bug 1381728.

> Also, do we have tests that ensure that data: scripts don't get their errors
> sanitized?  
Sorry but I might not fully understand what you mean by "don't get their errors santized" here.
We have test like dom/base/test/test_script_loader_crossorigin_data_url.html,
 
> 
> Finally, shouldn't the image/font/style tests be web platform tests, if
> they're based on the spec?

So far I found there are only a few web-platform tests will test data: URI,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d4d998659b74aacd75959f8db8837bea58aae98
Filed Bug 1381744, but just fyi right now our goal is to fix the failures in Gecko now(Because the number of failures in still huge).
Flags: needinfo?(allstars.chh)
> Sorry but I might not fully understand what you mean by "don't get their errors santized" here.

I mean, actually show a useful url, line number, message, etc.

> We have test like dom/base/test/test_script_loader_crossorigin_data_url.html,

What about when the "crossorigin" attribute is not set?  That needs to be tested too.

And ideally these would all be web platform tests, of course, since none of this is Gecko-specific in theory.

> but just fyi right now our goal is to fix the failures in Gecko now

I guess my point is that as long as we're touching these tests, we should at the very least be compiling a list of the ones that can move to WPT, if not moving them there...
(Assignee)

Comment 29

2 months ago
(In reply to Boris Zbarsky [:bz] from comment #28)
> > Sorry but I might not fully understand what you mean by "don't get their errors santized" here.
> 
> I mean, actually show a useful url, line number, message, etc.
> 
> > We have test like dom/base/test/test_script_loader_crossorigin_data_url.html,
> 
> What about when the "crossorigin" attribute is not set?  That needs to be
> tested too.
> 
The test failed when I remove 'crossorigin', even when the pref is off, I'll check that and file a bug for it.

Thanks
You need to log in before you can comment on or make changes to this bug.