Ian Melven :imelven Assignee
	Description • 12 years ago

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.

	Ian Melven :imelven Assignee
	Comment 1 • 12 years ago

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.

	Ian Melven :imelven Assignee
	Comment 2 • 12 years ago

(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

	Ian Melven :imelven Assignee
	Comment 3 • 12 years ago

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.

	Ian Melven :imelven Assignee
	Comment 4 • 12 years ago

An initial attempt.

	Ian Melven :imelven Assignee
	Comment 5 • 12 years ago

Initial attempt.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 12 years ago

Doesn't this patch allow a page to bypass worker limits by just creating lots of sandboxed iframes and running the workers from those?

	Ian Melven :imelven Assignee
	Comment 7 • 12 years ago

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

	Ian Melven :imelven Assignee
	Comment 8 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 12 years ago

Ian, have you managed to get hold of Ben?

	Ian Melven :imelven Assignee
	Comment 10 • 12 years ago

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

	Ian Melven :imelven Assignee
	Comment 11 • 12 years ago

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.

	Ian Melven :imelven Assignee
	Comment 12 • 12 years ago

This keeps GetBaseDomain() and works as described in comment 11

	Daira Hopwood
	Comment 13 • 12 years ago

(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?

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 12 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 12 years ago

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.

	Ian Melven :imelven Assignee
	Comment 16 • 12 years ago

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

	Ian Melven :imelven Assignee
	Comment 17 • 12 years ago

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

	Ian Melven :imelven Assignee
	Comment 18 • 12 years ago

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.

	Daira Hopwood
	Comment 19 • 12 years ago

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

	Ian Melven :imelven Assignee
	Comment 20 • 12 years ago

(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

	Ian Melven :imelven Assignee
	Comment 21 • 12 years ago

Unbitrot, remove the [noscript] as bz suggested, carrying over r+

	Ian Melven :imelven Assignee
	Comment 22 • 12 years ago

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.

	Daira Hopwood
	Comment 23 • 12 years ago

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

	Ian Melven :imelven Assignee
	Comment 24 • 12 years ago

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

	Ian Melven :imelven Assignee
	Comment 25 • 12 years ago

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);

	Ian Melven :imelven Assignee
	Comment 26 • 12 years ago

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 ?

	Ian Melven :imelven Assignee
	Comment 27 • 12 years ago

Unbitrot, carry over r+

	Ian Melven :imelven Assignee
	Comment 28 • 12 years ago

Unbitot, carrying over the r+.

	Ian Melven :imelven Assignee
	Comment 29 • 12 years ago

Pushed to try : https://tbpl.mozilla.org/?tree=Try&rev=fd3e12b72533

	Ian Melven :imelven Assignee
	Comment 30 • 12 years ago

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.

	Ian Melven :imelven Assignee
	Comment 31 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3225d581951e
https://hg.mozilla.org/integration/mozilla-inbound/rev/752a9a829df2

see comment 29 for try results

	Ed Morley [:emorley]
	Comment 32 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/3225d581951e
https://hg.mozilla.org/mozilla-central/rev/752a9a829df2