1179191 - When updating a sw via about:sw in b2g the parent process kills the settings app

Status: RESOLVED DUPLICATE of bug 1225121

(Core :: DOM: Core & HTML, defect)

Description

[Noemí Freire (:noemi)]

STRs:
1- register a sw (via hosted app or browser)
2- go to about:sw, the sw is properly listed
3- click on the “update” button, nothing happens since there is no sw script change
4- re-start the phone
5- change the sw script or not. open the app or not
6- go to about:sw
7- click on the “update” button

Actual result:
Settings/System app crashes

Expected result:
Nothing should crash, in case a change in the sw script has been performed, clicking on the "update" button should launch the corresponding update process, in case not, nothing should happen


Environmental variables:
Flame device
Build Id: 20150701063840
Gecko: b617e38
Gaia: b8b329a
Platform version: 42.0a1

Comment 1

[Noemí Freire (:noemi)]

Hi,

With today's (7/9) master build, Settings/System app crashes most of the attempts when trying to update via about:sw without the need of restarting the device so changing the subject accordingly.
The issue seems to be that processes which nsIPrincipal is not the one that registered the sw are performing update proceses. Adding 1182117 dependency.

Environmental variables:
Flame device
Build Id: 20150709141033
Gecko: 60fd820
Gaia: 20e6960
Platform version: 42.0a1

Comment 2

[Noemí Freire (:noemi)]

(In reply to Noemí Freire (:noemi) from comment #1)

> The issue seems to be that processes which nsIPrincipal is not the one that
> registered the sw are performing update proceses. Adding 1182117 dependency.

ni to José Antonio for a more detailed explanation about which is the root cause with update/unregister (Bug 1175949) issues. Thanks!

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Noemí Freire (:noemi) from comment #2)
> (In reply to Noemí Freire (:noemi) from comment #1)
> 
> > The issue seems to be that processes which nsIPrincipal is not the one that
> > registered the sw are performing update proceses. Adding 1182117 dependency.
> 
> ni to José Antonio for a more detailed explanation about which is the root
> cause with update/unregister (Bug 1175949) issues. Thanks!

If I'm not wrong the issue is basically that there are chances a child process tries to update/unregister a SW that was registered with a different principal. Let say conten process A registers a SW, content process B is around and content process C tries to update A's SW via about:sw. The update request is propagated from content process C to content process A and content process B. When content process B tries to update the SW the parent process asserts the app principal and it kills content process B.

Find below part of the log seen when updating the SW. The logic asserting the app principal is part of the cache API code so ni? to bkelly. baku and I have been discussing about it during this week. Catalin is working on a refactor (bug 1182117) that will fix this issue. baku, am I right?

I/Gecko   (  212): [Parent 212] WARNING: Principal is invalid, killing app process: file /Volumes/firefoxos/dev/mozilla-central/dom/ipc/AppProcessChecker.cpp, line 221
I/Gecko   (  212): [Parent 212] WARNING: 'actor && !AssertAppPrincipal(actor, principal)', file /Volumes/firefoxos/dev/mozilla-central/dom/cache/PrincipalVerifier.cpp, line 135
I/Gecko   (  212): [Parent 212] WARNING: waitpid failed pid:1899 errno:10: file /Volumes/firefoxos/dev/mozilla-central/ipc/chromium/src/base/process_util_posix.cc, line 267

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Ah, yes.  This will be a problem for something like devtools as well if it was running with child IPC security enabled.  While moving the SWM to the parent will work for the SW case, it would not help for the devtools or other cases.

One solution might be to add teach CacheStorage that it really has two principals; the parent's principal and its target principal.  We would perform the anti-spoofing check only on the parent principal.  If the parent principal is a system principal then we would allow the target principal to be different (and unverified).

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Dimi, I understand you're working on b2g-related service worker things.  Would you be interested in doing the CacheStorage changes I outline in comment 5?  Its probably something that could be fixed sooner than the SWM refactor and might help other cases.  Thanks.

Comment 6

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> Dimi, I understand you're working on b2g-related service worker things. 
> Would you be interested in doing the CacheStorage changes I outline in
> comment 5?  Its probably something that could be fixed sooner than the SWM
> refactor and might help other cases.  Thanks.
Hi Ben,
Yes, I would like to help.
Is there already a bug for that ? if not, i will create one.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #6)
> Is there already a bug for that ? if not, i will create one.

You can just do it in this bug I think.

Comment 8

[Dimi Lee [:dimi]]

Hi Ben
Have some questions about the solution you mentioned in comment 4.
1.
I am not quite sure if I understand 'target principal' & 'parent principal' correctly. In the scenario of Comment 3, does target principal stand for the principal passed to CacheStorage and parent principal stand for the principal of parent process(b2g process's principal in this case) ?

If above thing is correct, do you mean we should modify the logic in
https://dxr.mozilla.org/mozilla-central/source/dom/cache/PrincipalVerifier.cpp#135
to include check if parent principal is a system principal ?

2.
If this solution is applyed, does that mean any content process could update service worker ?
Is that what we want ?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I didn't really explain my terminology.

So, a CacheStorage object is created to store data for a particular principal.  This is what I was calling the "target principal".

Currently the "target principal" is essentially inherited from whatever code creates the CacheStorage.  This differs a little bit depending on whether its main thread or worker thread:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/CacheStorage.h#51
  https://dxr.mozilla.org/mozilla-central/source/dom/cache/CacheStorage.h#56

The principal of the person creating the CacheStorage is what I'm calling the "parent principal".

Today, CacheStorage assumes that the target principal and the parent principal are the same.  This is an assumption from when I first wrote the code.  Its no longer valid now that arbitrary principals can be passed to CreateOnMainThread() and CacheStorage::Constructor().

So what I think we need to do is:

1) Modify CreateOnMainThread() to take two principals: aTargetPrincipal and aParentPrincipal
2) The .caches getter in nsGlobalWindow would specify the window's principal for both of these when calling CreateOnMainThread().
3) The CacheStorage::Constructor() would need to get the parent principal out of the global in order to call the right values on CreateOnMainThread().  (Since this is a chrome-only constructor, we can also probably assert that its the system principal here.)
4) The code in dom/workers/ScriptLoader.cpp and ServiceWorkerScriptCache.cpp would need to be modified to pass the system principal for the parent principal.
5) You would then need to modify the PCacheStorage() method in ipc/glue/PBackground.ipdl to take both the target and parent principals.
6) As you suggested, you would then need to modify the PrincipalVerifier code.  It would need to store both target and parent principal.
7) In The PrincipalVerifier::VerifyOnMainThread() it would then:
  a) verify the parent principal matches the child process (this the check thats currently there)
  b) if the parent principal is not the system principal, verify the target principal is identical to the parent principal
