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

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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]

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: data:image, data:css, and data:fonts should be same origin

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Revert Bug 1364367

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: Revert Bug 1363634

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(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

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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 11

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8879871 [details] [diff] [review]
Part 2: Revert Bug 1364367

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

r=me

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8879874 [details] [diff] [review]
Part 3: Revert Bug 1363634

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

lgtm, r=me

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

[Olli Pettay [:smaug][[e-mail]]]

So should I review something here?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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

[Olli Pettay [:smaug][[e-mail]]]

Am I supposed to now review something here?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

So workers need opaque origin.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38db3cf25d69
https://hg.mozilla.org/mozilla-central/rev/7704471f1be7

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3467531e2747

Comment 26

[Boris Zbarsky [:bzbarsky]]

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?

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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).

Comment 28

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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