1211967 - Fix how we report errors when loading a worker from a data url.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Andrea Marchesini [:baku]]

data:text/html,<script>var%20w%20=%20new%20Worker("data:application/javascript,%&%^&%^");</script>

This URL doesn't report any error into the console.

Comment 1

[Boris Zbarsky [:bzbarsky]]

It's not just the console.  It doesn't fire an error event on the worker either, I expect.

The reason for that is that in ScriptLoaderRunnable::OnStreamCompleteInternal we have an nsNullPrincipal as channelPrincipal, so it's not subsumed by principal.

Do we not inherit the principal for data: loads in workers?  If not, why not?  I suppose we could never mute for toplevel worker loads, since those are already subject to same-origin policy.  Or something.  Inheriting the principal for data: seems cleaner, in case we ever introduce cross-origin workers.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - AsyncOpen2 must have 2 params as AsyncOpen has.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - sec flags in ScriptLoader.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8673058 [details] [diff] [review]
part 2 - sec flags in ScriptLoader.

Seems ok, though I assume we want data: to inherit for non-main scripts too, right?  Otherwise we'll mute errors from importScripts(["data:whatever"]) which we don't want to do.

The fact that there is no loadinfo flag for this behavior seems broken to me; you probably want to talk to Christoph about that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> The fact that there is no loadinfo flag for this behavior seems broken to me

And that's because there _is_ such a flag.  SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8673058 [details] [diff] [review]
part 2 - sec flags in ScriptLoader.

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

::: dom/workers/ScriptLoader.cpp
@@ +161,5 @@
>    }
>  
>    aLoadFlags |= nsIChannel::LOAD_CLASSIFY_URI;
> +  uint32_t secFlags = aIsMainScript ? nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS
> +                                    : nsILoadInfo::SEC_NORMAL;

When calling asyncOpen2() on a channel you have to pass any of the 5 security flags:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#34

SEC_NORMAL will not work. It was the default value when attaching a loadInfo to channels, but now that we are adding security checks by calling asyncOpen2(), you have to pass any of those 5 security flags.

@@ +174,1 @@
>                         aContentPolicyType,

What can aContentPolicyType be here? Should it be any of TYPE_INTENRAL_WORKER, right?
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentPolicyBase.idl#200

Or can it be something else?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

If you are going to convert dom/workser/ScriptLoader to use AsyncOpen2() you probably also want to delete security checks form the callsite which are now going to be performed within AsyncOpen2(). Starting [1] you have to change/remove (careful!!!):
* NS_CheckContentLoadPolicy
* CheckMayLoad
* CheckLoadURIWithPrincipal

[1] http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#126

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8673055 [details] [diff] [review]
part 1 - AsyncOpen2 must have 2 params as AsyncOpen has.

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

Don't do this. Having audited most callers of AsyncOpen in our tree, we have so far found exactly 2 such callers (including yours) which made good use of the argument, and lots and lots of code which had to add complexity to pass around a null value or a effectively useless value.

There's many other ways you can solve this here. Either create a small proxy object which acts as the listener and which holds the index as a member. Or even holds the ScriptLoadInfo* directly. That way you could also call OnStreamCompleteInternal directly without the need to do the conversion at [1].

Alternatively you could make the ScriptLoadInfo be a refcounted object and let it act as nsIStreamLoaderObserver.

But don't add this feature to channels since so far its clear that many more callers end up getting more complex, than gets simplified.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#584

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 1 - AsyncOpen2 must have 2 params as AsyncOpen has.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - sec flags in ScriptLoader.

Comment 11

[Andrea Marchesini [:baku]]

I'll update patch 1 following the Jonas' comments.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8678151 [details] [diff] [review]
patch

>+class LoaderProxy final : public nsIStreamLoaderObserver

Maybe more like LoaderListener?  It's not really proxying for anything...

r=me

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please note that I don't think that we should make

new Worker("data:,var x = ...");

inherit the principal of the loading page. The reason for this is the same as why we don't inherit the principal if a chrome document loads a data: URL. I.e. that it might allow an attacker to XSS a page by tricking it into loading a particular URL.

For this reason other browsers don't even support loading data URLs in workers.

Right now the data URL runs with a null principal in the worker. I think we need to continue to do that.

Ideally I would like to either change Gecko to follow other browsers and completely forbid data URLs. Or we could support syntax like:

new Worker({ url: "data:,var x = ...", allowInherit: true });
or
new Worker({ contents: "var x =" });