8) The target principal is then used to create ManagerId
9) We should also probably add a check somewhere that if the Namespace is ChromeOnly, then the parent principal is the system principal.  You could probably do this directly in the CacheStorageParent on the worker thread, though.

Does that help?

Sorry the timezones make coordinating difficult.  I will try to check my email before going to bed in the evenings.

Comment 10

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> Sorry, I didn't really explain my terminology.
> 
> So, a CacheStorage object is created to store data for a particular
> principal.  This is what I was calling the "target principal".
> 
> Currently the "target principal" is essentially inherited from whatever code
> creates the CacheStorage.  This differs a little bit depending on whether
> its main thread or worker thread:
> 
>   https://dxr.mozilla.org/mozilla-central/source/dom/cache/CacheStorage.h#51
>   https://dxr.mozilla.org/mozilla-central/source/dom/cache/CacheStorage.h#56
> 
> The principal of the person creating the CacheStorage is what I'm calling
> the "parent principal".
> 
> Today, CacheStorage assumes that the target principal and the parent
> principal are the same.  This is an assumption from when I first wrote the
> code.  Its no longer valid now that arbitrary principals can be passed to
> CreateOnMainThread() and CacheStorage::Constructor().
> 
> So what I think we need to do is:
> 
> 1) Modify CreateOnMainThread() to take two principals: aTargetPrincipal and
> aParentPrincipal
> 2) The .caches getter in nsGlobalWindow would specify the window's principal
> for both of these when calling CreateOnMainThread().
> 3) The CacheStorage::Constructor() would need to get the parent principal
> out of the global in order to call the right values on CreateOnMainThread().
> (Since this is a chrome-only constructor, we can also probably assert that
> its the system principal here.)
> 4) The code in dom/workers/ScriptLoader.cpp and ServiceWorkerScriptCache.cpp
> would need to be modified to pass the system principal for the parent
> principal.
> 5) You would then need to modify the PCacheStorage() method in
> ipc/glue/PBackground.ipdl to take both the target and parent principals.
> 6) As you suggested, you would then need to modify the PrincipalVerifier
> code.  It would need to store both target and parent principal.
> 7) In The PrincipalVerifier::VerifyOnMainThread() it would then:
>   a) verify the parent principal matches the child process (this the check
> thats currently there)
>   b) if the parent principal is not the system principal, verify the target
> principal is identical to the parent principal
> 8) The target principal is then used to create ManagerId
> 9) We should also probably add a check somewhere that if the Namespace is
> ChromeOnly, then the parent principal is the system principal.  You could
> probably do this directly in the CacheStorageParent on the worker thread,
> though.
> 
> Does that help?
> 
> Sorry the timezones make coordinating difficult.  I will try to check my
> email before going to bed in the evenings.

