1428793 - Chrome blocks subresource redirect to data: url as dangerous

Status: RESOLVED FIXED

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

Description

[Khalil Zhani]

This is from bug 949706 - Actually I'm not sure about this bug, the alert is blocked on Chrome, is there something missing?

PoC: http://vulnerabledoma.in/fx_csp_bypass_with_importScripts 

CSP does not block unpermitted resource.

Comment 1

[Daniel Veditz [:dveditz]]

This is blocked on Chrome because they don't allow a redirect to a data: url (I thought we blocked it too!). Nothing to do with CSP (which doesn't mean the CSP behavior isn't wrong, just that it shouldn't come into play here).

Workers don't inherit the loading page's CSP (most of it doesn't even apply to workers), workers need to be served with their own CSP.

https://w3c.github.io/webappsec-csp/2/#processing-model-workers

(actually workers loaded from blob: or data: do inherit, but a worker with a real URL from a real server needs to be sent with a header to get a policy)

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

As discussed, I'll fix that, clearing ni? for me.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Honza, we would like to block insecure redirects to data: URIs. At this point I am not entirely sure whether to block redirects to data: for all subresource loads or just for scripts. Anyway, where in Necko code would be the best place to block such redirects?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

This is where we enforce nsILoadInfo::SEC_DONT_FOLLOW_REDIRECTS:

https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/netwerk/base/nsAsyncRedirectVerifyHelper.cpp#84

Perhaps somewhere around there.

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Honza, we would like to block insecure redirects to data: URIs. At this
> point I am not entirely sure whether to block redirects to data: for all
> subresource loads or just for scripts. Anyway, where in Necko code would be
> the best place to block such redirects?

You should probably do this as part of the CSP checking, likely somewhere around/after [1] (called from [2]).  If not suitable, then [3] could be used to hard-block, but I would not personally suggest that.  CSP is a better place.


[1] https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/dom/security/nsCSPContext.cpp#179
[2] https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/dom/security/nsCSPService.cpp#246
[3] https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/netwerk/protocol/http/HttpBaseChannel.cpp#3399

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [:bkelly] from comment #4)
> This is where we enforce nsILoadInfo::SEC_DONT_FOLLOW_REDIRECTS:
> 
> https://searchfox.org/mozilla-central/rev/
> 48cbb200aa027a0a379b6004b6196a167344b865/netwerk/base/
> nsAsyncRedirectVerifyHelper.cpp#84
> 
> Perhaps somewhere around there.

Could stand as an option between [1] and [3] from the previous comment, yes.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Thanks Ben and Honza. Looking a little closer maybe the right place in the ContentSecurityManager where we perform security checks for redirects [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#584

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> Thanks Ben and Honza. Looking a little closer maybe the right place in the
> ContentSecurityManager where we perform security checks for redirects [1].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/security/
> nsContentSecurityManager.cpp#584

Yes, this looks like the right place!

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1428793_block_insec_redirects_to_data.patch

Hey smaug, this patch blocks insecure redirects to data URIs for scripts. Chrome is doing this and we should do the same in my opinion. Not sure about other types of subresource loads. Happy to block more but I think only script is really worth blocking.

The STR from comment 0 [1] now work correctly and log a message to the console that the load was blocked.

[1] http://vulnerabledoma.in/fx_csp_bypass_with_importScripts

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1428793_test_insec_redirects_to_data.patch

And the automated test for that new behavior.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

This blocks for worker scripts, importScripts, and modules, right?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ben Kelly [:bkelly] from comment #11)
> This blocks for worker scripts, importScripts, and modules, right?

It blocks all redirects to a data: URI for loads that are loaded using the TYPE_SCRIPT, which includes worker scripts as well as importscripts. I don't know what you mean by modules though.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I meant `<script module>`.  I guess that uses TYPE_SCRIPT as well?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ben Kelly [:bkelly] from comment #13)
> I meant `<script module>`.  I guess that uses TYPE_SCRIPT as well?

Oh, I see. Yes it does.

Comment 15

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

Comment on attachment 8943649 [details] [diff] [review]
bug_1428793_test_insec_redirects_to_data.patch

do you need to escape the url here?
And, actually, do you need to unescape in c++ code?


Could you test also what happens when non-escaped data url is used?

Comment 16

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

Comment on attachment 8943649 [details] [diff] [review]
bug_1428793_test_insec_redirects_to_data.patch

I guess this should be r-, since we do need to test also workers and modules and such.

And why the data url is  "<script>alert('this alert should be blocked');</script>"; 
Shouldn't it be just alert('this alert should be blocked');

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #15)
> do you need to escape the url here?
> And, actually, do you need to unescape in c++ code?

I guess so, if we don't unescape the URL then the error log looks like this

Security Error: Content at http://mochi.test:8888/tests/dom/security/test/general/test_block_subresource_redir_to_data.html may not load data from data:text/html,onmessage%20%3D%20function%28event%29%20%7B%20postMessage%28%27worker-loaded%27%29%3B%20%7D

which seems hard to decipher. I would rather keep the unescaping.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1428793_test_insec_redirects_to_data.patch

Hey Ben, I am about to extend the test to not only handle regular script, but also workers and module scripts. If I apply the patch with the code changes within the contentSecurityManager to stop redirects to data: URIs then I get three tests that pass.

If I do the opposite and just run the test without the actual code changes applied, then I observe the following:
* regular script tests loads (as expected)
* worker script gets blocked, because workers need to be same origin (exepcted, but not sure how much sense it makes to keep that subtest within that file)
* script modules gets blocked with the following error in the console:

Module source URI is not allowed in this document: “http://mochi.test:8888/tests/dom/security/test/general/file_block_subresource_redir_to_data.sjs?modulescript”.

I tried many things, but I can't get even a regular module script to load; not even without any redirect testing. Am I missing something? Asked differently, can you guide me to a test that should work?

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)
> * worker script gets blocked, because workers need to be same origin
> (exepcted, but not sure how much sense it makes to keep that subtest within
> that file)

Interesting.  We do allow data URL workers with an opaque origin.  Its interesting that the same-origin redirect check gets to it first.  In theory we could let redirect to data URL from secure origins work in worker scripts.  I'm not sure sure its worth the effort, though.

I'm happy to leave the test for now, though.  We want something that verifies its blocked, regardless of the reason.

> * script modules gets blocked with the following error in the console:
> 
> Module source URI is not allowed in this document:
> “http://mochi.test:8888/tests/dom/security/test/general/
> file_block_subresource_redir_to_data.sjs?modulescript”.
> 
> I tried many things, but I can't get even a regular module script to load;
> not even without any redirect testing. Am I missing something? Asked
> differently, can you guide me to a test that should work?

Probably want to ask Jon about testing modules.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Olli, would you be fine with me landing the change only using the testcase for regular script and workers and file a follow up for script modules? Given the merge date I would like to get that landed?

Comment 21

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

Sounds ok

Comment 22

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

But try to find the bug to enable modules in release builds and file a bug to add the test and make that to block the enabling bug.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1428793_test_insec_redirects_to_data.patch

Olli, I filed Bug 1432137 for adding the script module test. Could you please green light this test? Thanks!

Comment 24

[Jon Coppeard (:jonco)]

Comment on attachment 8944385 [details] [diff] [review]
bug_1428793_test_insec_redirects_to_data.patch

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

::: dom/security/test/general/test_block_subresource_redir_to_data.html
@@ +31,5 @@
> +testScriptRedirectToData.onload = function() {
> +  ok(false, "script that redirects to data: URI should not load");
> +  checkFinish();
> +}
> +testScriptRedirectToData.src = "file_block_subresource_redir_to_data.sjs?script";

I'm confused by this.  Does setting the 'src' attribute for a script that has already been processed make it get loaded again?  Why does that happen?

Note modules are enabled by default in nightly only at the moment, otherwise you'll need to set dom.moduleScripts.enabled.

Comment 25

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

That src attribute setting works because of 
https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/dom/script/ScriptElement.cpp#125 See HasScriptContent() call.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6f3f1ffb41
Block insecure redirects to data: URIs. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccef1ce1cb68
Test block insecure redirects to data: URIs. r=smaug

Comment 27

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/bb6f3f1ffb41
https://hg.mozilla.org/mozilla-central/rev/ccef1ce1cb68

Comment 28

[Jon Coppeard (:jonco)]

(In reply to Olli Pettay [:smaug] from comment #25)
Thanks.

Comment 29

[[e-mail]]

I am pretty sure the change here is what breaks uBlock Origin's ability to redirect blocked network requests to local neutered scripts (through data: URIs).

This is a big deal, as redirection is used by uBlock Origin for key core features such as:

- Preventing site breakages as a result of blocking scripts
- Foiling anti-blocker mechanisms

Consider this issue: https://github.com/uBlockOrigin/uAssets/issues/1370

I get this at the browser console:

> Redirecting to insecure data: URI not allowed
> (Blocked loading of: “data:application/javascript;base64,KGZ1bmN0aW9uKCk...”)

uBO's anti-blocker abilities no longer work with Nightly.
It still work fine for FF58, Chromium 63, Chrome 65.

I would appreciate insights about I could restore these uBO core features in Nightly given the changes here.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Hm, interesting. I am wondering what kind of distinction Chrome makes when blocking redirects as insecure and when not, given that Chrome also blocks the POC from comment 0:
  http://vulnerabledoma.in/fx_csp_bypass_with_importScripts 

I guess that needs some further debugging. I'll take a look and post an update here!

Comment 31

[[e-mail]]

Chromium will also block uBO's redirects to data: URIs *if* a site declares a CSP header which prevent the use of data: URIs. This is actually an open issue on Chromium's issue tracker[1].

However the site provided as having the issue[2] does not declare any CSP header, so it appears the change here affects all redirections to data: URIs, even without a CSP directive forbidding data: URIs.

Consider this very simple test case, in which uBO blocks and redirects the Google Analytics script:
http://motherfuckingwebsite.com/

It does not work with Nightly, even though the site does not declare any CSP header.

***

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=749236
[2] https://github.com/uBlockOrigin/uAssets/issues/1370

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

(In reply to [e-mail] from comment #31)
> Chromium will also block uBO's redirects to data: URIs *if* a site declares
> a CSP header which prevent the use of data: URIs.

Really, that's not what I am observing when visiting http://vulnerabledoma.in/fx_csp_bypass_with_importScripts. I gotta setup some more test sites but it seems that they are blocking insecure redirects to data: URIs regardless of the CSP.

Comment 33

[Daniel Veditz [:dveditz]]

(In reply to [e-mail] from comment #31)
> Chromium will also block uBO's redirects to data: URIs *if* a site declares
> a CSP header which prevent the use of data: URIs. This is actually an open
> issue on Chromium's issue tracker[1].

Where do your scripts come from? If they're fixed could you use moz-extension: urls and bundle them? That would solve your Firefox issue _and_ your Chrome issue.

I bet they come from dynamic filter lists though, so maybe not possible. Turn the data into blob: urls? That would evade the Firefox block. Might even work around your Chrome issue because I think they track blob: origin and might give a free-pass to ones they know were created in an extension context.

Comment 34

[Daniel Veditz [:dveditz]]

We should file a _different_ bug "uBlock Origin neutered scripts broken by preventing redirects to data: urls" or something rather than carry on this conversation here.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Please note that redirecting to a blob URI might still be blocked by CSP because blobs needs to be explicitly whitelisted within CSP.

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Raymond, what do you think about comment 33? Could that work?
FWIW, we are about to file a new bug to make sure uBlock Origin continues to work within Firefox.

Comment 37

[Daniel Veditz [:dveditz]]

Filed bug 1434357 for this breakage.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #36)
> Raymond, what do you think about comment 33? Could that work?
> FWIW, we are about to file a new bug to make sure uBlock Origin continues to
> work within Firefox.

Clearing ni? on this bug, since the conversation continues within bug 1434357.