Closed Bug 1125961 Opened 9 years ago Closed 7 years ago

ServiceWorker must set provide nsITabChild in its LoadGroup callbacks

Categories

(Core :: DOM: Service Workers, defect)

Product:
Core
Component:
DOM: Service Workers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: bkelly, Assigned: jdm)

References

Details

Attachments

(1 file, 11 obsolete files)

Allow sending null PBrowser actors when there's a triggering principal which can be used for security checks
33.86 KB, patch
Details | Diff | Splinter Review 
While investigating bug 1118845, I realized that the workers need more than just nsILoadContext in their LoadGroups.  In particular, we need to set the nsITabChild as a group callback in order to pass child security checks on b2g.  Without this, I believe we will get this error:

   WARNING: child tried to open http IPDL channel w/o security info

I'm fixing this issue for SharedWorkers by proxying the interface request to the original load group.  For ServiceWorkers, however, we don't have an original load group.  We need to somehow get a reference to the nsITabChild object.
Asking in #e10s, Dave suggested that it might not make sense for a ServiceWorker to have a TabChild.  Apparently thats really associated with documents.

Instead, we might need to alter the security checks in some way for ServiceWorkers.
The nsITabChild is important to associate the request with a particular app/browser combination for b2g. This is necessary both for app permissions as well as proper network usage statistics.
Would it make sense for a ServiceWorker to construct its own TabChild()?  Or its own implementation of nsITabChild?  Of course, it would not surprise me if we have code that always casts nsITabChild to TabChild.
No, constructing a TabChild would not be the correct solution. The network code does cast nsITabChild to TbaChild as you suspect (http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1512), so we need to make sure that we're providing callbacks on the channel or loadgroup that can resolve to the linked document's nsITabChild.
But... a document doesn't always exist for a ServiceWorker.  For example, in the case of a push notification there may not be any window at all.

For the short term maybe we can require whatever code that starts up a ServiceWorker to require an nsITabChild from whatever context triggered the fetch event.  After bug 1118845 the load group for SharedWorkers and dedicated Workers should have an nsITabChild available even if they don't know about their document any more.  Is that possible?

For push notifications we would then have to create something new to represent the case when there is no existing TabChild.  (I guess an extra question for push notifications is which child process are they created in???)
Even further, maybe this is starting to touch on the multi-child process issue for ServiceWorkers.  Long term, should ServiceWorkers be running in their own child process for the case where there are two tab child processes producing fetch events?

Right now on b2g I think we will end up with two ServiceWorker threads in different child processes for two different apps from the same origin.
I think forcing any code that could start up a ServiceWorker to provide the relevant context to associate it with a TabChild is the way to go (in child processes only, of course). We can make the loadgroup notificationCallbacks resolve any GetInterface requests to nsITabChild to the proper one and that should satisfy the security checks.
I guess the trick is defining "the proper one" for all cases where we can start up a ServiceWorker.
So can we change the two ServiceWorkerManager::CreateServiceWorker() methods to require passing in a load group?

It seems the patch in bug 1065216 could get this load group from the document in ServiceWorkerManager::DispatchFetchEvent().  And ServiceWorkerManager::CreateServiceWorkerForWindow() should also have a load group from the window's document.

Is there a way to get at the document or load group from the ServiceWorkerRegisterJob?  Its an nsIStreamListener, so can we get it from the channel its listening to?

If we are able to pass in the load group to ServiceWorkerManager::CreateServiceWorker(), then we should be able to do something like the following after bug 1118845:

  LoadInfo loadInfo;
  loadInfo.mLoadGroup = aPassedInLoadGroup;
  WorkerPrivate::OverrideLoadInfoLoadGroup(loadInfo);

The OverrideLoadInfoLoadGroup() call is a new method I'm adding that will create a proxy that properly handles the nsITabChild.  It will also ensure the load group is canceled when the worker exits.

Josh, Nikhil, what do you think?
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(josh)
Oh, and the if we end up getting an existing WorkerPrivate because the ServiceWorker is already running, then we want this called:

  workerPrivate->UpdateOverridenLoadGroup(aPassedInLoadGroup);