Obviously adding something like that is not for this bug. But I don't think we should change our security policy in this bug either, which I think the current patch does.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I know you don't think we should do that.  But if we don't, then errors from the worker have to be muted and the page won't get to see them in a useful way.

Is that really the behavior we want?

> Ideally I would like to either change Gecko to follow other browsers and completely
> forbid data URLs.

Why, exactly?

> which I think the current patch does.

It does, because the current security policy means no useful error reporting.  And _that_ is a regression; we used to have sane error reporting here until bug 1160890.

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK, I'm somewhat sympathetic to not changing the security policy, esp if we want to backport this.

Then the next solution is to change the muted boolean to true in the data: case even if the principal is not subsumed.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd still like to land the patch to use AsyncOpen2 though. I don't have an opinion about if we do it here or in another bug since it doesn't fix the stated problem in this bug.

(What we'd have to do is to check if the loaded URL is an inheriting one, and if so use SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL. I think)

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, to clarify, there's no need to backport the AsyncOpen2 patch to aurora or beta.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Baku, have you seen Comment 7? Is there a specific reason we don't want to remove security checks from the callsite? At the moment we would perform security checks twice which is sub optimal. It would be great if we could clean that up with this patch in my opinion! Please don't land before we haven't clarified that.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Christoph, we need to backport this patch to beta, and time is running very short for that.  We're going to take a minimal patch for this bug as discussed in comment 16, then put any followup work, as discussed in comment 7 and comment 16 to separate bugs.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> Christoph, we need to backport this patch to beta, and time is running very
> short for that.  We're going to take a minimal patch for this bug as
> discussed in comment 16, then put any followup work, as discussed in comment
> 7 and comment 16 to separate bugs.

Sure, sounds like you have a plan there. Just wanted to make sure we are not missing things. Thanks for clarification.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: worker.patch

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8678915 [details] [diff] [review]
worker.patch

r=me, but please add a comment explaining why we don't mute main worker scripts: because we've already done same-origin checks on them so we should be able to see their errors.  And call out the data: situation specifically, where we allow it through the same-origin check but then give it a different origin.

Comment 24

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Things that used to work are now broken, silent failures in worker JS without a clear error report in some (rare) cases.

Comment 25

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d085f3b38c99

Comment 26

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

Assuming this will make it into m-c soon -- baku can you request uplift to aurora here? Thanks.

Comment 27

[Andrea Marchesini [:baku]]

Comment on attachment 8678915 [details] [diff] [review]
worker.patch

Approval Request Comment
[Feature/regressing bug #]: Worker and error reporting
[User impact if declined]: errors of workers from data: url are wrongly reported.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: very very low risk: we don'tt enable muted error flag for mainWorkerScript.
[String/UUID change made/needed]: none

Comment 28

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

Comment on attachment 8678915 [details] [diff] [review]
worker.patch

OK to uplift to esr38, once it looks good on m-c.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d085f3b38c99

Comment 30

[Carsten Book [:Tomcat]]

backed this out from aurora for bustage like  https://treeherder.mozilla.org/logviewer.html#?job_id=1390896&repo=mozilla-aurora from this or the other patch

Comment 31

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

Comment on attachment 8678915 [details] [diff] [review]
worker.patch

It sounds like we don't need to land this on aurora after all, as it's taken care of in bug 1211970 (comment 23).

Comment 32

[Boris Zbarsky [:bzbarsky]]

I don't believe it actually is.  What evidence is there for it being taken care of, exactly?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8678915 [details] [diff] [review]
worker.patch

Putting flag back, because comment 31 is confused.

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Relanded: https://hg.mozilla.org/releases/mozilla-aurora/rev/e2c0b15aed6b

Comment 35

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

Thanks. bz I can't match your expertise, but what I can do is keep asking questions of the people like you who have it, until we figure out what's correct. All the flag-twiddling is annoying, but it does help us keep a complicated process tracked between many people.  Usually it works pretty well. On reading through all the comments and bugs mentioned, I summarized what I thought was true, and asked for feedback onirc directly from baku, who said it was the same issue or dependent on the patch that already landed. Release managers depend on the developers and reviewers to give us accurate information, since we tend to be generalists. 

Anyway, let's see if this looks good in aurora tomorrow.  I hope we have all the pieces in order now.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Yeah, I think we should be good now.  I know you only have the data we give you to work with...

Comment 37

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

Belatedly tracking in case this reopens.

Comment 38

[Sylvestre Ledru [:Sylvestre]]

Not tracking as we already shipped and don't think it is worth doing a dot release.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Isn't this fixed on 42 by bug 1160890 being backed out?