Thanks for detailed explanation, it really helps!
I will start working on this and let you know if I have any question.

Comment 11

[Dimi Lee [:dimi]]

Attachment: Patch v1

Hi Ben,
I haven't done lots of tests for this patch, just upload first so you can take a look to check if there is any major problem in this patch.

I am not sure if the logic in PrincipalVerifier is the same as you suggest in comment 9, so may need your help to check this part more detaily, thanks!

Also I have one question about comment 9 point 4, you mentioned that we can use system principal directly for ScriptLoader.cpp and ServiceWorkerScriptCache.cpp.
Does that mean |CacheStorage::CreateOnMainThread| in these two files will only be executed with chrome privileged ? If yes, could you explain a little bit about the reason? I think it could help me understand more about sw, thanks for your help !

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8635253 [details] [diff] [review]
Patch v1

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

Looks great!

r=me with action items addressed.  In particular, I'd like to explicitly name the variable targetPrincipal (instead of just principal) from CacheStorage.h to PrincipalVerifier.cpp.  You can leave ManagerId.h on as just principal.

::: dom/cache/CacheStorage.cpp
@@ +161,5 @@
>      nsRefPtr<CacheStorage> ref = new CacheStorage(NS_ERROR_DOM_SECURITY_ERR);
>      return ref.forget();
>    }
>  
>    PrincipalInfo principalInfo;

Again, can you rename this to targetPrincipalInfo here and other places for clarity.