This will append the nsITabChild for the document to the list of possible tab child in the load group.
(In reply to Ben Kelly [:bkelly] from comment #9)
> So can we change the two ServiceWorkerManager::CreateServiceWorker() methods
> to require passing in a load group?
> 
> It seems the patch in bug 1065216 could get this load group from the
> document in ServiceWorkerManager::DispatchFetchEvent().  And
> ServiceWorkerManager::CreateServiceWorkerForWindow() should also have a load
> group from the window's document.
> 
> Is there a way to get at the document or load group from the
> ServiceWorkerRegisterJob?  Its an nsIStreamListener, so can we get it from
> the channel its listening to?
> 
> If we are able to pass in the load group to
> ServiceWorkerManager::CreateServiceWorker(), then we should be able to do
> something like the following after bug 1118845:
> 
>   LoadInfo loadInfo;
>   loadInfo.mLoadGroup = aPassedInLoadGroup;
>   WorkerPrivate::OverrideLoadInfoLoadGroup(loadInfo);
> 
> The OverrideLoadInfoLoadGroup() call is a new method I'm adding that will
> create a proxy that properly handles the nsITabChild.  It will also ensure
> the load group is canceled when the worker exits.
> 
> Josh, Nikhil, what do you think?

It is hard to get a loadgroup for the "install" and "activate" spinups. On the first run we could get them from the document, but on soft (background) updates and delayed activations (where we don't dispatch the activate until everything is uncontrolled), we don't have obvious candidates. Any suggestions?
Flags: needinfo?(nsm.nikhil)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #11)
> It is hard to get a loadgroup for the "install" and "activate" spinups. On
> the first run we could get them from the document, but on soft (background)
> updates and delayed activations (where we don't dispatch the activate until
> everything is uncontrolled), we don't have obvious candidates. Any
> suggestions?

Hmm.  I guess an added implication here is that soft (background) updates probably don't work on b2g.  These network requests will need an nsITabChild (or something new and equivalent) in order to pass child security checks.

Nikhil, how do you know which process to run the background updates in?  This isn't done in the parent process is it?
Flags: needinfo?(nsm.nikhil)
Talking in the meeting today, maybe we could lazily perform background updates and activation when we next have an open window.
Jonas, do you have any input on how best to handle nsITabChild for ServiceWorkers?  We might be able to work around this for fetch event, but it seems things like push notification may not have a window in any reasonable time frame.
Flags: needinfo?(jonas)
(In reply to Ben Kelly [:bkelly] from comment #12)
> (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #11)
> > It is hard to get a loadgroup for the "install" and "activate" spinups. On
> > the first run we could get them from the document, but on soft (background)
> > updates and delayed activations (where we don't dispatch the activate until
> > everything is uncontrolled), we don't have obvious candidates. Any
> > suggestions?
> 
> Hmm.  I guess an added implication here is that soft (background) updates
> probably don't work on b2g.  These network requests will need an nsITabChild
> (or something new and equivalent) in order to pass child security checks.
> 
> Nikhil, how do you know which process to run the background updates in? 
> This isn't done in the parent process is it?

They are not run in the parent. It would be good if we could spawn a process for SWs.
Related to this, we need this checking to work as well:

  https://dxr.mozilla.org/mozilla-central/source/dom/ip/AppProcessChecker.cpp?from=AppProcessChecker.cpp#209

Which means we need a TabContext as well for cases where ServiceWorker is running without an associated window.
(In reply to Ben Kelly [:bkelly] from comment #14)
> Jonas, do you have any input on how best to handle nsITabChild for
> ServiceWorkers?  We might be able to work around this for fetch event, but
> it seems things like push notification may not have a window in any
> reasonable time frame.

No, I don't know enough about nsITabChild. This sounds like a question for bent.

How do we handle Shared Workers?
Flags: needinfo?(jonas) → needinfo?(bent.mozilla)
SharedWorkers are easier because there iis always at least one document.  I recently fixed this (sorry, dontbhave bug # handy) to use the first nsITabChild found for the list of docs using the SharedWorker.  This iis done with a custom nsIInterfaceRequestor iin WorkerPrivate.

For ServiceWorkers, though, we may not have a document at all.  We may not even know what child process to run; for example, a push event on b2g.
Makes sense. Sadly I don't know enough about nsITabChild to answer, so I'll still have to defer to bent.


Regarding which child process to use to handle a push event, that question is simpler, at least in B2G. You'd have to first find a process which is currently running the given appid/browserflag (you can look at the LoadContext of the FrameLoader of each nsITabChild). If one is found, you can use that nsITabChild. If not, we need to start a new child process.
Josh has kindly volunteered to take this one.

His proposal is to add a new callbacks interface providing a unique ServiceWorker ID.  This ID will then get passed to the parent instead of the nsITabChild actor.  The parent side will then use this ID to find the registration and validate the request.  Auth prompting will be disabled for ServiceWorkers.
Assignee: nobody → josh
Status: NEW → ASSIGNED
Flags: needinfo?(josh)
Depends on: 1137683
These are all still work-in-progress patches that pass tests but don't allow any kind of verification.
My plan right now is to use a new IPDL actor type that is managed by both PBrowser and PContent - the former will allow us to obtain a PBrowser from PServiceWorker easily for the necko security checks, and the former should only be created by the parent process for things like the 24 hour update timer.
(In reply to Josh Matthews [:jdm] from comment #26)
> My plan right now is to use a new IPDL actor type that is managed by both
> PBrowser and PContent - the former will allow us to obtain a PBrowser from
> PServiceWorker easily for the necko security checks, and the former should
> only be created by the parent process for things like the 24 hour update
> timer.

Note that for security checks you'll want to use PBrowserOrId and always do the check via PContent since nested process has no authority to check security.

See http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp?rev=468eb0cd72c5#118 for example
I don't know much about nsITabChild, except that it's the common stuff for both child processes and nested processes.
Flags: needinfo?(bent.mozilla)
Attachment #8575509 - Attachment is obsolete: true
Attachment #8575510 - Attachment is obsolete: true
Attachment #8575511 - Attachment is obsolete: true
Attachment #8575512 - Attachment is obsolete: true
Here's my latest work which passes tests when the network.disable.ipc.security pref is flipped and the patch from bug 1137683 is applied. The third patch should probably be expanded to support non-HTTP channels, too.
Attachment #8580320 - Flags: review?(jduell.mcbugs)
Attachment #8580323 - Flags: review?(bkelly)
Attachment #8580324 - Flags: review?(jduell.mcbugs)
Comment on attachment 8580323 [details] [diff] [review]
Part 2: Associate service worker loadcontext with registered principal

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

r=me with comments addressed.  Thanks!

::: dom/interfaces/base/nsIServiceWorker.idl
@@ +7,5 @@
> +
> +interface nsIPrincipal;
> +
> +[builtinclass, uuid(84cb69ce-19b3-4b5c-90d4-a95cd0c1f127)]
> +interface nsIServiceWorker : nsISupports

Add a comment about what this interface is for and how its expected to be used?

::: dom/workers/ServiceWorkerManager.cpp
@@ +2425,5 @@
>  
> +  nsRefPtr<ServiceWorkerRegistrationInfo> registration =
> +    GetServiceWorkerRegistrationInfo(info.mBaseURI);
> +  MOZ_ASSERT(registration);
> +  info.mServiceWorker = new ServiceWorkerPrincipalProvider(registration);

Do we not do this for the other CreateServiceWorker case because a tab child exists for the window?  In theory we might still want the nsIServiceWorker as a fallback if the ServiceWorker outlives the window.

::: dom/workers/ServiceWorkerRegistrar.cpp
@@ +672,5 @@
> +
> +ServiceWorkerPrincipalProvider::ServiceWorkerPrincipalProvider(
> +    workers::ServiceWorkerRegistrationInfo* aInfo)
> +: mInfo(aInfo)
> +{

MOZ_ASSERT(mInfo);

@@ +678,5 @@
> +
> +NS_IMETHODIMP
> +ServiceWorkerPrincipalProvider::GetPrincipal(nsIPrincipal** aPrincipal)
> +{
> +  *aPrincipal = mInfo->mPrincipal;

I think you need to AddRef().  The other patch uses getter_AddRefs() when calling GetPrincipal().  So something like:

  MOZ_ASSERT(NS_IsMainThread());
  nsRef<nsIPrincipal> ref = mInfo->mPrincipal;
  ref.forget(aPrincipal);

::: dom/workers/WorkerPrivate.cpp
@@ +2141,5 @@
>  void
>  WorkerLoadInfo::
> +InterfaceRequestor::AddServiceWorker(nsIServiceWorker* aServiceWorker)
> +{
> +  mServiceWorker = aServiceWorker;

MOZ_ASSERT(!mServiceWorker);
MOZ_ASSERT(aServiceWorker);

I assume we don't want to ever set this twice.

@@ +4640,5 @@
>                                             aLoadInfo.mLoadGroup);
>    aLoadInfo.mInterfaceRequestor->MaybeAddTabChild(aLoadInfo.mLoadGroup);
>  
> +  if (aLoadInfo.mServiceWorker) {
> +    aLoadInfo.mInterfaceRequestor->AddServiceWorker(aLoadInfo.mServiceWorker);

Can you add a MOZ_ASSERT(NS_IsMainThread()) to OverrideLoadInfoLoadGroup()?

GetLoadInfo() can run off the main thread in some cases, but it should be main thread when it calls this.

::: dom/workers/Workers.h
@@ +211,5 @@
>    nsCOMPtr<nsPIDOMWindow> mWindow;
>    nsCOMPtr<nsIContentSecurityPolicy> mCSP;
>    nsCOMPtr<nsIChannel> mChannel;
>    nsCOMPtr<nsILoadGroup> mLoadGroup;
> +  nsCOMPtr<nsIServiceWorker> mServiceWorker;

You need to handle mServiceWorker in StealFrom().

@@ +220,5 @@
>  
>    public:
>      InterfaceRequestor(nsIPrincipal* aPrincipal, nsILoadGroup* aLoadGroup);
>      void MaybeAddTabChild(nsILoadGroup* aLoadGroup);
> +    void AddServiceWorker(nsIServiceWorker* aServiceWorker);

nit: Since we don't maintain a list, I think SetServiceWorker() would be a bit better.
Attachment #8580323 - Flags: review?(bkelly) → review+
For reference, network.disable.ipc.security needs to be set to true on FirefoxOS until this bug is fixed.  Repeating here so that its easy to find for people trying to test b2g on mozilla-central.
Attachment #8580320 - Flags: review?(jduell.mcbugs) → review?(dd.mozilla)
Attachment #8580324 - Flags: review?(jduell.mcbugs) → review?(dd.mozilla)
Comment on attachment 8580320 [details] [diff] [review]
Part 1: Allow sending principals over HTTP channels, and check that they match the data in the serialized load context

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

The principal provided in PBrowserOrPrincipalOrId is the same as loadingPrincipal provided in loadInfo (this came from a chat with jdm).

Since there is idea of moving from load context to loadinfo bug 1125916. We could use loadingPrincipal to do necessary checks if PBrowserOrId is not present.
Attachment #8580320 - Flags: review?(dd.mozilla)
Attachment #8580324 - Flags: review?(dd.mozilla)
Attachment #8580320 - Attachment is obsolete: true
Attachment #8580323 - Attachment is obsolete: true
Attachment #8580324 - Attachment is obsolete: true
Comment on attachment 8592260 [details] [diff] [review]
Allow sending null PBrowser actors when there's a triggering principal which can be used for security checks

Dragana, is this the design you had in mind?
Attachment #8592260 - Flags: feedback?(dd.mozilla)
Comment on attachment 8592260 [details] [diff] [review]
Allow sending null PBrowser actors when there's a triggering principal which can be used for security checks

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

that is what i had in mind.

::: netwerk/ipc/NeckoParent.cpp
@@ +371,5 @@
>  NeckoParent::AllocPWebSocketParent(const PBrowserOrId& browser,
>                                     const SerializedLoadContext& serialized)
>  {
>    nsCOMPtr<nsILoadContext> loadContext;
> +  const char *error = CreateChannelLoadContext(browser, nullptr, Manager(),

is this always having browser set?

::: netwerk/protocol/websocket/WebSocketChannelChild.cpp
@@ +491,5 @@
>                                  getter_AddRefs(iTabChild));
>    if (iTabChild) {
>      tabChild = static_cast<mozilla::dom::TabChild*>(iTabChild.get());
>    }
> +  if (MissingRequiredTabChild(tabChild, mLoadInfo, "websocket")) {

should it be nullptr instead of mLoadInfo because loadinfo is not serialized and in NeckoParent.cpp you set it to nullptr.

::: netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp
@@ +656,5 @@
>    URIParams originalURI;
>    SerializeURI(mOriginalURI, originalURI);
>  
>    mozilla::dom::TabChild* tabChild = GetTabChild(this);
> +  if (MissingRequiredTabChild(tabChild, mLoadInfo, "wyciwyg")) {

the same here as with the websocket.
Attachment #8592260 - Flags: feedback?(dd.mozilla) → feedback+
(In reply to Dragana Damjanovic [:dragana] from comment #39)
> ::: netwerk/ipc/NeckoParent.cpp
> @@ +371,5 @@
> >  NeckoParent::AllocPWebSocketParent(const PBrowserOrId& browser,
> >                                     const SerializedLoadContext& serialized)
> >  {
> >    nsCOMPtr<nsILoadContext> loadContext;
> > +  const char *error = CreateChannelLoadContext(browser, nullptr, Manager(),
> 
> is this always having browser set?

No, but I fixed this by explicitly sending the requesting principal in the IPDL constructor and passing that instead of nullptr.

> ::: netwerk/protocol/websocket/WebSocketChannelChild.cpp
> @@ +491,5 @@
> >                                  getter_AddRefs(iTabChild));
> >    if (iTabChild) {
> >      tabChild = static_cast<mozilla::dom::TabChild*>(iTabChild.get());
> >    }
> > +  if (MissingRequiredTabChild(tabChild, mLoadInfo, "websocket")) {
> 
> should it be nullptr instead of mLoadInfo because loadinfo is not serialized
> and in NeckoParent.cpp you set it to nullptr.

mLoadInfo is actually serialized just below this block, so this is fine.

> ::: netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp
> @@ +656,5 @@
> >    URIParams originalURI;
> >    SerializeURI(mOriginalURI, originalURI);
> >  
> >    mozilla::dom::TabChild* tabChild = GetTabChild(this);
> > +  if (MissingRequiredTabChild(tabChild, mLoadInfo, "wyciwyg")) {
> 
> the same here as with the websocket.

I switched this to nullptr, because we don't have any corresponding verification in the parent when creating a WyciwygChannelParent.
Attachment #8592260 - Attachment is obsolete: true
We're using ServiceWorker-v2 for B2G-specific things so moving this there.
Blocks: ServiceWorkers-B2G
No longer blocks: ServiceWorkers-v1
Component: DOM → DOM: Service Workers
Comment on attachment 8614827 [details] [diff] [review]
Allow sending null PBrowser actors when there's a triggering principal which can be used for security checks

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

looks good

::: netwerk/ipc/NeckoParent.cpp
@@ +197,5 @@
> +
> +          // If a null tab parent is provided, we rely on comparing the requesting principal's
> +          // reported data against the provided load context's data.
> +          uint32_t reportedAppId = NECKO_UNKNOWN_APP_ID;
> +          aRequestingPrincipal->GetAppId(&reportedAppId);

you can check the return value here.

@@ +202,5 @@
> +          if (appId != reportedAppId) {
> +            return "app id mismatch for request without associated browser";
> +          }
> +          bool reportedInBrowser = false;
> +          aRequestingPrincipal->GetIsInBrowserElement(&reportedInBrowser);

also here.
Attachment #8614827 - Flags: review?(dd.mozilla) → review+
s/Windows//
Dragana, can you please take a look at this bug too if you have some free time?  Thanks!
Flags: needinfo?(dd.mozilla)
Attached patch take2ipdl_v2 (obsolete) — — Splinter Review 
In case of a redirect loadInfo was not present. I have change that for http and ftp channel
Flags: needinfo?(dd.mozilla)
There are another problem with appid not passing the test this patch introduces....
Attachment #8626538 - Attachment is patch: true
Attachment #8626538 - Attachment is obsolete: true
Attachment #8614827 - Attachment is obsolete: true
Depends on: 1188091
Attachment #8629038 - Attachment is obsolete: true
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/2bdaed564656451cc9de7f47f1120548b4cfe228
changeset:  2bdaed564656451cc9de7f47f1120548b4cfe228
user:       Josh Matthews <josh@joshmatthews.net>
date:       Wed Jun 03 15:07:42 2015 -0400
description:
Bug 1125961 - Allow sending null PBrowser actors when there's a triggering principal which can be used for security checks. r=bkelly,ddamjano
Flags: needinfo?(josh)
Blocks: 1207265
No longer blocks: ServiceWorkers-B2G
We aren't supporting b2g apps any more, so this is OBE.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: