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

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42- wontfix, firefox43+ fixed, firefox44+ fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

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

This URL doesn't report any error into the console.
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 3

4 years ago
Attachment #8673058 - Flags: feedback?(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.
Attachment #8673058 - Flags: feedback?(bzbarsky) → feedback+
> 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 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?
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 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
Attachment #8673055 - Flags: review-
(Assignee)

Comment 9

4 years ago
Attachment #8673055 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Attachment #8673058 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
I'll update patch 1 following the Jonas' comments.
(Assignee)

Comment 12

4 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8678070 - Attachment is obsolete: true
Attachment #8678071 - Attachment is obsolete: true
Attachment #8678151 - Flags: review?(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
Attachment #8678151 - Flags: review?(bzbarsky) → review+
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.
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.
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.
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)
Also, to clarify, there's no need to backport the AsyncOpen2 patch to aurora or beta.
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.
Flags: needinfo?(amarchesini)
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.
(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.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 22

4 years ago
Posted patch worker.patchSplinter Review
Attachment #8678151 - Attachment is obsolete: true
Attachment #8678915 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Blocks: 1218433
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.
Attachment #8678915 - Flags: review?(bzbarsky) → review+
[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.
Assuming this will make it into m-c soon -- baku can you request uplift to aurora here? Thanks.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 27

4 years ago
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
Flags: needinfo?(amarchesini)
Attachment #8678915 - Flags: approval-mozilla-aurora?
Comment on attachment 8678915 [details] [diff] [review]
worker.patch

OK to uplift to esr38, once it looks good on m-c.
Attachment #8678915 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 29

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d085f3b38c99
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
Flags: needinfo?(amarchesini)
(Assignee)

Updated

4 years ago
Flags: needinfo?(amarchesini)
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).
Attachment #8678915 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
I don't believe it actually is.  What evidence is there for it being taken care of, exactly?
Flags: needinfo?(lhenry)
Comment on attachment 8678915 [details] [diff] [review]
worker.patch

Putting flag back, because comment 31 is confused.
Attachment #8678915 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
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.
Flags: needinfo?(lhenry)
Yeah, I think we should be good now.  I know you only have the data we give you to work with...
Belatedly tracking in case this reopens.
Not tracking as we already shipped and don't think it is worth doing a dot release.
Isn't this fixed on 42 by bug 1160890 being backed out?
You need to log in before you can comment on or make changes to this bug.