@@ +179,5 @@
>    bool testingEnabled = aForceTrustedOrigin ||
>      Preferences::GetBool("dom.caches.testing.enabled", false) ||
>      Preferences::GetBool("dom.serviceWorkers.testing.enabled", false);
>  
>    if (!IsTrusted(principalInfo, testingEnabled)) {

I think we probably want to call IsTrusted() for both the target and the parent principal.

::: dom/cache/CacheStorage.h
@@ +48,5 @@
>  
>  public:
>    static already_AddRefed<CacheStorage>
>    CreateOnMainThread(Namespace aNamespace, nsIGlobalObject* aGlobal,
> +                     nsIPrincipal* aPrincipal, nsIPrincipal* aParentPrincipal,

I think I would prefer to call this aTargetPrincipal here and other places to be clear.  A comment block describing these two would also be useful here for people trying to figure out how to use the API.

@@ +94,5 @@
>    CreatePushStream(nsIAsyncInputStream* aStream) override;
>  
>  private:
>    CacheStorage(Namespace aNamespace, nsIGlobalObject* aGlobal,
> +               const mozilla::ipc::PrincipalInfo& aPrincipalInfo, 

nit: trailing whitespace
aTargetPrincipalInfo

@@ +104,5 @@
>    void MaybeRunPendingRequests();
>  
>    const Namespace mNamespace;
>    nsCOMPtr<nsIGlobalObject> mGlobal;
>    UniquePtr<mozilla::ipc::PrincipalInfo> mPrincipalInfo;

mTargetPrincipalInfo

::: dom/cache/CacheStorageParent.cpp
@@ +48,5 @@
>    MOZ_COUNT_CTOR(cache::CacheStorageParent);
>    MOZ_ASSERT(aManagingActor);
>  
> +  if (CHROME_ONLY_NAMESPACE == mNamespace) {
> +    MOZ_ASSERT(aParentPrincipalInfo.type() == PrincipalInfo::TSystemPrincipalInfo);

nit: use MOZ_ASSERT_IF() instead of a separate if statement

::: dom/cache/PrincipalVerifier.cpp
@@ +137,4 @@
>    // We disallow null principal and unknown app IDs on the client side, but
>    // double-check here.
>    if (NS_WARN_IF(principal->GetIsNullPrincipal() ||
>                   principal->GetUnknownAppId())) {

I don't think its strictly necessary, but can you check parentPrincipal for null and unknown app ID here as well.  I think we'll effectively check this later on by comparing for system or equality, but if that changes for some reason I don't want this condition to slip through.

::: ipc/glue/PBackground.ipdl
@@ +55,5 @@
>    PServiceWorkerManager();
>  
>    ShutdownServiceWorkerRegistrar();
>  
> +  PCacheStorage(Namespace aNamespace, PrincipalInfo aPrincipalInfo,

Please use aTargetPrincipalInfo for clarity.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8635253 [details] [diff] [review]
Patch v1

Noemi, can you retest about:serviceworkers on b2g with this patch applied?  Does it fix the issue?

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8635253 [details] [diff] [review]
Patch v1

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

::: dom/cache/PrincipalVerifier.cpp
@@ +148,5 @@
>      return;
>    }
>  
> +  // If the parent principal is a system principal then we would allow the target
> +  // principal to be different

Can you also make a note in the comment that we are explicitly allowing the target principal to be different from the principal for the child actor to support use cases like devtools and about:serviceworkers?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8635253 [details] [diff] [review]
Patch v1

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

::: dom/cache/CacheStorageParent.cpp
@@ +48,5 @@
>    MOZ_COUNT_CTOR(cache::CacheStorageParent);
>    MOZ_ASSERT(aManagingActor);
>  
> +  if (CHROME_ONLY_NAMESPACE == mNamespace) {
> +    MOZ_ASSERT(aParentPrincipalInfo.type() == PrincipalInfo::TSystemPrincipalInfo);

On second thought, I think we should make this a runtime check to prevent spoofing the namespace.

I'm not sure how to return an error during an actor allocation, but you can do the check as part of the PrincipalVerifier.

Comment 16

[Dimi Lee [:dimi]]

Attachment: Patch v2

Hi Ben,
There are lots of changes in this patch, although mostly are naming rule. But could you help take a look at it again ? thanks!

Major changes in this patch:
1.Naming change : use targetPrincipal 
2.Add comments
3.Move namespace checking from CacheStorageParent.cpp to PrincipalVerifier.cpp
4.Address nits

Comment 17

[Noemí Freire (:noemi)]

Attachment: bug1179191_apply_check_log.rtf

(In reply to Ben Kelly [:bkelly] from comment #13)
> Comment on attachment 8635253 [details] [diff] [review]
> Patch v1
> 
> Noemi, can you retest about:serviceworkers on b2g with this patch applied? 
> Does it fix the issue?

Hi,

I've tried to apply that patch but it fails:
error: patch failed: dom/base/nsGlobalWindow.cpp:10656
error: dom/base/nsGlobalWindow.cpp: patch does not apply

waiting for your feedback. Thanks!

Comment 18

[Dimi Lee [:dimi]]

Attachment: Patch v3

This patch is based on patch v2 and rebase to latest code

Hi Ben,
As mentioned in Comment 16, could you help check it again ? thanks

Hi Noemi,
Could you help use this patch to test again ? It should solve the patch apply problem, thanks

Comment 19

[Noemí Freire (:noemi)]

(In reply to Dimi Lee[:dimi][:dlee] from comment #18)
> Created attachment 8635926 [details] [diff] [review]
> Patch v3

> 
> Hi Noemi,
> Could you help use this patch to test again ? It should solve the patch
> apply problem, thanks

Hi,

I've just checked Patch v3 and the issue still persists, settings app (pid:3976) is killed while updating:

I/Gecko   ( 3976): Execution context(v1): opening cache...
I/Gecko   ( 1991): [Parent 1991] WARNING: Principal is invalid, killing app process: file /Volumes/firefoxos/B2G/gecko/dom/ipc/AppProcessChecker.cpp, line 221
I/Gecko   ( 1991): [Parent 1991] WARNING: 'actor && !AssertAppPrincipal(actor, parentPrincipal)', file /Volumes/firefoxos/B2G/gecko/dom/cache/PrincipalVerifier.cpp, line 170
I/Gecko   ( 1991): [Parent 1991] WARNING: 'NS_FAILED(aRv)', file /Volumes/firefoxos/B2G/gecko/dom/cache/CacheStorageParent.cpp, line 136
I/Gecko   ( 1991): [Parent 1991] WARNING: waitpid failed pid:3976 errno:10: file /Volumes/firefoxos/B2G/gecko/ipc/chromium/src/base/process_util_posix.cc, line 267
I/Gecko   ( 1991): [Parent 1991] WARNING: 'NS_FAILED(mVerifiedStatus)', file /Volumes/firefoxos/B2G/gecko/dom/cache/CacheStorageParent.cpp, line 108
I/Browser ( 2987): Content JS LOG: Execution context(v1): opened cache so adding content!

Comment 20

[Dimi Lee [:dimi]]

Comment on attachment 8635926 [details] [diff] [review]
Patch v3

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

Cancel review because Noemi still can reproduce this issue after applying patch.
But this patch did work in my environment, I will keep checking this.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

I'm in meetings all day today, but I'll look at this tomorrow.

Comment 22

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #21)
> I'm in meetings all day today, but I'll look at this tomorrow.
Hi Ben,
I found this issue happened when calling CacheStorage::CreateOnWorker, in this patch, i wrote following:

nsRefPtr<CacheStorage> ref = new CacheStorage(aNamespace, aGlobal,
                                              principalInfo, principalInfo, feature);

In this case, |principalInfo| is not system principal, so error occurs when verify it in PrincipalVerifier.cpp. Should i change parent principal to system principal ?

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Wait, are we creating a CacheStorage for a different origin on the worker?  What is the call site for that?  I didn't think we did that, but maybe we do.

If so, we need to let the parent principal be passed in to the CreateOnWorker() method.

Comment 24

[Dimi Lee [:dimi]]

Hi Ben,
After applying current patch, update procedure will pass PrincipalVerifier check and trigger "update" algorithm[1] and then 'install' event will be fired. Apps receive 'install' event and call cache related API in event handler, this will lead the code to execute |CacheStorage::CreateOnWorker|.

[1]https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?from=ContinueUpdate#1210
[2]https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerScope.cpp#135

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Yea, I think we need an architectural change somewhere.  The AssertAppPrincipal() function assumes just a single principal is valid per process.  This can easily be violated using 3rd party iframes or by something like about:serviceworkers starts a cross-origin service worker to update.

I'm going to try to talk to Jonas today or tomorrow about it.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

So after speaking to Jonas today, I'm no longer certain my idea in comment 4 is a good idea.

Consider, we one of the points of using separate processes is to sandbox untrusted code.  We can trust that IPC messages coming from a process have not been spoofed.  In particular, what happens if a child process spoofs a principal to be the system principal.

Today, all that happen with a spoofed system principal is you can store stuff in the chrome origin's directory, but nothing is really there for CacheStorage.

With my idea in comment 4, however, an attacker could spoof the system principal and then another arbitrary origin for the target.  This would give them access to the CacheStorage for any origin.

This is bad.  We should not do it.

Dimi, I'm sorry to have spent your time on this. :-(

I think the only solution here is the refactor that moves the ServiceWorkerManager to the parent process.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8635926 [details] [diff] [review]
Patch v3

As I stated in comment 26, my conversation with Jonas convinced me this is no longer a good idea.  I'm sorry again for the bad direction.

Comment 28

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #27)
> Comment on attachment 8635926 [details] [diff] [review]
> Patch v3
> 
> As I stated in comment 26, my conversation with Jonas convinced me this is
> no longer a good idea.  I'm sorry again for the bad direction.

No problem, I have learned a lots from this bug and i am happy about it :)