1434357 - uBlock Origin neutered scripts broken by preventing redirects to data: urls

Status: RESOLVED FIXED

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

Description

[Daniel Veditz [:dveditz]]

uBlock Origin has stub scripts to replace some blocked content, and they are served as data: urls. This was broken when we blocked redirects to data: url script subresources as dangerous in bug 1428793. We may need to back that out or help uBO come up with a workaround (bonus if it helps them out on pages with CSP)

Comment 1

[[e-mail]]

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

Yes, these are shipped as separate assets like filter lists.

I can use blob:, there is only one single place where this is done. However I will use browser/version sniffing, I can't afford to risk destabilizing uBO for all other browsers or older versions of all browsers.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Please remember to call `revokeObjectURL()` after you use the blob URL.  Sadly its an all too-easy-to-trigger memory leak in the API.  Sorry for the spam if you were already aware of this.

Comment 3

[[e-mail]]

So I modified the code to redirect to blob URLs, but it's not working, I am getting security error messages at the browser console.

Take the following example web page (disregard the site's name and web page's content -- it's just a very simple page with which I can reproduce the issue while using uBO as is): http://motherfuckingwebsite.com/

The page tries to load a script from Google Analytics, and uBO tries to redirect that GA script to its neutered scriptlet. Below is the result at the browser console:

    Security Error: Content at http://motherfuckingwebsite.com/ may not load data from blob:moz-extension://fb7296ff-aef4-4304-98f4-fe5ac3e6956e/d36cbb5c-30fa-4d50-a294-59d665459296.
    <script> source URI is not allowed in this document: “http://www.google-analytics.com/analytics.js”.  motherfuckingwebsite.com:1

Consider:

- The page does not declare any CSP header
- The page does not declare any "http-equiv" for "content-security-policy" in its source
- uBO does not inject any CSP header for that page
- "fb7296ff-aef4-4304-98f4-fe5ac3e6956e" confirms this is a blob URI from uBO

So the use of blob URI fails the same way as when using data URI.

Am I missing something?

Comment 4

[[e-mail]]

Privacy Badger and Ghostery are two other extensions also affected by the now forbidden redirection-to-data-URI issue (try above test page and see browser console output). That uBO appear more prominently affected is because the redirection ability is used to work around anti-blockers.

Comment 5

[[e-mail]]

I will just reiterate to be sure it's taken into account that extensions are not forbidden to redirect to data: URIs with Chromium (including current dev version of Chrome).

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

My guess is the blob URL doesn't work because we don't permit cross-origin blob URL loads.

Christoph, what do you think should be done here?  Can we exempt extensions from the new restrictions somehow?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Or Andrea, do you think it would be possible to support blob URLs here?  Note that we don't really want to support cross-origin blob URLs for content scripts.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

It also occurs to me that this "redirect to data URL" approach is basically a work-around for not have a way to provide a synthetic response body via an API.  Chrome is working towards using service workers for that, but I'm pretty opposed to implementing that at the moment.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to [e-mail] from comment #5)
> I will just reiterate to be sure it's taken into account that extensions are
> not forbidden to redirect to data: URIs with Chromium (including current dev
> version of Chrome).

Let me see what I can do to make that work for extensions.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ben Kelly [:bkelly] from comment #6)
> Christoph, what do you think should be done here?  Can we exempt extensions
> from the new restrictions somehow?

Yeah, let me see how easily we can do that. I have to reach out to a few extensions folks.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Chris, Andy, I don't know who the right person to ask this question is - maybe you can forward accordingly. Anyway, within Bug 	1428793 we started to block redirects of type script to insecure data: URIs. It seems that various extensions make use of such redirects to data: URIs however. I think it would be fine to exempt redirects triggered by a web extension from that restriction. I think we could set some flag on the channel, or the loadInfo of the channel annotating that the redirect was triggered by a web extension. I assume there is some code similar to AsyncOnChannelRedirect() where all web extension redirects pass through, right? Alternatively, with what privileges to web extensions run? Maybe we can base the exemption on the principal? I am wide open to suggestions - thanks!

Comment 12

[Andy McKay]

I think Kris might be best person to help out on this one.

Comment 13

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> I think it
> would be fine to exempt redirects triggered by a web extension from that
> restriction. I think we could set some flag on the channel, or the loadInfo
> of the channel annotating that the redirect was triggered by a web
> extension. I assume there is some code similar to AsyncOnChannelRedirect()
> where all web extension redirects pass through, right? Alternatively, with
> what privileges to web extensions run? Maybe we can base the exemption on
> the principal? I am wide open to suggestions - thanks!

This is a complicated question. Technically WebExtensions code runs with a content principal that we attach a WebExtensionPolicy object to. Request modifications happen in the parent process, though, and aren't really tied to an extension principal.

I think the simplest way to handle this from our side is to exempt API redirects from this restriction. That would be inline with our other CSP exemptions for extension callers.

The easiest way to handle it from the extension side would be to use filterResponseData to replace the response rather than redirecting.

Comment 14

[[e-mail]]

> The easiest way to handle it from the extension side would be to use filterResponseData to replace the response rather than redirecting.

That does not work: the feature currently broken is "block and redirect". The "block" part is the most important. For example, I don't want the connection to Google Analytics to be made at all. The redirect part is to minimize breakage as a result of blocking Google Analytics.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1434357_allow_insec_redirects_for_extensions.patch

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #13)
> This is a complicated question. Technically WebExtensions code runs with a
> content principal that we attach a WebExtensionPolicy object to.

Hey Kris, thanks for your reply. I haven't tested this at all yet, but if I understand you correctly, we could check if the principal that triggered the load has an extension policy applied, right? This is what I have been trying in this refactored method. What do you think? Could that work?

From what I checked, this is the same mechanism we use within nsDocument [1] when applying a CSP. The only difference is, that nsDocument uses the NodePrincipal which is the same as the loadingPrincipal within the loadInfo. I think we should use the triggeringPrincipal, because it's the principal that triggered the load.

Anyway, my question in general is: Could that work or am I missing something?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2952

Comment 16

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> Created attachment 8947787 [details] [diff] [review]
> bug_1434357_allow_insec_redirects_for_extensions.patch
> 
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #13)
> > This is a complicated question. Technically WebExtensions code runs with a
> > content principal that we attach a WebExtensionPolicy object to.
> 
> Hey Kris, thanks for your reply. I haven't tested this at all yet, but if I
> understand you correctly, we could check if the principal that triggered the
> load has an extension policy applied, right? This is what I have been trying
> in this refactored method. What do you think? Could that work?
> 
> From what I checked, this is the same mechanism we use within nsDocument [1]
> when applying a CSP. The only difference is, that nsDocument uses the
> NodePrincipal which is the same as the loadingPrincipal within the loadInfo.
> I think we should use the triggeringPrincipal, because it's the principal
> that triggered the load.
> 
> Anyway, my question in general is: Could that work or am I missing something?

Will it work? No. Can it work? Maybe...

The problem is that we don't actually change the triggering principal when we initiate an API redirect:

https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/netwerk/protocol/http/HttpBaseChannel.cpp#2196-2213

We do track whether the redirect was an API-based redirect or a header-based redirect, so we can (and do) treat the two differently in some places, but the triggering principal remains the same.

It *might* be a good idea to change the triggering principal to the principal of the extension that initiated the redirect. It would certainly have some advantages in being able to make finer-grained decisions about what redirects are OK (especially redirects to moz-extension: URLs, and blob: URLs with extension principals). But I haven't thought through all of the implications of that.

If we do go that route, though, then I think your solution would work. Otherwise, I think we'd have to have to fallback to exempting all API redirects.

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8947787 [details] [diff] [review]
bug_1434357_allow_insec_redirects_for_extensions.patch

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

I think this is a good solution if we decide to start changing the triggering principal on redirect, but that would be a prerequisite.

We'd need feedback from a necko peer for that change, though.

Comment 19

[Caspy7]

I didn't read every comment, so apologies if I missed it.
In bug 1435611, which was just duped on this one, I noted that tracking protection *also* broke the pagination script at the bottom of a page ( https://www.onmeda.de/heilpflanzen/johanniskraut.html ). 

I just wanted to add that datapoint in case this was viewed as a webextension only issue - or bug 1435611 is a different problem.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #16)
> It *might* be a good idea to change the triggering principal to the
> principal of the extension that initiated the redirect. It would certainly
> have some advantages in being able to make finer-grained decisions about
> what redirects are OK (especially redirects to moz-extension: URLs, and
> blob: URLs with extension principals). But I haven't thought through all of
> the implications of that.

Now that I think about that again, I am getting seconds thoughts about changing the triggeringPrincipal. One of the promises of the loadInfo is that the triggeringPrincipal will never change throughout the lifetime of a channel. Maybe we can re-evaluate that decision in a different bug. Since this bug needs to be uplifted as well I think we should work on a simpler solution.

@Honza: Do you happen to know where in the redirect code we know that the redirect was triggered by an web-extension? If we know that place, then maybe it's simpler to just add some flag to the loadInfo to exempt the redirect blocking.

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #16)
> > It *might* be a good idea to change the triggering principal to the
> > principal of the extension that initiated the redirect. It would certainly
> > have some advantages in being able to make finer-grained decisions about
> > what redirects are OK (especially redirects to moz-extension: URLs, and
> > blob: URLs with extension principals). But I haven't thought through all of
> > the implications of that.
> 
> Now that I think about that again, I am getting seconds thoughts about
> changing the triggeringPrincipal. One of the promises of the loadInfo is
> that the triggeringPrincipal will never change throughout the lifetime of a
> channel. Maybe we can re-evaluate that decision in a different bug. Since
> this bug needs to be uplifted as well I think we should work on a simpler
> solution.
> 
> @Honza: Do you happen to know where in the redirect code we know that the
> redirect was triggered by an web-extension? If we know that place, then
> maybe it's simpler to just add some flag to the loadInfo to exempt the
> redirect blocking.

And what all ways can a web-extension trigger a redirect?  This is hard and I don't think http channel should even know about this.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #16)
> We do track whether the redirect was an API-based redirect or a header-based
> redirect, so we can (and do) treat the two differently in some places, but
> the triggering principal remains the same.

Hey Kris, before moving forward with this bug in any direction, I was wondering if you could guide me to the code where we treat API based redirects differently. Maybe that gives us some more ideas of what we can do here do exempt redirects to data: URIs being blocked for extensions. Thanks!

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Andrew, Giorgio, we would like to exempt web extensions from our 'insecure redirect to a data: URI blocker'. It seems there are various APIs that would allow a web extension to redirect to a data: URI. I am not sure if we can exempt all of them; maybe you know better than I do. Anyway, could you guide me to the webrequest API (which I think uBlock is using for their redirects). Within that redirect code I think we could just add a flag to the loadInfo which we can then check in our blocking code to wave those redirects through.

What do you think-could that work?

Comment 24

[Andrew Swan [:aswan]]

Hi, I believe the redirect actually happens here:
https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/toolkit/modules/addons/WebRequest.jsm#833-841

I don't know how practical extending that interface is, somebody closer to necko would probably be in a better position to evaluate that.  Also I'm happy to help however I can but both Kris (who is already cc'ed on this bug) and Shane (:mixedpuppy) know this code from the webextensions side much better than I do...

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrew Swan [:aswan] from comment #24)
> Hi, I believe the redirect actually happens here:
> https://searchfox.org/mozilla-central/rev/
> 9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/toolkit/modules/addons/WebRequest.
> jsm#833-841

Thanks, I think it makes sense to just toggle some kind of flag there, similar to:
channel.loadInfo.allowRedirectToDataURI = true;
and then just wave things through.

:rhill, would that make uBlock work again? I mean, just to make sure, is uBlock using Webrequest API for their redirects?

Comment 26

[[e-mail]]

> is uBlock using Webrequest API for their redirects?

Yes.

Comment 27

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #22)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #16)
> > We do track whether the redirect was an API-based redirect or a header-based
> > redirect, so we can (and do) treat the two differently in some places, but
> > the triggering principal remains the same.
> 
> Hey Kris, before moving forward with this bug in any direction, I was
> wondering if you could guide me to the code where we treat API based
> redirects differently. Maybe that gives us some more ideas of what we can do
> here do exempt redirects to data: URIs being blocked for extensions. Thanks!

We process ordinary HTTP redirects here:

https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/netwerk/protocol/http/nsHttpChannel.cpp#5529-5600

vs. API redirects here:

https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/netwerk/protocol/http/nsHttpChannel.cpp#2803-2851

There are also some places where we set the REDIRECT_INTERNAL flag, and treat those redirects differently from redirect verify callbacks, but we don't set that flag for extension-generated redirects (except for HTTPS upgrades).

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1434357_allow_insec_redirects_for_extensions.patch

Kris, Honza, what do you think. Acceptable? We would then also need an automated test for that, can someone guide me to some other tests which make use of the webrequest API?

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8950667 [details] [diff] [review]
bug_1434357_allow_insec_redirects_for_extensions.patch

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

::: netwerk/base/LoadInfo.cpp
@@ +345,5 @@
>    , mUpgradeInsecureRequests(rhs.mUpgradeInsecureRequests)
>    , mVerifySignedContent(rhs.mVerifySignedContent)
>    , mEnforceSRI(rhs.mEnforceSRI)
>    , mForceAllowDataURI(rhs.mForceAllowDataURI)
> +  , mAllowInsecureRedirectToDataURI(rhs.mAllowInsecureRedirectToDataURI)

Should we clear this when we clone for redirect?

::: toolkit/modules/addons/WebRequest.jsm
@@ +832,5 @@
>  
>          if (result.redirectUrl) {
>            try {
>              channel.suspended = false;
> +            channel.loadInfo.allowInsecureRedirectToDataURI = true;

We should probably only do this if the redirectUrl is a data: URL. Otherwise an HTTP redirect to a data: URL down the line will still work even when it shouldn't.

Comment 30

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #28)
> Kris, Honza, what do you think. Acceptable? We would then also need an
> automated test for that, can someone guide me to some other tests which make
> use of the webrequest API?

This is probably as good a starting point as any:

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_webrequest_upgrade.html

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 8950667 [details] [diff] [review]
bug_1434357_allow_insec_redirects_for_extensions.patch

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

::: toolkit/modules/addons/WebRequest.jsm
@@ +837,1 @@
>              channel.redirectTo(Services.io.newURI(result.redirectUrl));

hmm.... something tells me this is not the best approach...  when someone calls redirectTo on the same channel with null before opening the channel, and the response is a redirect to data URI, we will allow it and it could be possibly harmful.  This is one possible code path (that needs some bad extension interaction, or some kind of an omission in our code base) that could override the no-redit-to-data-URI strict safety measure.  There could be more such paths, I'm afraid.  The "allow" flag hangs on a bad object and is not tight well to the redirect target the channel may end up at.

Better would be if uBlock provided the content via moz-extension urls, we allow redirects to it.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Honza Bambas (:mayhemer) from comment #31)
> Better would be if uBlock provided the content via moz-extension urls, we
> allow redirects to it.

:rhill, would that work for uBlock? Could you use a moz-extension url instead?

Comment 33

[[e-mail]]

> Could you use a moz-extension url instead?

This has been proposed earlier, see comment #1, in which I quoted from bug 1428793#c33:

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

It's not possible because the response to redirect to is dynamic, hence why data: URI works well for this case.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Honza Bambas (:mayhemer) from comment #31)
> hmm.... something tells me this is not the best approach...  when someone
> calls redirectTo on the same channel with null before opening the channel,
> and the response is a redirect to data URI, we will allow it and it could be
> possibly harmful.  This is one possible code path (that needs some bad
> extension interaction, or some kind of an omission in our code base) that
> could override the no-redit-to-data-URI strict safety measure.  There could
> be more such paths, I'm afraid.  The "allow" flag hangs on a bad object and
> is not tight well to the redirect target the channel may end up at.

Honza, I see your concern but until recently we did not stop insecure redirects to data: URIs. I am personally more worried about web pages than extensions, and regular web pages can not access the webrequest API, right?. It seems moz-extension: does not work for uBlock and we need a way to move forward. How about if we only set the flag in the loadinfo if the original channel was http/https? That would somehow mitigate that risk, right? Alternatively we have the whole redirect chain available within the loadInfo, so we could even do some more sophisticated checks if needed. Would that be an option here?

Comment 35

[Honza Bambas (:mayhemer)]

Comment on attachment 8950667 [details] [diff] [review]
bug_1434357_allow_insec_redirects_for_extensions.patch

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

Hmm.. there is a way(s) AFAIK to generate content with the service workers API, but I may just mix apples and tomatoes.

Based on c34, let's take this patch with few small tweaks:
- let the channel's redirectTo implementation drop the this->loadInfo->allowInsecureRedirectToDataURI to false
- set loadInfo.allowInsecureRedirectToDataURI to true in WebRequest.jsm AFTER you called redirectTo

redirectTo is currently the only way to remove the redirectTo URI (and allow the default processing of the response which can be a redirect) or redir to a yet different (potentially data: and potentially harmful) URI.  hence, if we drop loadInfo.allowInsecureRedirectToDataURI there, always, my concern is fixed with it as it can be true only when set by the WebRequest code.

r+ with this addressed

Comment 36

[Honza Bambas (:mayhemer)]

And please add good explanatory comments into the code, thanks.

Comment 37

[[e-mail]]

For what it's worth, I will try the moz-extension:/chrome-extension: approach (or at least a mixed approach to minimize reliance on data: URIs for redirection purpose as much as possible). The resources used for redirection currently change rarely, and in any case I am not allowed to have them update dynamically on Firefox (AMO policy).

One major concern with the web_accessible_resources approach is that unlike Firefox, the extension id is not randomized with Chromium and thus it's possible for web pages to identify that a specific extension is in use. However I believe I found a solution to prevent this, and this opens the door for me to use the approach. As a virtuous side effect this would also solve the other issue which occurs when a page forbid the use of data: URI resources through a content security policy.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Hey Honza, I wrote a testcase which exercises the codepath, but something strange is happening. In particular, we get two calls to ::AsyncOnChannelRedirect() [1], where the first time we receive a call in ::AsyncOnChannelRedirect() the loadInfo has the flag set to true, but the second time around we receive a call into ::AsyncOnChannelRedirect (within the content process) that flag is not set to true. I verified multiple times that I do the propagation correct within BackgroundUtils.cpp (see patch). I have done that propagation several times for other flags (e.g. upgradeinsecurerequests and what not) where that used to work just fine. Something strange is going on and I can't figure out why. I have realized we are receiving a call to ChannelWrapper::RedirectTo. Ultimately I assume it must have something to do with the web extension code. Is there something in particular I have to take care of regarding loadinfo and webrequests? I spend several hours debugging but it seems it's not going anywhere. Any ideas what might be wrong here?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#585
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/webrequest/ChannelWrapper.cpp#121

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: test_wip.patch

FWIW, here is the WIP test.

Comment 40

[Giorgio Maone [:ma1]]

I've been out of the loop of WebExtensions internals from the developer point of view for a while, so I'm not sure I can help with implementation.
However, as a consumer of the API, I suspect uBlock's use case, especially if the response needs to be dynamic, could be served my webReques.filterResponseData() - https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/filterResponseData

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Update to comment 38: Since Web Extensions do not run in the child it seems that we do not pass through WebRequest.jsm for the child. The parent loadinfo gets flaged correctly, but the child loadInfo does not pass through WebRequest.jsm but we still call AsyncOnChannelRedirect for parent and child. I don't know what the best way is to propagate the flag from parent to child loadinfo.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #41)
> Update to comment 38: Since Web Extensions do not run in the child it seems
> that we do not pass through WebRequest.jsm for the child. The parent
> loadinfo gets flaged correctly, but the child loadInfo does not pass through
> WebRequest.jsm but we still call AsyncOnChannelRedirect for parent and
> child. I don't know what the best way is to propagate the flag from parent
> to child loadinfo.

You probably need to pass the information through in SendRedirect1Begin(), etc.

Comment 43

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [:bkelly] from comment #42)
> You probably need to pass the information through in SendRedirect1Begin(),
> etc.

Yes, that is one option.  Other is to change how we create load info for the target channel on the child process (I think worth doing).

The problem is at [1].  We clone the child source (aka old) channel load info there.  Hence, even though you modify the old channel's load info in WebRequest.jsm on the parent process before we clone it for the new (target) channel (on the parent), and your new flag thus gets copied to the parent target channel's load info, the change to load info of the old channel is not propagated.  So, on the child we clone from an unmodified (not up to date) load info.

Hence, I think, we may need to send the whole new channel's load info serialization along other args of SendRedirect1Begin, update it on the old channel on child, and clone from that updated load info for the new channel.

I'll file a bug for it, since it's IMO worth fixing independently of this bug, it seems more correct to me that what we do now.  Tho, it could reveal some hidden bugs, so not a small change!

If you want to go the safe way, just send the flag as Ben suggested in comment 42 and set it on the old channel's (|this|) load info before [1].


[1] https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/netwerk/protocol/http/HttpChannelChild.cpp#1716

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #43)
> (In reply to Ben Kelly [:bkelly] from comment #42)
> > You probably need to pass the information through in SendRedirect1Begin(),
> > etc.
> 
> Yes, that is one option.  Other is to change how we create load info for the
> target channel on the child process (I think worth doing).
> 
> The problem is at [1].  We clone the child source (aka old) channel load
> info there.  Hence, even though you modify the old channel's load info in
> WebRequest.jsm on the parent process before we clone it for the new (target)
> channel (on the parent), and your new flag thus gets copied to the parent
> target channel's load info, the change to load info of the old channel is
> not propagated.  So, on the child we clone from an unmodified (not up to
> date) load info.
> 
> Hence, I think, we may need to send the whole new channel's load info
> serialization along other args of SendRedirect1Begin, update it on the old
> channel on child, and clone from that updated load info for the new channel.
> 
> I'll file a bug for it, since it's IMO worth fixing independently of this
> bug, it seems more correct to me that what we do now.  Tho, it could reveal
> some hidden bugs, so not a small change!

If we do this there is some code I added to flow a specific LoadInfo value back through OnStartRequest here:

https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/netwerk/protocol/http/HttpChannelParent.cpp#1537-1552

That bit of code could go away if we serialized back the LoadInfo for OnStartRequest as well.

Comment 45

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [:bkelly] from comment #44)
> If we do this there is some code I added to flow a specific LoadInfo value
> back through OnStartRequest here:
> 
> https://searchfox.org/mozilla-central/rev/
> 74b7ffee403c7ffd05b8b476c411cbf11d134eb9/netwerk/protocol/http/
> HttpChannelParent.cpp#1537-1552
> 
> That bit of code could go away if we serialized back the LoadInfo for
> OnStartRequest as well.

It's a different point to update load info on a channel, probably worth yet another bug :)

Comment 46

[Honza Bambas (:mayhemer)]

Also, we don't call OnStartRequest on child until redirection pre-handling is done.  And passing LI there would be too late for the issue here to fix.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #46)
> Also, we don't call OnStartRequest on child until redirection pre-handling
> is done.  And passing LI there would be too late for the issue here to fix.

I know.  I was more just speaking to the use cases for "parent changes to LoadInfo need to be reflected in the child process".

Comment 48

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1434357_allow_insec_redirects_for_extensions.patch

Honza, do you mind taking another look at this patch? Thanks for filing Bug 	
1438935; I agree that it's worth fixing to pass the loadInfo through SendRedirect1Begin. For this patch however, since it needs to be uplifted I just pass around the one flag needed.

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1434357_test_allow_insec_redirects_for_extensions.patch

Jonathan, do you mind taking a look at that test? It took me quite some time to write that test and I am happy to incorporate suggestions, but I think there is no good way of veryfing that the data: URI redirect occured. I tried using scriptEl.src and what not, but the .src for example remains the initial URL of the load, not the reidrected one. Hence I ended up with that detour of letting the script update a div container. As I said, happy to update if there are better ideas to write that automated test.

Comment 50

[Jonathan Kingston [:jkt] he/him]

This seems fine to me however I don't have a huge tonne of writing these kinds of extension tests anyway.
I assume using window.addEventListener("message", (event) => { and window.postMessage didn't work. I don't really think this improves the test that much anyway.

Is there any security concerns we should be aware of with permitting extensions from redirecting to data: urls? Should we consider requiring a "data:" permission from the extension?

Comment 51

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonathan Kingston [:jkt] from comment #50)
> This seems fine to me however I don't have a huge tonne of writing these
> kinds of extension tests anyway.
> I assume using window.addEventListener("message", (event) => { and
> window.postMessage didn't work. I don't really think this improves the test
> that much anyway.

Yeah, I tried that, but it didn't work :-*(

> Is there any security concerns we should be aware of with permitting
> extensions from redirecting to data: urls? Should we consider requiring a
> "data:" permission from the extension?

We can consider doing that in the future. Potentially we should file a new bug where we can discuss options. For this bug however (since it needs to be uplifted) I think we should just focus on getting the actual problem fixed here.

Comment 52

[Honza Bambas (:mayhemer)]

Comment on attachment 8952006 [details] [diff] [review]
bug_1434357_allow_insec_redirects_for_extensions.patch

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

thanks.

::: dom/security/nsContentSecurityManager.cpp
@@ +135,5 @@
> +  if (loadInfo->GetAllowInsecureRedirectToDataURI()) {
> +    return true;
> +  }
> +
> +  nsAutoCString dataSpec;

hmmm... if we use nsCString, will the buffer be shared?  because data: specs can be really (!!!) large and any allocations that can be avoided should be avoided.

Comment 53

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8952008 [details] [diff] [review]
bug_1434357_test_allow_insec_redirects_for_extensions.patch

SGTM, didn't test but it looks like it's checking the right thing. Thanks

Comment 54

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Honza Bambas (:mayhemer) from comment #52)
> hmmm... if we use nsCString, will the buffer be shared?  because data: specs
> can be really (!!!) large and any allocations that can be avoided should be
> avoided.

I agree that we should try to avoid that wherever we can and I think nsCString is shared, but I am also not 100% sure. I'll follow up on that pattern in a separate bug because we are doing such data: URI checks elsewhere in the codebase as well. The good thing is that we first check for the scheme and in this case we only do it for loads of type script. Anyway, I'll follow up on that.

Comment 55

[Honza Bambas (:mayhemer)]

None of the patches can be applied cleanly AT ALL on up to date m-c...

Comment 56

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Because bug 1432358 landed.

Comment 57

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #56)
> Because bug 1432358 landed.

Ah, of course. Sorry, I'll take care of it.

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/416adbc7c8e7
Exempt Web Extensions from insecure redirects to data: URIs. r=kmag,mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/380b630d362a
Test allow insecure redirect to data: URI through WebRequest API. r=jkt

Comment 59

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/416adbc7c8e7
https://hg.mozilla.org/mozilla-central/rev/380b630d362a

Comment 60

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8952006 [details] [diff] [review]
bug_1434357_allow_insec_redirects_for_extensions.patch

Approval Request Comment
Bug 1428793 starts to block insecure redirects to data: URIs for loads of TYPE_SCRIPT. This change cases certain extension (e.g. uBlock) to stop working because they rely on redirecting to a data: URI when a resource gets blocked.

[Feature/Bug causing the regression]:
Bug 1428793

[User impact if declined]:
Extensions like uBlock don't work.

[Is this code covered by automated tests?]:
Yes, Bug 1428793 added automated tests for the correct blocking of insecure redirects to data: URIs on regular web pages and this bug adds a test exercising the codepath that extensions trigger.

[Has the fix been verified in Nightly?]:
Nope, not yet.

[Needs manual test from QE? If yes, steps to reproduce]: 
No, I guess Ublock maintainers (CCed) on this bug will verify the fix for us.

[List of other uplifts needed for the feature/fix]:
nope.

[Is the change risky?]:
no

[Why is the change risky/not risky?]:
only allows web extensions to redirect to a data: URI which gets blocked on regular web pages.

[String changes made/needed]:
no

Comment 61

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8952008 [details] [diff] [review]
bug_1434357_test_allow_insec_redirects_for_extensions.patch

Approval Request Comment
see previous comment.

Comment 62

[Liz Henry (:lizzard) (relman/hg->git project)]

Christoph, it looks like the patches in bug 1428793 only landed for 60.  
Does this issue actually affect 59 (beta)?

Comment 63

[Liz Henry (:lizzard) (relman/hg->git project)]

Note, I emailed Alexei who is a maintainer for Privacy Badger to let him know.

Comment 64

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #62)
> Christoph, it looks like the patches in bug 1428793 only landed for 60.  
> Does this issue actually affect 59 (beta)?

Oh sorry, that was my mistake then. I blame the jetlag here. In that case there is no need for an uplift of course - sorry for the confusion.