Closed Bug 752529 Opened 8 years ago Closed 7 years ago

workers shouldn't use string origins as null principals don't have them

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: imelven, Assigned: imelven)

References

Details

Attachments

(2 files, 5 obsolete files)

As part of of iframe sandbox (see bug 341604 comment 136), i needed to work around workers needing a string origin, as a null principal didn't return one when asked via the APIs that are used to get one from a real principal 

the workaround looks like this : 

diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -2534,16 +2536,24 @@ WorkerPrivate::Create(JSContext* aCx, JS
           JS_ReportError(aCx, "Could not determine if codebase is file!");
           return nsnull;
         }
 
         if (isFile) {
           // XXX Fix this, need a real domain here.
           domain = file;
         }
+        // Workaround for workers needing a string domain - will be fixed
+        // in a followup after this lands.
+        else if (document->SandboxFlags() & SANDBOXED_ORIGIN) {
+          if (NS_FAILED(codebase->GetAsciiSpec(domain))) {
+            JS_ReportError(aCx, "Could not get URI's spec for sandboxed document!");
+            return nsnull;
+          }
+        }

from looking through the workers code, it looks like this domain is used to enforce the limit on workers per domain and also for some memory reporting. 

in discussion with bz, he said that we likely don't use nsIPrincipal's GetOrigin here to stop people using multiple ports or subdomains to work around the limit - i need to check the workers spec here to see what's intended

bz also said "the solution would be to either expose something
on nsIPrincipal that returns what we want here or to fix the worker code
to correctly handle non-hierarchical schemes."

