1214572 - Should use a new process to load content when loading from a signed packaged web app.

Status: RESOLVED WONTFIX

(Core :: General, defect, P2)

Description

[Henry Chang [:hchang]]

In Bug 1186290 we only resolve half of the "process isolation" NSec story (loading signed packaged web content with a brand new process). We need to complete the other half.

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug1214572.patch

Comment 2

[Henry Chang [:hchang]]

Comment on attachment 8677477 [details] [diff] [review]
Bug1214572.patch

Hi Kanru,

The patch is to handle the case that loading a whatever page from a signed package (i.e. visit a signed package first then navigate to any other page).

The idea is pretty simple: do the switch as soon as possible but in the parent side. Although the load is triggered in the child process and we could know if it's loading from a signed package, the child process still needs to IPC to parent process to trigger the process switch. Therefore, I decide to do the switch in HttpChannelParent::DoAsyncOpen. If it's loading from a signed package (told by loadInfo.loadingPrincipal.originAttributes.signedPkg), notify TabParent and it will further decide if a process switch is needed. (necko part shouldn't be aware of the fact of process switch.)

Basically the process switch should be triggered for all kinds of top level document except that the "URI to load" has the same origin (no suffix) as the signed package. The reason we use "origin no suffix" here is the extended origin couldn't be known until actually loading it. If the extended origin turns out be different, a process switch will still be triggered in the subsequent OnStartSignedPackageRequest.

A test case is also included in the patch.

Thanks!

Comment 3

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

Comment 4

[Henry Chang [:hchang]]

Comment on attachment 8678704 [details] [diff] [review]
0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

Changed to asking for review since Bug 1180088 has been landed and I am thinking Bug 1178526 shouldn't block this.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8678704 [details] [diff] [review]
0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

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

Looks good but please address the comments in the test file and test again. Please find someone to review the HttpChannelParent change.

::: netwerk/test/mochitests/test_from_signed_web_packaged_app.html
@@ +38,5 @@
> +var nodePrincipalTestResult = undefined;
> +
> +// Listen for and count process-created event. Since we are loading a
> +// signed content from a remote tab, there shouls be two processes created.
> +// One is for remote tab and one for the signed package.

In this test case you are expecting 3 processes created, right?

@@ +47,5 @@
> +  if (processCreatedCnt < 3) {
> +    ok(true, "We have more processes to create: " + (3 - processCreatedCnt));
> +  } else if (processCreatedCnt == 3 && nodePrincipalTestResult === true) {
> +    // Considering the case that 'my-e10s-extension-message' is recevied earlier
> +    // than this test, we check the test result in both event handler.

Add a 'ok(nodePrincipalTestResult, ...)' statement.

@@ +99,5 @@
> +
> +    // Considering the case that kProcessCreatedTopic is recevied earlier
> +    // than this test, we check the test result in both event handler.
> +    if (processCreatedCnt === 3) {
> +      SimpleTest.finish();

Shouldn't you also check nodePrincipalTestResult here?

@@ +108,5 @@
> +`
> +function getNodePrincipalOrigin() {
> +sendAsyncMessage("my-e10s-extension-message", {}, { origin: content.document.nodePrincipal.origin });
> +}
> +`;

Indentation. What does the message name mean? Please replace it with a more meaningful one.

Comment 6

[Henry Chang [:hchang]]

Thanks Kanru!

I am trying to use TabContext to replace the current implementation to make it independent of Bug 1178526. No idea which approach is better so I will give it a try and see how it looks like.

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #5)
> Comment on attachment 8678704 [details] [diff] [review]
> 0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch
> 
> Review of attachment 8678704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but please address the comments in the test file and test again.
> Please find someone to review the HttpChannelParent change.
> 
> ::: netwerk/test/mochitests/test_from_signed_web_packaged_app.html
> @@ +38,5 @@
> > +var nodePrincipalTestResult = undefined;
> > +
> > +// Listen for and count process-created event. Since we are loading a
> > +// signed content from a remote tab, there shouls be two processes created.
> > +// One is for remote tab and one for the signed package.
> 
> In this test case you are expecting 3 processes created, right?
> 
> @@ +47,5 @@
> > +  if (processCreatedCnt < 3) {
> > +    ok(true, "We have more processes to create: " + (3 - processCreatedCnt));
> > +  } else if (processCreatedCnt == 3 && nodePrincipalTestResult === true) {
> > +    // Considering the case that 'my-e10s-extension-message' is recevied earlier
> > +    // than this test, we check the test result in both event handler.
> 
> Add a 'ok(nodePrincipalTestResult, ...)' statement.
> 
> @@ +99,5 @@
> > +
> > +    // Considering the case that kProcessCreatedTopic is recevied earlier
> > +    // than this test, we check the test result in both event handler.
> > +    if (processCreatedCnt === 3) {
> > +      SimpleTest.finish();
> 
> Shouldn't you also check nodePrincipalTestResult here?
> 
> @@ +108,5 @@
> > +`
> > +function getNodePrincipalOrigin() {
> > +sendAsyncMessage("my-e10s-extension-message", {}, { origin: content.document.nodePrincipal.origin });
> > +}
> > +`;
> 
> Indentation. What does the message name mean? Please replace it with a more
> meaningful one.

Comment 7

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

Comment 8

[Henry Chang [:hchang]]

Comment on attachment 8685207 [details] [diff] [review]
0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

Hi Kanru, Honza,

Could you help please review the patch again?

For Honza,

The only change in necko is in HttpChannelParent::DoAsyncOpen. We notify TabParent if loading from a signed package by testing if TabParent::OriginAttributes::mSignedPkg is empty. TabParent would return true to let us (TabParent::OnLoadingFromSignedPacakge) know a process switch is going to happen. In that case, just returns from DoAsyncOpen without doing the rest of work.

For Kanru,

The changes in TabParent mostly remain the same except for one edge case:

"TabParent's mSignedPkg is not empty but loading origin is 'moz-safe-about'."

The edge case is where the process is just created to load a signed package triggered by TabParent::OnStartSignedPackageRequest. Therefore, we don't need to switch process for the edge case.

Thanks you two!

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8685207 [details] [diff] [review]
0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

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

::: dom/ipc/TabParent.cpp
@@ +577,5 @@
> +    return false;
> +  }
> +
> +  if (nsIContentPolicy::TYPE_DOCUMENT != aLoadInfo->InternalContentPolicyType()) {
> +    // No switch process for non-top-level document. 

nit: trailing white space

::: netwerk/test/mochitests/test_from_signed_web_packaged_app.html
@@ +42,5 @@
> +// 2) When navigating to a signed package
> +// 3) When navigating outside of signed package (to http://example.org)
> +var kProcessCreatedTopic = "process-priority-manager:TEST-ONLY:process-created";
> +var processCreatedCnt = 0;
> +SpecialPowers.addObserver(() => {

I think this observer is leaked. It needs cleanup before we call finish().

@@ +47,5 @@
> +  processCreatedCnt++;
> +  if (processCreatedCnt < 3) {
> +    ok(true, "We have more processes to create: " + (3 - processCreatedCnt));
> +  } else if (processCreatedCnt == 3 && nodePrincipalTestResult === true) {
> +    // Considering the case that 'my-e10s-extension-message' is recevied earlier

Fix this comment too.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8685207 [details] [diff] [review]
0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

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

::: dom/ipc/TabParent.cpp
@@ +568,5 @@
> +
> +  if (loadingOriginNoSuffix.EqualsLiteral("moz-safe-about:blank")) {
> +    // The tab is just created for loading a signed package. No need to switch again.
> +    return false;
> +  }

docShell has a method GetHasLoadedNonBlankURI. Maybe we should have something similar in TabParent to prevent loading a signed package after loading a document. Could you file a bug to track this?

Comment 11

[Henry Chang [:hchang]]

Hi Honza,

Could you help review the patch regarding the necko part? The changes in HttpChannelParent.cpp is to notify TabParent when it loaded a signed package before. TabParent would decide if we have to switch process (this part has been reviewed by Kanru).

Thanks!

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Henry Chang [:henry] from comment #11)
> Hi Honza,
> 
> Could you help review the patch regarding the necko part? The changes in
> HttpChannelParent.cpp is to notify TabParent when it loaded a signed package
> before. TabParent would decide if we have to switch process (this part has
> been reviewed by Kanru).
> 
> Thanks!

Sure, just ask me for the review on the proper patch, I'll do it.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8685207 [details] [diff] [review]
0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

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

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +390,5 @@
> +    // Loading content by a tab which loaded a signed package before.
> +    if (mTabParent->OnLoadingFromSignedPackage(loadInfo, uri)) {
> +      // Do nothing and a new identifcal request will be made to
> +      // a new process.
> +      return true;

problem here is that the child channel will just indefinitely hang.  I think you should send back SendFailedAsyncOpen(NS_BINDING_ABORTED) either now or when we are sure to start (or fail) loading the uri in a new process.

what happens when OnLoadingFromSignedPackage returns true but we fail reaching to start the loading in a new process from some reason?  do we show any error to the user?  do we have any fail-over mechanisms?

if it's hard or we don't need to be concerned, just ask again for r and I'll r+ this patch.

Comment 14

[Henry Chang [:hchang]]

Hi Honza,

Thanks for your review :)

(In reply to Honza Bambas (:mayhemer) from comment #13)
> Comment on attachment 8685207 [details] [diff] [review]
> 0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch
> 
> Review of attachment 8685207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/HttpChannelParent.cpp
> @@ +390,5 @@
> > +    // Loading content by a tab which loaded a signed package before.
> > +    if (mTabParent->OnLoadingFromSignedPackage(loadInfo, uri)) {
> > +      // Do nothing and a new identifcal request will be made to
> > +      // a new process.
> > +      return true;
> 
> problem here is that the child channel will just indefinitely hang.  I think
> you should send back SendFailedAsyncOpen(NS_BINDING_ABORTED) either now or
> when we are sure to start (or fail) loading the uri in a new process.
> 

|TabParent::OnLoadingFromSignedPackage| (also in the patch) should return true 
only when the "process switch" happened. (i.e. nsFrameLoader::SwitchProcessAndLoadURI 
is called successfully.)

> what happens when OnLoadingFromSignedPackage returns true but we fail
> reaching to start the loading in a new process from some reason?  do we show
> any error to the user?  do we have any fail-over mechanisms?
> 

If we fail to load a new process and load URI again, |OnLoadingFromSignedPackage|
will return false (synchronously). Do you think we still have to do
"SendFailedAsyncOpen(NS_BINDING_ABORTED)"?

Thanks!

> if it's hard or we don't need to be concerned, just ask again for r and I'll
> r+ this patch.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Henry Chang [:henry] from comment #14)
> Hi Honza,
> 
> Thanks for your review :)
> 
> (In reply to Honza Bambas (:mayhemer) from comment #13)
> > Comment on attachment 8685207 [details] [diff] [review]
> > 0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch
> > 
> > Review of attachment 8685207 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: netwerk/protocol/http/HttpChannelParent.cpp
> > @@ +390,5 @@
> > > +    // Loading content by a tab which loaded a signed package before.
> > > +    if (mTabParent->OnLoadingFromSignedPackage(loadInfo, uri)) {
> > > +      // Do nothing and a new identifcal request will be made to
> > > +      // a new process.
> > > +      return true;
> > 
> > problem here is that the child channel will just indefinitely hang.  I think
> > you should send back SendFailedAsyncOpen(NS_BINDING_ABORTED) either now or
> > when we are sure to start (or fail) loading the uri in a new process.
> > 
> 
> |TabParent::OnLoadingFromSignedPackage| (also in the patch) should return
> true 
> only when the "process switch" happened. (i.e.
> nsFrameLoader::SwitchProcessAndLoadURI 
> is called successfully.)

Aha!  That is good.  Didn't know.  Worth a comment.

> 
> > what happens when OnLoadingFromSignedPackage returns true but we fail
> > reaching to start the loading in a new process from some reason?  do we show
> > any error to the user?  do we have any fail-over mechanisms?
> > 
> 
> If we fail to load a new process and load URI again,
> |OnLoadingFromSignedPackage|
> will return false (synchronously). Do you think we still have to do
> "SendFailedAsyncOpen(NS_BINDING_ABORTED)"?

Depends on what you want to do.  If you insist on either load in a process or rather fail completely, then send back the failure.  If you are OK to go w/o switching the process (I assume that is not the case for security reasons) just go on as you do in the patch.

So, I presume you should abort on failure of OnLoadingFromSignedPackage if failure of it is not just indication that "the switch is not needed" (what I presume is not according your comments).

> 
> Thanks!
> 
> > if it's hard or we don't need to be concerned, just ask again for r and I'll
> > r+ this patch.

Sorry for late answer!

Comment 16

[Henry Chang [:hchang]]

Hi Honza,

Thanks for your feedback!

(In reply to Honza Bambas (:mayhemer) from comment #15)
> > |TabParent::OnLoadingFromSignedPackage| (also in the patch) should return
> > true 
> > only when the "process switch" happened. (i.e.
> > nsFrameLoader::SwitchProcessAndLoadURI 
> > is called successfully.)
> 
> Aha!  That is good.  Didn't know.  Worth a comment.
> 

There's a comment in the function's header. I will add the comment in the callsite as well!

> > 
> > > what happens when OnLoadingFromSignedPackage returns true but we fail
> > > reaching to start the loading in a new process from some reason?  do we show
> > > any error to the user?  do we have any fail-over mechanisms?
> > > 
> > 
> > If we fail to load a new process and load URI again,
> > |OnLoadingFromSignedPackage|
> > will return false (synchronously). Do you think we still have to do
> > "SendFailedAsyncOpen(NS_BINDING_ABORTED)"?
> 
> Depends on what you want to do.  If you insist on either load in a process
> or rather fail completely, then send back the failure.  If you are OK to go
> w/o switching the process (I assume that is not the case for security
> reasons) just go on as you do in the patch.
> 
> So, I presume you should abort on failure of OnLoadingFromSignedPackage if
> failure of it is not just indication that "the switch is not needed" (what I
> presume is not according your comments).
> 

You remind me that there are two case where OnLoadingFromSignedPackage would return false:

1) The process switch is not required.
2) Process switch is required but failed to do it.

I might have to make it return not just a boolean to differentiate those two falsy cases.

Thanks!

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Henry Chang [:henry] from comment #16)
> I might have to make it return not just a boolean to differentiate those two
> falsy cases.

Definitely!  These two must be distinguished.

Comment 18

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

Comment 19

[Henry Chang [:hchang]]

Comment on attachment 8719394 [details] [diff] [review]
0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

Hi Honza,

May I have your review for the new attached patch? It now takes "failing to switch process" into account as we discussed earlier. Thanks :)

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8719394 [details] [diff] [review]
0001-Bug-1214572-When-loading-from-a-signed-package-switc.patch

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

::: dom/ipc/TabParent.cpp
@@ +573,5 @@
> +{
> +  // We need to switch process except aURI has the same origin (w/o suffix)
> +  // as aLoadInfo.originNoSuffix. If the origin (with suffix) associated with
> +  // aURI is a signed and has different origin from loading origin,
> +  // a process switch will still be triggered in OnStartSignedPackageRequest.

// We need to switch process except when aURI has the same origin (w/o suffix)
	
  // as aLoadInfo.originNoSuffix. If the origin (with suffix) associated with
	
  // aURI is signed and has a different origin from the loading origin,
	
  // a process switch will still be triggered in OnStartSignedPackageRequest.
	

Few grammar things fixes, tho this comment still doesn't explain to me what happens.  Can you rephrase?

@@ +582,5 @@
> +  aWouldSwitchProcess = false;
> +
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +  rv = aLoadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
> +  NS_ENSURE_TRUE(loadingPrincipal, rv);

NS_ENSURE_SUCCESS(rv, rv); is not enough here?  can it happen that !loadingPrincipal and rv == NS_OK?  Is it ok to not switch the loading process and not interrupt the origin load (i.e. load in the current process?)

kinda undeterministic...

I'd like to see this resolved first.

@@ +606,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (nsIContentPolicy::TYPE_DOCUMENT != aLoadInfo->InternalContentPolicyType()) {
> +    // No switch process for non-top-level document.

No process switch

::: dom/ipc/TabParent.h
@@ +621,5 @@
>  
> +    // Called by HttpChannelParent::RecvAsyncOpen. 
> +    // When it returns NS_OK, checks aWouldSwitchProcess to know if
> +    // a process switch is going to happen. Otherwise, an error is occured
> +    // while switching process.

nit: trailing white space

better comment:

Called by HttpChannelParent::RecvAsyncOpen.
@returns
  failure - we failed the process switch, that was required.  this is fatal and the original load has to be canceled
  success - conditions to do process switching has NOT been met (aWouldSwitchProcess is returned false), or has been met and process switch has been successful (aWouldSwitchProcess is returned true) ; in the latter case the original load must not continue

@@ +624,5 @@
> +    // a process switch is going to happen. Otherwise, an error is occured
> +    // while switching process.
> +    nsresult OnLoadingFromSignedPackage(nsILoadInfo* aLoadInfo,
> +                                        nsIURI* aURI,
> +                                        bool& aWouldSwitchProcess);

nit: wrong indention

also is |aWouldSwitchProcess| a correct name?  aProcessSwitched maybe?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +285,5 @@
> +  if (mTabParent && !mTabParent->OriginAttributesRef().mSignedPkg.IsEmpty()) {
> +    // Loading content by a tab which loaded a signed package before.
> +    bool wouldSwitchProcess;
> +    rv = mTabParent->OnLoadingFromSignedPackage(loadInfo, uri, wouldSwitchProcess);
> +    if (!NS_SUCCEEDED(rv)) {

NS_FAILED(rv)

Comment 21

[Henry Chang [:hchang]]

Process switch for signed package is no longer needed.