i agreed i would clean this up via a followup, this is that followup bug.
Assignee: nobody → imelven
It looks like WorkerPrivate.cpp contains the only call to mozIThirdPartyUtil's GetBaseDomain method - so one way forward here is to change that to pass back a usable domain for documents with null principals (probably just a string like moz-nullprincipal:<uuid>) which can then be used for memory reporting and to enforce the quota - the quota will be per each individual, specific null principal.
(In reply to Ian Melven :imelven from comment #1)
> It looks like WorkerPrivate.cpp contains the only call to
> mozIThirdPartyUtil's GetBaseDomain method - so one way forward here is to
> change that to pass back a usable domain for documents with null principals
> (probably just a string like moz-nullprincipal:<uuid>) which can then be
> used for memory reporting and to enforce the quota - the quota will be per
> each individual, specific null principal.

I misread mxr or it lied - I just found ThirdPartyUtil::IsThirdPartyWindow also calls GetBaseDomain
bz suggested two options - adding something to nsIPrincipal to give the base
domain that would be implemented to work in nsNullPrincipal OR fixing the worker code

i messed around with the worker code a bit, it seems like anything there would boil
down to, if this is a null principal, use the path as the base domain (the path
will be the unique UUID for the null principal)

it's starting to seem cleaner to me to do something on nsIPrincipal - i'm thinking about 
adding a GetBaseDomain there and having it return the path for a null principal
and calling out to thirdPartyUtil for everything else. i suppose for file
we could also have it return the URI path (i'm assuming that that will be the file
path itself) rather than using the string 'file' as the domain.

i'll code this up and attach a patch.
Attached patch nsPrincipal idl v1 (obsolete) — Splinter Review
An initial attempt.
Attachment #653007 - Flags: review?(bzbarsky)
Attached patch implementation v1 (obsolete) — Splinter Review
Initial attempt.
Attachment #653008 - Flags: review?(bzbarsky)
Doesn't this patch allow a page to bypass worker limits by just creating lots of sandboxed iframes and running the workers from those?
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #6)
> Doesn't this patch allow a page to bypass worker limits by just creating
> lots of sandboxed iframes and running the workers from those?

Good question - in short, yes - i need to read the workers spec more closely and find out how the limits are described and what they're trying to prevent, I suppose. I believe that other implementations don't allow workers in sandboxed iframes (but abarth has said this wasn't an intended restriction and there's a bug on changing this for chromium IIRC), so we're off in uncharted waters here. 

If a page created a lot of workers in separate sandboxed iframes, each of those workers should only be able to load themselves with scripts from their own sandboxed frame (with their same null principal, or from data: or blob URI's) for whatever that's worth. I guess not much, since they can still postMessage back to the parent page, assuming they can get a reference to it.
(In reply to Ian Melven :imelven from comment #7)
> (In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from
> comment #6)
> > Doesn't this patch allow a page to bypass worker limits by just creating
> > lots of sandboxed iframes and running the workers from those?
> 
> Good question - in short, yes - i need to read the workers spec more closely
> and find out how the limits are described and what they're trying to
> prevent, I suppose. I believe that other implementations don't allow workers
> in sandboxed iframes (but abarth has said this wasn't an intended
> restriction and there's a bug on changing this for chromium IIRC), so we're
> off in uncharted waters here. 

I actually didn't see anything in the spec about these limits so maybe they're something we came up with for Gecko - i'll try to ask Ben about them.
Ian, have you managed to get hold of Ben?
(In reply to Boris Zbarsky (:bz) from comment #9)
> Ian, have you managed to get hold of Ben?

I've been working on other things (CSP 1.0) so I haven't been following up, I'll email him and Jonas again.
Discussed with Jonas over email - he suggested using the parent of the sandboxed document as the domain, I suggested going upwards until a non-sandboxed frame is found so nesting isn't an issue. If a non sandboxed parent can't be found (I'm thinking mostly of CSP sandbox here), I think we should then use the UUID string as a fallback.
Attached patch implementation v2 (obsolete) — Splinter Review
This keeps GetBaseDomain() and works as described in comment 11
Attachment #653008 - Attachment is obsolete: true
Attachment #653008 - Flags: review?(bzbarsky)
Attachment #663616 - Flags: review?(bzbarsky)
(In reply to Ian Melven :imelven from comment #0)
> in discussion with bz, he said that we likely don't use nsIPrincipal's
> GetOrigin here to stop people using multiple ports or subdomains to work
> around the limit - i need to check the workers spec here to see what's
> intended

If that's correct, wouldn't using the UUID in the case of CSP sandbox bypass that intention?
Comment on attachment 663616 [details] [diff] [review]
implementation v2

NodePrincipal() never returns null.  So you can get rid of that null-check.

r=me with that.  Thanks!
Attachment #663616 - Flags: review?(bzbarsky) → review+
Attachment #653007 - Attachment is obsolete: true
Attachment #653007 - Flags: review?(bzbarsky)
Comment on attachment 653007 [details] [diff] [review]
nsPrincipal idl v1

Er, no, this is still relevant.

Why noscript?  Might as well leave it scriptable.  r=me with that.
Attachment #653007 - Attachment is obsolete: false
Attachment #653007 - Flags: review+
(In reply to David-Sarah Hopwood from comment #13)
> (In reply to Ian Melven :imelven from comment #0)
> > in discussion with bz, he said that we likely don't use nsIPrincipal's
> > GetOrigin here to stop people using multiple ports or subdomains to work
> > around the limit - i need to check the workers spec here to see what's
> > intended
> 
> If that's correct, wouldn't using the UUID in the case of CSP sandbox bypass
> that intention?

yes, but I'm not sure what else we can do here.. if the top level document is sandboxed via CSP sandbox with a unique origin, we don't have much else to go on. I suppose you could use the document's 'natural' origin, but its null principal won't contain that. Sites that want to create lots of workers could use a wildcard DNS and a lot of subdomains to get the same effect as well...
(In reply to Boris Zbarsky (:bz) from comment #14)
> Comment on attachment 663616 [details] [diff] [review]
> implementation v2
> 
> NodePrincipal() never returns null.  So you can get rid of that null-check.
> 
> r=me with that.  Thanks!

Thanks Boris, it's nice to finally get this followup done as I had promised :) 

I'll clean up the nits and land it shortly.
I was on the fence, but after a brief discussion with Sid, i'm now going to write a mochitest for this before landing it.
(In reply to Ian Melven :imelven from comment #16)
> (In reply to David-Sarah Hopwood from comment #13)
> > (In reply to Ian Melven :imelven from comment #0)
> > > in discussion with bz, he said that we likely don't use nsIPrincipal's
> > > GetOrigin here to stop people using multiple ports or subdomains to work
> > > around the limit - i need to check the workers spec here to see what's
> > > intended
> > 
> > If that's correct, wouldn't using the UUID in the case of CSP sandbox bypass
> > that intention?
> 
> yes, but I'm not sure what else we can do here.. if the top level document
> is sandboxed via CSP sandbox with a unique origin, we don't have much else
> to go on. I suppose you could use the document's 'natural' origin, but its
> null principal won't contain that. Sites that want to create lots of workers
> could use a wildcard DNS and a lot of subdomains to get the same effect as
> well...

It sounds like the restriction can't be enforced. In which case, it should probably be removed -- unless it is there only to mitigate *unintentional* denial of service.
(In reply to David-Sarah Hopwood from comment #19)
> (In reply to Ian Melven :imelven from comment #16)
> > (In reply to David-Sarah Hopwood from comment #13)
> > > (In reply to Ian Melven :imelven from comment #0)
> > > > in discussion with bz, he said that we likely don't use nsIPrincipal's
> > > > GetOrigin here to stop people using multiple ports or subdomains to work
> > > > around the limit - i need to check the workers spec here to see what's
> > > > intended
> > > 
> > > If that's correct, wouldn't using the UUID in the case of CSP sandbox bypass
> > > that intention?
> > 
> > yes, but I'm not sure what else we can do here.. if the top level document
> > is sandboxed via CSP sandbox with a unique origin, we don't have much else
> > to go on. I suppose you could use the document's 'natural' origin, but its
> > null principal won't contain that. Sites that want to create lots of workers
> > could use a wildcard DNS and a lot of subdomains to get the same effect as
> > well...
> 
> It sounds like the restriction can't be enforced. In which case, it should
> probably be removed -- unless it is there only to mitigate *unintentional*
> denial of service.

Last night I had this idea : if there's no non-sandboxed parent, use the top most sandboxed document's GUID. This means that a top level CSP sandboxed document (keep in mind we don't have CSP sandbox yet though) would have any sandboxed workers charged to its limit and couldn't create an arbitrary amount of workers.

Also I realized I was wrong about the wildcard/subdomain approach - the limit code has always (and still does in this patch) use the base domain to stop this workaround, as I mentioned in comment 0
Attached patch nsPrincipal idl v2 (obsolete) — Splinter Review
Unbitrot, remove the [noscript] as bz suggested, carrying over r+
Attachment #653007 - Attachment is obsolete: true
Attachment #672395 - Flags: review+
Attached patch implementation v3 (obsolete) — Splinter Review
Remove the null check for NodePrincipal(), unbitrot, carrying over the r+

I need to work on CSP for a bit but will return to this to write a test when I can.
Attachment #663616 - Attachment is obsolete: true
Attachment #672396 - Flags: review+
(In reply to Ian Melven :imelven from comment #20)
> Last night I had this idea : if there's no non-sandboxed parent, use the top
> most sandboxed document's GUID. This means that a top level CSP sandboxed
> document (keep in mind we don't have CSP sandbox yet though) would have any
> sandboxed workers charged to its limit and couldn't create an arbitrary
> amount of workers.

Does "top level document" just correspond to a top-level window/tab (so that window.open for example creates a top-level document)? If so then that approach won't help.

If new windows have their creator as a parent, it would help. (The limit would be per time the user visited the CSP sandboxed site in an independent tab, which is fine.)
(In reply to David-Sarah Hopwood from comment #23)
> (In reply to Ian Melven :imelven from comment #20)
> > Last night I had this idea : if there's no non-sandboxed parent, use the top
> > most sandboxed document's GUID. This means that a top level CSP sandboxed
> > document (keep in mind we don't have CSP sandbox yet though) would have any
> > sandboxed workers charged to its limit and couldn't create an arbitrary
> > amount of workers.
> 
> Does "top level document" just correspond to a top-level window/tab (so that
> window.open for example creates a top-level document)? If so then that
> approach won't help.

yes, window.open will create a new top level document - but window.open is blocked in sandboxed documents (unless they have allow-popups, which isn't implemented in Gecko yet, that's bug 766282)

> If new windows have their creator as a parent, it would help. (The limit
> would be per time the user visited the CSP sandboxed site in an independent
> tab, which is fine.)

I can see your point about the limit not necessarily being the most useful - it's not part of the spec, so it's something we chose to implement in Gecko. My personal opinion is that it's ok to just raise the bar a bit here, since there's many other means of sites DoS'ing the browser - the string domain in workers is also used for memory reporting, so I think I should try to land this and then we can revisit the question of whether the limit makes sense (or if the spec should cover these DoS type issues for workers) in another bug perhaps.
Looks like the default for the limit is 20 at the moment : 

/modules/libpref/src/init/all.js line 84 -- pref("dom.workers.maxPerDomain", 20);
I finally got back to looking at making a test for this. It looks like the max workers per domain isn't a hard limit - when this number of active workers is reached for a particular domain, workers are queued after that and then run as active workers finish (assuming the workers that contributed to hitting the limit do finish). 

From content, I don't see any sort of error or result when creating workers above this limit. 

Boris, Ben, any thoughts ? If the domain is just used for this soft limit on active workers and memory reporting, I'm not sure if I can write a test for this after all ?
Status: NEW → ASSIGNED
Unbitrot, carry over r+
Attachment #672395 - Attachment is obsolete: true
Attachment #697271 - Flags: review+
Unbitot, carrying over the r+.
Attachment #672396 - Attachment is obsolete: true
Attachment #697273 - Flags: review+
Spoke to bz - there doesn't seem to be a good way to write a test for this, so I'm going to land it without one.
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/3225d581951e
https://hg.mozilla.org/mozilla-central/rev/752a9a829df2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.