1178526 - Set appropriate origin attributes for signed packages

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Once package signatures have been verified we need to ensure that packages receive the appropriate origin attributes (and vice versa). See also bug 1163254.

Comment 1

[Henry Chang [:hchang]]

Just discussed with Kanru earlier about this. When necko notifies to switch process, the new origin will also be delivered so that the new process would know what the origin it should be. In other words, "setting new origin for a signed package" is very likely to happen in the process switching point.

I will still leave this issue open at the moment and set it to dependent on the bug that Kanru is mainly working on.

Comment 2

[Henry Chang [:hchang]]

It seems the origin still needs to be set to what we extract from the package manifest.

Comment 3

[Dimi Lee [:dimi]]

Discussed with henry, i will take this bug

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Moving to p2 since technically devs can use without this.

Comment 5

[Dimi Lee [:dimi]]

Attachment: (WIP) Patch v1

Hi Henry,
Could you help check if this fit your requirement ? thanks

Comment 6

[Dimi Lee [:dimi]]

Attachment: (WIP) Patch v2

Comment 7

[Dimi Lee [:dimi]]

Patch V2 fix issue that parent side cannot receive packageId

Hi Henry,
Please let me know if there is any problem with this patch, thanks

Comment 8

[Henry Chang [:hchang]]

Attachment: Bug1178526.patch

Comment 9

[Henry Chang [:hchang]]

The patch depends on Bug 1186290 and Bug 1163254:

Bug 1163254 will extend originAttributes to have a "packageId" property. When it's not empty, it means the content is from a signed package.

This bug will try to set the originAttribute's packageId when the network layer notifies that a signed package is ready to load. Bug 1186290 will add a callback chain with packageId carried through 

nsHttpChannel => HttpChannelParentListener => HttpChannelParent.

In order to update the origin, our approach is to store the packageId into the loadcontext of the channel (both parent and child side) On parent side, we store into loadcontext so that when we call GetChannelResultPrincipal(), we can get a principal with originAttribute containing the packageId. On child side, we store into loadcontext so that when nsDocument is about to create a node principal, it will get the principal with updated packageId.

The IPC is involved since we need to deliver the packageId through PHttpChannel::OnStartRequest.

Not sure if the loadContext is the best placeholder for the packageId but it seems simple enough to get the things done and works perfectly.

Comment 10

[Henry Chang [:hchang]]

Comment on attachment 8659180 [details] [diff] [review]
Bug1178526.patch

Hi Jonas,

Since Bob Holly is on PTO, would you mind providing some feedback regarding the patch? Please see the last comment for the detailed explanation of this patch. Thanks!

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8659180 [details] [diff] [review]
Bug1178526.patch

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

few nits:

::: netwerk/protocol/http/nsHttpChannel.h
@@ +509,5 @@
>      // If non-null, warnings should be reported to this object.
>      HttpChannelSecurityWarningReporter* mWarningReporter;
>  
> +    // Indicate that if this channel is associated with a signed content.
> +    bool mIsSignedContent;

please add this among the bit fields.

::: uriloader/prefetch/OfflineCacheUpdateParent.h
@@ +52,5 @@
>      bool mIPCClosed;
>  
>      bool     mIsInBrowserElement;
>      uint32_t mAppId;
> +    nsCString mPackageId;

what are these offline cache update changes?

Comment 12

[Henry Chang [:hchang]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #11)
> Comment on attachment 8659180 [details] [diff] [review]
> Bug1178526.patch
> 
> Review of attachment 8659180 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> few nits:
> 
> ::: netwerk/protocol/http/nsHttpChannel.h
> @@ +509,5 @@
> >      // If non-null, warnings should be reported to this object.
> >      HttpChannelSecurityWarningReporter* mWarningReporter;
> >  
> > +    // Indicate that if this channel is associated with a signed content.
> > +    bool mIsSignedContent;
> 
> please add this among the bit fields.
> 

Do you refer to a specific existing one or use a new variable? I cannot find a proper one
in nsHttpChannel.h/HttpBaseChannel.h

> ::: uriloader/prefetch/OfflineCacheUpdateParent.h
> @@ +52,5 @@
> >      bool mIPCClosed;
> >  
> >      bool     mIsInBrowserElement;
> >      uint32_t mAppId;
> > +    nsCString mPackageId;
> 
> what are these offline cache update changes?

That's because OfflineCacheUpdateParent inherits from nsILoadContext and
we need to implement Set/GetPackageId. To be honest, I don't know if we
should actually need to implement the setter/getter or use stub ones like [1]


[1] https://dxr.mozilla.org/mozilla-central/source/uriloader/prefetch/OfflineCacheUpdateParent.cpp?offset=1100#243

Comment 13

[Henry Chang [:hchang]]

Attachment: Bug1178526.patch

Comment 14

[Henry Chang [:hchang]]

Comment on attachment 8659702 [details] [diff] [review]
Bug1178526.patch

Sorry that I forget to add the changes of HttpChannelParent.cpp/HttpChannelChild.cpp to the previous patch. By the way, the reason I put some code in nsHttpChannel is to consider the in-process http request as well.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Henry Chang [:henry] from comment #12)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #11)
> > Comment on attachment 8659180 [details] [diff] [review]
> > Bug1178526.patch
> > 
> > Review of attachment 8659180 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > few nits:
> > 
> > ::: netwerk/protocol/http/nsHttpChannel.h
> > @@ +509,5 @@
> > >      // If non-null, warnings should be reported to this object.
> > >      HttpChannelSecurityWarningReporter* mWarningReporter;
> > >  
> > > +    // Indicate that if this channel is associated with a signed content.
> > > +    bool mIsSignedContent;
> > 
> > please add this among the bit fields.
> > 
> 
> Do you refer to a specific existing one or use a new variable? I cannot find
> a proper one
> in nsHttpChannel.h/HttpBaseChannel.h

Among these: http://hg.mozilla.org/mozilla-central/annotate/c0abc2a6e11f/netwerk/protocol/http/nsHttpChannel.h#l450

> 
> > ::: uriloader/prefetch/OfflineCacheUpdateParent.h
> > @@ +52,5 @@
> > >      bool mIPCClosed;
> > >  
> > >      bool     mIsInBrowserElement;
> > >      uint32_t mAppId;
> > > +    nsCString mPackageId;
> > 
> > what are these offline cache update changes?
> 
> That's because OfflineCacheUpdateParent inherits from nsILoadContext and
> we need to implement Set/GetPackageId. To be honest, I don't know if we
> should actually need to implement the setter/getter or use stub ones like [1]

Aha.  Probably a stub is OK.  I think I will revisit these all in bug 1165256 anyway, so don't worry that much.

> 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/uriloader/prefetch/
> OfflineCacheUpdateParent.cpp?offset=1100#243

Comment 16

[Henry Chang [:hchang]]

Hi Bobby,

Just like what I mentioned in Bug 1163254 comment 31, are you okay to review or give some feedback of the patch? You can find the detailed explanation in comment 9 and I am willing to answer any question to let you know my idea as possible.

Thanks!

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8659702 [details] [diff] [review]
Bug1178526.patch

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

Looks good to me, though of course we ideally should use OriginAttributes rather than adding more members. But I think that's happening elsewhere already?

Comment 18

[Bobby Holley (:bholley)]

Yeah, I think this patch should land on top of bug 1165466, which adds OriginAttributes to TabContext. I don't think we should add more properties before then, since we'll just need to remove them again.

Comment 19

[Henry Chang [:hchang]]

Thanks Jonas and Bobby :)

Already set dependency on Bug 1165466. Thanks!

Comment 20

[Henry Chang [:hchang]]

Attachment: set-origin-process-switch.diff

Comment 21

[Henry Chang [:hchang]]

Attachment: Bug1178526.patch

Comment 22

[Henry Chang [:hchang]]

Attachment: Part 1: Add attr 'packageIdentifier' to nsIPackagedAppUtils

Comment 23

[Henry Chang [:hchang]]

Attachment: Part 2: Call OnStartSignedPackageRequest with packageIdentifier

Comment 24

[Henry Chang [:hchang]]

Attachment: Part 3: Populate packageId to child-side origin attribute

Comment 25

[Henry Chang [:hchang]]

Attachment: Part 4: Test cases

Comment 26

[Henry Chang [:hchang]]

Comment on attachment 8670721 [details] [diff] [review]
Part 1: Add attr 'packageIdentifier' to nsIPackagedAppUtils

Hi Valentin,

Could you please help review this simple patch which add support of extracting the packageIdentifier from the manifest? You can find the test case in patch Part 4. Thanks!

Comment 27

[Henry Chang [:hchang]]

Comment on attachment 8670723 [details] [diff] [review]
Part 2: Call OnStartSignedPackageRequest with packageIdentifier

This patch is to callback OnStartSignedPackageRequest with package-identifier we parsed from manifest. Also, the metadata we write to the cache becomes the packageId instead of the origin. I replace nsIPackagedAppVerifier.packageOrigin with packageIdentifier and PackagedAppService will callback |OnStartSignedPackageRequest| along with nsIPackagedAppVerifier.packageIdentifier

Comment 28

[Henry Chang [:hchang]]

Comment on attachment 8670726 [details] [diff] [review]
Part 3: Populate packageId to child-side origin attribute

Hi Bobby,

I wonder if you are available to review this patch? The main purpose is to populate the package-identifier we extracted in the network layer (parent side) to child side load context (nsDocShell) so that the content could get a node principal with signedPkg properly set.

Thanks!

Comment 29

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50b477cee997

Comment 30

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8670721 [details] [diff] [review]
Part 1: Add attr 'packageIdentifier' to nsIPackagedAppUtils

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

::: netwerk/protocol/http/PackagedAppUtils.js
@@ +56,5 @@
>        let manifestBody = aManifest.substr(aManifest.indexOf('\r\n\r\n') + 4);
>        debug("manifestBody: " + manifestBody);
>  
>        // Parse manifest, store resource hashes
> +      let manifestObj = JSON.parse(manifestBody); 

Trailing whitespace.

Comment 31

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8670727 [details] [diff] [review]
Part 4: Test cases

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

Thanks for adding tests.

::: netwerk/test/mochitests/signed_web_packaged_app.sjs
@@ +16,3 @@
>  
>    content: [
> +   { headers: ["Content-Location: manifest.webapp", "Content-Type: application/x-web-app-manifest+json"], 

Trailing whitespace.

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8670726 [details] [diff] [review]
Part 3: Populate packageId to child-side origin attribute

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

The parts in LoadContext/DocShell/TabParent look fine, but you should get a Necko person to review the stuff on the network side.

Also, we still need bug 1209162 in order for our handling of signedPkg to be correct.

Comment 33

[Bobby Holley (:bholley)]

Comment on attachment 8670727 [details] [diff] [review]
Part 4: Test cases

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

Somebody who's already familiar with these tests should review them.

Comment 34

[Henry Chang [:hchang]]

(In reply to Bobby Holley (:bholley) from comment #32)
> Comment on attachment 8670726 [details] [diff] [review]
> Part 3: Populate packageId to child-side origin attribute
> 
> Review of attachment 8670726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The parts in LoadContext/DocShell/TabParent look fine, but you should get a
> Necko person to review the stuff on the network side.
> 
> Also, we still need bug 1209162 in order for our handling of signedPkg to be
> correct.

Thanks, Bobby :)

Regarding Bug 1209162, do you specifically mean the nsDocShell part? It seems the only dependency of this bug.

Comment 35

[Bobby Holley (:bholley)]

(In reply to Henry Chang [:henry] from comment #34)
> > The parts in LoadContext/DocShell/TabParent look fine, but you should get a
> > Necko person to review the stuff on the network side.
> > 
> > Also, we still need bug 1209162 in order for our handling of signedPkg to be
> > correct.
> 
> Thanks, Bobby :)
> 
> Regarding Bug 1209162, do you specifically mean the nsDocShell part? It
> seems the only dependency of this bug.

I mean InheritFromDocToChildDocShell, InheritFromDocShellToChildDocShell, InheritFromDocShellToChannel, which prevent us fro accidentally inheriting signedPkg where we shouldn't (see part 2).

Comment 36

[Henry Chang [:hchang]]

Since this bug needs to wait for Bug 1209162, I moved Part 1 and its test case in Part 4 to Bug 1210573 to land it first.

Comment 37

[Henry Chang [:hchang]]

(In reply to Bobby Holley (:bholley) from comment #35)
> (In reply to Henry Chang [:henry] from comment #34)
> > > The parts in LoadContext/DocShell/TabParent look fine, but you should get a
> > > Necko person to review the stuff on the network side.
> > > 
> > > Also, we still need bug 1209162 in order for our handling of signedPkg to be
> > > correct.
> > 
> > Thanks, Bobby :)
> > 
> > Regarding Bug 1209162, do you specifically mean the nsDocShell part? It
> > seems the only dependency of this bug.
> 
> I mean InheritFromDocToChildDocShell, InheritFromDocShellToChildDocShell,
> InheritFromDocShellToChannel, which prevent us fro accidentally inheriting
> signedPkg where we shouldn't (see part 2).

Got it! In this case, do you think Bug 1209162 should block this bug or I can still try to land this bug and wait for the fix from Bug 1209162? Thanks!

Comment 38

[Bobby Holley (:bholley)]

(In reply to Henry Chang [:henry] from comment #37)
> Got it! In this case, do you think Bug 1209162 should block this bug or I
> can still try to land this bug and wait for the fix from Bug 1209162? Thanks!

I think it's probably fine to land on it's own, assuming we add todo() testcases checking the cases where we shouldn't inherit signedPkg, and make sure we have a bug on file for those that blocks shipping NSEC.

Though it also depends on what the necko reviewer thinks.

Comment 39

[Henry Chang [:hchang]]

(In reply to Bobby Holley (:bholley) from comment #38)
> (In reply to Henry Chang [:henry] from comment #37)
> > Got it! In this case, do you think Bug 1209162 should block this bug or I
> > can still try to land this bug and wait for the fix from Bug 1209162? Thanks!
> 
> I think it's probably fine to land on it's own, assuming we add todo()
> testcases checking the cases where we shouldn't inherit signedPkg, and make
> sure we have a bug on file for those that blocks shipping NSEC.
> 
> Though it also depends on what the necko reviewer thinks.

I will still try to get all the patches reviewed and see how far from the landing of Bug 1209162 by then. Thanks!

Comment 40

[Henry Chang [:hchang]]

Hi Honza,

Would you be available to review the patch "Part 3" regarding the necko part? We need to pass the package-identifier along the OnStartSignedPackagedRequest callbacks all the way to HttpChannelParent, IPC to HttpChannelChild and store into the loadcontext (nsDocShell). The package-id would be also stored to the loadcontext in the parent side.

Thanks!

Comment 41

[Honza Bambas (:mayhemer)]

Definitely not this week.  Is it OK to get reviews on Monday?

Comment 42

[Honza Bambas (:mayhemer)]

Grr.r... comment 41 still applies.

Comment 43

[Henry Chang [:hchang]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #41)
> Definitely not this week.  Is it OK to get reviews on Monday?

Sure! Thank you Honza :)

Comment 44

[Honza Bambas (:mayhemer)]

Comment on attachment 8670726 [details] [diff] [review]
Part 3: Populate packageId to child-side origin attribute

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

Something smells wrong here.  Updating loadcontext's origin attributes from arbitrary http channels doesn't seem right.  The place where we would be doing it might rather be somewhere in docshell/loadgroup/documentloadinfo directly.  the approach in this patch simply seems to me too racy and - wrong.


Some questions need to be answered first.

::: docshell/base/nsDocShell.cpp
@@ +13052,5 @@
>  
> +NS_IMETHODIMP
> +nsDocShell::SetPackageId(const nsACString& aPackageId)
> +{
> +  mPackageId = aPackageId;

can this id contain any of dom::quota::QuotaManager::kReplaceChars?  it must be ensured that it doesn't.

who constructs the id?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1134,5 @@
> +    }
> +    packageId = NS_ConvertUTF16toUTF8(attr.mSignedPkg);
> +  }
> +
> +  LOG(("HttpChannelParent::OnStartRequest: packageId: %s", packageId.get()));

log only when non-empty

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5670,5 @@
>  
>  NS_IMETHODIMP
>  nsHttpChannel::OnStartSignedPackageRequest(const nsACString& aPackageId)
>  {
> +    mIsSignedContent = true;

for which channel is this called?  Is it the package loading channel?  or a subresource loading channel?  or just the top-level-document-subresource loading channel?  it seems to my like all channels loading from a signed package gets this called.

after looking more closely to the PackagedAppService code, this apparently is called between AsyncOpen and OnCacheEntryCheck/Available calls.  seems ensured that it's always that way, right?

modifying origin attribs of the load context (shared by the whole load group, right?) changes the cache context.  so, after you do this change on the load context, channels in this load group (not loading from the package) will, after that, open entries from a different cache jar.  not sure this is what we should do and you want.  this is also very time-sensitive, hence all the questions above.  simply, modifying the loadcontext here seems pretty strange to me.

@@ +5708,5 @@
> +    if (!mIsSignedContent) {
> +        nsCOMPtr<nsILoadContext> loadContext;
> +        NS_QueryNotificationCallbacks(this, loadContext);
> +        if (loadContext) {
> +            loadContext->SetPackageId(EmptyCString());

to confirm, the order of calls here is:

channel->AsyncOpen
channel->OnStartSignedPackageRequest (under a conditions that need to clear first for me)
channel->OnCacheEntryAvailable -> ReadFromCache (where the entry is open with load context info NOT having the package id in the origin attributes string!)
channel->OnStartRequest (called when data starts to arrive, in this case from the cache)


apparently, any channel that doesn't have the mIsSignedContent flag set can manipulate the package id (drop it off) on the shared loadcontext at any time.  are you sure this is the right approach?

or I just may not understand what you are trying to achieve here.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +520,5 @@
>      // If non-null, warnings should be reported to this object.
>      HttpChannelSecurityWarningReporter* mWarningReporter;
>  
> +    // Indicate that if this channel is associated with a signed content.
> +    bool mIsSignedContent;

put this to the already existing set of bit fields please.

::: uriloader/prefetch/OfflineCacheUpdateParent.cpp
@@ +286,5 @@
>  
> +NS_IMETHODIMP
> +OfflineCacheUpdateParent::SetPackageId(const nsACString& aPackageId)
> +{
> +    mOriginAttributes.mSignedPkg = NS_ConvertUTF8toUTF16(aPackageId);

note: mOriginAttrubutes in this class will be replaced with mLoadingPrincipal (nsIPrincipal), bug 1165256

how should that be implemented then?  what is the meaning of changing it here, in this class anyway?  I think OfflineCacheUpdateParent::SetPackageId should never be called.

Comment 45

[Henry Chang [:hchang]]

Hi Honza,

Thanks for the review first :)

Please see my reply below:

(In reply to Honza Bambas (:mayhemer) from comment #44)
> Comment on attachment 8670726 [details] [diff] [review]
> Part 3: Populate packageId to child-side origin attribute
> 
> Review of attachment 8670726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Something smells wrong here.  Updating loadcontext's origin attributes from
> arbitrary http channels doesn't seem right.  The place where we would be
> doing it might rather be somewhere in docshell/loadgroup/documentloadinfo
> directly.  the approach in this patch simply seems to me too racy and -
> wrong.
> 
> 
> Some questions need to be answered first.
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +13052,5 @@
> >  
> > +NS_IMETHODIMP
> > +nsDocShell::SetPackageId(const nsACString& aPackageId)
> > +{
> > +  mPackageId = aPackageId;
> 
> can this id contain any of dom::quota::QuotaManager::kReplaceChars?  it must
> be ensured that it doesn't.
> 

The package id is supposed to be an UUID embedded in the manifest of a signed package.

> who constructs the id?
> 

Supposed to be generated by market place or the packaging/signing tool.

> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +5670,5 @@
> >  
> >  NS_IMETHODIMP
> >  nsHttpChannel::OnStartSignedPackageRequest(const nsACString& aPackageId)
> >  {
> > +    mIsSignedContent = true;
> 
> for which channel is this called?  Is it the package loading channel?  or a
> subresource loading channel?  or just the top-level-document-subresource
> loading channel?  it seems to my like all channels loading from a signed
> package gets this called.
> 
> after looking more closely to the PackagedAppService code, this apparently
> is called between AsyncOpen and OnCacheEntryCheck/Available calls.  seems
> ensured that it's always that way, right?
> 

Yes! The purpose of this callback is a signalling path to notify TabParent that it should reload the content from a new process. Here we just make use of this signalling path to populate the package id.

So, the channel which is called here is the one we use to download the package (e.g. https://foo.com/app.pak), which is the internal channel.

> modifying origin attribs of the load context (shared by the whole load
> group, right?) changes the cache context.  so, after you do this change on
> the load context, channels in this load group (not loading from the package)
> will, after that, open entries from a different cache jar.  not sure this is
> what we should do and you want.  this is also very time-sensitive, hence all
> the questions above.  simply, modifying the loadcontext here seems pretty
> strange to me.
> 

We seemed to discuss this issue in the work week in mountain view. What Jonas proposed is we should exlude the package from the cache key. But I might need to confirm this with him again. Does the approach make sense to you?

> @@ +5708,5 @@
> > +    if (!mIsSignedContent) {
> > +        nsCOMPtr<nsILoadContext> loadContext;
> > +        NS_QueryNotificationCallbacks(this, loadContext);
> > +        if (loadContext) {
> > +            loadContext->SetPackageId(EmptyCString());
> 
> to confirm, the order of calls here is:
> 
> channel->AsyncOpen
> channel->OnStartSignedPackageRequest (under a conditions that need to clear
> first for me)

If the package is not cached, the condition is the package has a valid signature.
If the package is cached, the condition is the cache file meta data has the information that it's signed. The current information is "signed-pak-origin" but will be changed to "signed-pak-id" in this bug.

> channel->OnCacheEntryAvailable -> ReadFromCache (where the entry is open
> with load context info NOT having the package id in the origin attributes
> string!)
> channel->OnStartRequest (called when data starts to arrive, in this case
> from the cache)
> 
> 
> apparently, any channel that doesn't have the mIsSignedContent flag set can
> manipulate the package id (drop it off) on the shared loadcontext at any
> time.  are you sure this is the right approach?
> 
> or I just may not understand what you are trying to achieve here.
> 

The flag "mIsSignedContent" is to solve an issue (introduced by this bug) which should be solved by Bug 1214572.
The issue we introduce is once we update the LoadContext in child side (the LoadContext of HttpChannelChild is actually nsDocShell), it will remain the same value even we navigate to other page. So, if we navigate to a signed packaged content and the child side loadContext's packageId is updated, then, we go to other page like google.com, the loadContext of this load (google.com) will contain the packageId, which is incorrect. So the flag "mIsSignedContent" is used to restore the loadContext::packageId.

However, in new security model, not only "to" a signed packaged content but also "from" a signed packaged content should we switch the loading process. So once Bug 1214572 is resolved, the flag "mIsSignedContent" can be removed then.


> ::: netwerk/protocol/http/nsHttpChannel.h
> @@ +520,5 @@
> >      // If non-null, warnings should be reported to this object.
> >      HttpChannelSecurityWarningReporter* mWarningReporter;
> >  
> > +    // Indicate that if this channel is associated with a signed content.
> > +    bool mIsSignedContent;
> 
> put this to the already existing set of bit fields please.
> 
> ::: uriloader/prefetch/OfflineCacheUpdateParent.cpp
> @@ +286,5 @@
> >  
> > +NS_IMETHODIMP
> > +OfflineCacheUpdateParent::SetPackageId(const nsACString& aPackageId)
> > +{
> > +    mOriginAttributes.mSignedPkg = NS_ConvertUTF8toUTF16(aPackageId);
> 
> note: mOriginAttrubutes in this class will be replaced with
> mLoadingPrincipal (nsIPrincipal), bug 1165256
> 
> how should that be implemented then?  what is the meaning of changing it
> here, in this class anyway?  I think OfflineCacheUpdateParent::SetPackageId
> should never be called.

In that case, I can let it be a stub function. The only reason I add this function is it implements nsILoadContext. 

-----------

To sum up, the purpose of this patch is to populate the package identifier (extracted from the signed package manifest) to the child side. The destination is nsDocShell, which is the loadContext of HttpChannelChild. Since the "nodePrincipal" will be created based on the channel's loadContext [1], I choose to store the packageId in the loadContext. Actually I have considered another more direct approach, which is to attach the packageId to the channel and change the way we create nodePrincipal in [1]. (Maybe need a function like nsIScriptSecurityManager::getLoadContextCodebasePrincipalForSignedPackage).

So, it seems to me that "should the cache key contain packageId" is still open for discussion. Other than that, what else could I answer to make the approach more clear :)

Thanks!


[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2381

Comment 46

[Henry Chang [:hchang]]

> 
> Yes! The purpose of this callback is a signalling path to notify TabParent
> that it should reload the content from a new process. Here we just make use
> of this signalling path to populate the package id.
> 
> So, the channel which is called here is the one we use to download the
> package (e.g. https://foo.com/app.pak), which is the internal channel.
> 

Well, I was totally wrong here. Sorry. The channel to get the OnStartSignedPackageRequest is the outer channel, which is the channel used to get "https://foo.com/app.pak!//index.html". The internal channel used to download the package will never get this callback.

Comment 47

[Henry Chang [:hchang]]

For the other approach (put the packageId in the channel), we also have to modify |NS_GetOriginAttributes| which is used in many places to get the originAttributes from a channel. The advantage of storing PackageId into LoadContext is that we can naturally have packageId in |NS_GetOriginAttributes| without changing the logic. |NS_GetOriginAttributes| will be used to generate the cookie key and packageId has to be the cookie key to split signed packages to different cookie jar.

Comment 48

[Mahendra Potharaju [:mahe]]

Similar to 1180091, 1180085. Need not block 2.5. Removing blocking flag

Comment 49

[Henry Chang [:hchang]]

Comment on attachment 8670721 [details] [diff] [review]
Part 1: Add attr 'packageIdentifier' to nsIPackagedAppUtils

Obsolete cuz it's been moved to another bug and landed.

Comment 50

[Henry Chang [:hchang]]

Comment on attachment 8670723 [details] [diff] [review]
Part 2: Call OnStartSignedPackageRequest with packageIdentifier

Obsolete cuz it's been moved to another bug and landed.

Comment 51

[Henry Chang [:hchang]]

Attachment: another approach

Comment 52

[Henry Chang [:hchang]]

Attachment: another approach

Comment 53

[Henry Chang [:hchang]]

Attachment: original approach.patch

This is the patch for my original approach except

1) Rebase to m-c
2) Remove the use around mIsSignedContent since Bug 1214572 will address it

Comment 54

[Honza Bambas (:mayhemer)]

Comment on attachment 8677966 [details] [diff] [review]
another approach

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

Nice!  definitely better than the first patch.  This isolates any changes to the attributes to the right places.  We can think of something even better later.  I was thinking of doing any update in nsDocumentLoadInfo or around.  But this seems good to me as well, if the updated principal is passed everywhere needed.

::: dom/base/nsDocument.cpp
@@ +2388,5 @@
> +        internal->GetPackageId(packageId);
> +      }
> +      if (!packageId.IsEmpty()) {
> +        mozilla::OriginAttributes attr =
> +          BasePrincipal::Cast(principal)->OriginAttributesRef();;

;;

::: netwerk/base/nsNetUtil.cpp
@@ +1243,5 @@
> +
> +    nsCOMPtr<nsIHttpChannelInternal> internal = do_QueryInterface(aChannel);
> +    if (!internal) {
> +      return true;
> +    }

nit: here rather if (internal) { do the job; }  return true;

so, this approach actually prevents random modifications of loadcontext and rather just updates the attributes with data extracted from an involved channel, on demand.  I like it.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +486,5 @@
>    // gHttpHandler->OnExamineResponse(this);
>  
>    mTracingEnabled = false;
>  
> +  // Store packageIdentifier.

This comment says nothing.  Please rather add a comment that this member is exposed on nsIHttpChannelInternal.packageId and used to update the principal.  And also that it's OK to fill it in OnStartRequest since it's not used earlier? (if that is true!)

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1135,5 @@
>    }
>  
> +  nsCString packageId;
> +  mChannel->GetPackageId(packageId);
> +  LOG(("HttpChannelParent::OnStartRequest: packageId: %s", packageId.get()));

log only when non-empty please.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5637,5 @@
>  NS_IMETHODIMP
>  nsHttpChannel::OnStartSignedPackageRequest(const nsACString& aPackageId)
>  {
> +    // Update the packageId of this channel.
> +    mPackageId = aPackageId;

could we assert here this is called before OnStartRequest has been called on the listener?  (maybe in a different bug, tho)

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +266,5 @@
>      readonly attribute nsIURI proxyURI;
>  
> +    /*
> +     * The package id for signed package when the channel is used to load
> +     * signed pacakge; would be empty otherwise.

I think more correctly this should be:

"The packageId of a signed package this channel is loading a resource from."

@@ +268,5 @@
> +    /*
> +     * The package id for signed package when the channel is used to load
> +     * signed pacakge; would be empty otherwise.
> +     */
> +    attribute ACString packageId;

should be infallible?

::: netwerk/test/mochitests/test_signed_web_packaged_app_origin.html
@@ +5,5 @@
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +</head>
> +<body>
> +

describe what the test does (what are the steps, what is the purpose, what is the main check the functionality works)

Comment 55

[Honza Bambas (:mayhemer)]

I think the feedback is the answer to ni.  If not, ask again please.

Comment 56

[Henry Chang [:hchang]]

(In reply to Honza Bambas (:mayhemer) from comment #55)
> I think the feedback is the answer to ni.  If not, ask again please.

Thanks Honza! I'll attach another patch based on your comments and ask for a formal review. Thanks :)

Comment 57

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb8024c1bf41

Comment 58

[Henry Chang [:hchang]]

Attachment: Bug1178526.patch

Comment 59

[Henry Chang [:hchang]]

Attachment: Patch for review

Comment 60

[Henry Chang [:hchang]]

Comment on attachment 8679369 [details] [diff] [review]
Patch for review

Hi Bobby, Honza,

Could you have another review for my new approach? The intention is the same but the approach is changed to keep LoadContext unchanged and store the packageId in the nsIHttpChannelInternal. When we need to populate the packageId (e.g. in nsDocument/nsScriptSecurityManager/NS_GetOriginAttribute), we grab it from nsIChannel and fill in the originAtttibutes.

Since the changes include necko as well as security, we might require Honza to review the necko part and Bobby to review the rest.

Thanks!

Comment 61

[Bobby Holley (:bholley)]

Comment on attachment 8679369 [details] [diff] [review]
Patch for review

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

::: caps/nsScriptSecurityManager.cpp
@@ +406,5 @@
> +      nsCOMPtr<nsIPrincipal> prin;
> +      rv = GetLoadContextCodebasePrincipal(uri, loadContext, getter_AddRefs(prin));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      // Re-create a principal if the channel is associated with a signed package.

The placement of this doesn't make sense to me. Why would we first call GetLoadContextSubjectPrincipal if we're just going to overwrite |prin| with the principal that we create below? It seems like the order here should be reversed, and that none of the the new code should be in the |if (loadContext) {| branch.

More importantly though, I'm concerned that GetChannelURIPrincipal is not supposed to be called in the common case (since consumers are supposed to use GetChannelResultPrincipal), and currently GetChannelURIPrincipal doesn't properly handle OriginAttributes at all (see bug 1211590). If we do anything in this method, we should fix that bug first.

In reality, it may make more sense to fix bug 1209162 before doing any of this work.

Comment 62

[Henry Chang [:hchang]]

Hi Bobby,

Thanks for the review first :)

Could we have a chat on either IRC or anywhere else to discuss what we should do at the moment? Ideally I wish to fix this bug before this week if we can come up with some solution.

The specific purpose of this bug is to populate the packageId to some places so that the cookie jar key and the data jar keys could be isolated by packageId. From what I know, we at least need

1) |NS_GetOriginAttributes| to return an originAttributes with packageId (used to generate cookie key)
2) content nodePrincipal has the origin attributes which has packageId (used to generate system message key)
3) Maybe nsIScriptSecurityManager::GetChannel*Principal. (maybe used to key the data jar)

My previous approach is to "directly" update the loadContext (which is nsDocShell on child side). It seems improper because mutating loadContext is dangerous (pointed by Honza). So I changed to not update loadContext but take packageId into account case by case. That's why I have to modify a couple of places to extract the packageId from the channel and fill in the originAttribute or principal.

If it's not a good time to change nsIScriptSecurityManager::GetChannel*Principal, what if I alternatively examine every consumer of this functions which is used to generate the data jar key?

Thanks!


(In reply to Bobby Holley (:bholley) from comment #61)
> Comment on attachment 8679369 [details] [diff] [review]
> Patch for review
> 
> Review of attachment 8679369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: caps/nsScriptSecurityManager.cpp
> @@ +406,5 @@
> > +      nsCOMPtr<nsIPrincipal> prin;
> > +      rv = GetLoadContextCodebasePrincipal(uri, loadContext, getter_AddRefs(prin));
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      // Re-create a principal if the channel is associated with a signed package.
> 
> The placement of this doesn't make sense to me. Why would we first call
> GetLoadContextSubjectPrincipal if we're just going to overwrite |prin| with
> the principal that we create below? It seems like the order here should be
> reversed, and that none of the the new code should be in the |if
> (loadContext) {| branch.
> 
> More importantly though, I'm concerned that GetChannelURIPrincipal is not
> supposed to be called in the common case (since consumers are supposed to
> use GetChannelResultPrincipal), and currently GetChannelURIPrincipal doesn't
> properly handle OriginAttributes at all (see bug 1211590). If we do anything
> in this method, we should fix that bug first.
> 
> In reality, it may make more sense to fix bug 1209162 before doing any of
> this work.

Comment 63

[Henry Chang [:hchang]]

Just studied the indexedDB API case. The key is derived from principal [1], which was set in [2]. I would guess localStorage and other data jar WebAPI follows the same path. So as long as we generate nodePrincipal with packageId, data jar will be isolated by different signed package. nsIScriptSecurityManager::GetChannel*Principal seems not required.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBFactory.cpp#190
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2541

Comment 64

[Henry Chang [:hchang]]

Attachment: approach 3 (create docshell with packageId)

Comment 65

[Henry Chang [:hchang]]

Sorry Bobby, I got to sleep. I found another approach which barely changes necko code, which is to create docShell with packageId taken into account. Since the signed package has its own process/tabContext/docshell, it makes sense to create its own docShell and initialize with packageId from tabContext (just like how we init its appId/isBrowserElement). Since we are not randomly changing docShell/loadContext, it should be okay regarding Honza's concern.

Besides, in nsDocShell::GetOriginAttributes, for now just unconditionally populate the packageId. I leave a comment to point out there is a bug to fix the inheritance issue.

This approach is the best one to me so far so I decide to ask for a review for the patch.

Honza, I might also need your review regarding the change in |NeckoParent::GetValidatedAppInfo|. We need to consider the packageId in that function as well.

Comment 66

[Bobby Holley (:bholley)]

Hey Henry,

Sorry I wasn't around this morning. I'll try to be around IRC this evening to get this sorted out.

Conceptually, sticking this on the DocShell is wrong, because it's a property of the document rather than a property of the browsing context (in html5 parlance). I understand that this hack works in the current setup because we always recreate the docshell when navigating to/from signed content in order to move it between processes (I'm a bit fuzzy on this part). However, it's still pretty confusing conceptually, and relies on the process-switching stuff in order to be correct.

The correct thing to do is to put the signedPkg on the channel and derive it when creating principals from a channel. My objection in comment 61 is that GetChannelURIPrincipal is not yet OriginAttributes-aware, and needs some refactoring first (in bug 1211590). In short, we need to refactor GetChannelResultPrincipal and GetChannelURIPrincipal, and split out an appropriate helper method which creates the actual principal (rather than having GetChannelResultPrincipal call GetChannelURIPrincipal). This helper method can accept an OriginAttributes from the caller, and then add the signedPkg from the channel appropriately.

But really, this will all be a heck of a lot less confusing once we get bug 1209162 landed, since that is the bug that defines all of the relationships by which OriginAttributes flow between data structures. Yoshi is working on that bug now I think.

So in summary:
* The Right Way to do this is to fix bug 1209162 first.
* The less-elegant-but-mostly-right approach is to refactor GetChannelURIPrincipal and GetChannelResultPrincipal a bit, and then do what your original patch here does.
* The super-hacky way is to do it on the docshell, as your current patch does.

The precise approach we take here depends a lot on how much of a hurry we're in. If we do the super-hacky approach, I think we should loop in KanRu (or whoever's working on the docshell process-switching), and possibly sicking as well.

Comment 67

[Honza Bambas (:mayhemer)]

(In reply to Henry Chang [:henry] from comment #65)
> Honza, I might also need your review regarding the change in
> |NeckoParent::GetValidatedAppInfo|. We need to consider the packageId in
> that function as well.

Then ask for it.

Comment 68

[Henry Chang [:hchang]]

Hey,

I just got up and will be around IRC from now.

(In reply to Bobby Holley (:bholley) from comment #66)
> Hey Henry,
> 
> Sorry I wasn't around this morning. I'll try to be around IRC this evening
> to get this sorted out.
> 
> Conceptually, sticking this on the DocShell is wrong, because it's a
> property of the document rather than a property of the browsing context (in
> html5 parlance). I understand that this hack works in the current setup
> because we always recreate the docshell when navigating to/from signed
> content in order to move it between processes (I'm a bit fuzzy on this
> part). However, it's still pretty confusing conceptually, and relies on the
> process-switching stuff in order to be correct.
>

Got it. I think what misled me is docShell already has appId in it. Does that mean appId should be also removed from docShell one day?

> The correct thing to do is to put the signedPkg on the channel and derive it
> when creating principals from a channel. My objection in comment 61 is that
> GetChannelURIPrincipal is not yet OriginAttributes-aware, and needs some
> refactoring first (in bug 1211590). In short, we need to refactor
> GetChannelResultPrincipal and GetChannelURIPrincipal, and split out an
> appropriate helper method which creates the actual principal (rather than
> having GetChannelResultPrincipal call GetChannelURIPrincipal). This helper
> method can accept an OriginAttributes from the caller, and then add the
> signedPkg from the channel appropriately.
> 
> But really, this will all be a heck of a lot less confusing once we get bug
> 1209162 landed, since that is the bug that defines all of the relationships
> by which OriginAttributes flow between data structures. Yoshi is working on
> that bug now I think.
> 
> So in summary:
> * The Right Way to do this is to fix bug 1209162 first.
> * The less-elegant-but-mostly-right approach is to refactor
> GetChannelURIPrincipal and GetChannelResultPrincipal a bit, and then do what
> your original patch here does.
> * The super-hacky way is to do it on the docshell, as your current patch
> does.
> 
> The precise approach we take here depends a lot on how much of a hurry we're
> in.

I cannot decide this since it's not blocking any release or something.
 
> If we do the super-hacky approach, I think we should loop in KanRu (or
> whoever's working on the docshell process-switching), and possibly sicking
> as well.

Kanru implemented the process switch API in nsFrameLoader and I am the one who used the API to carry out the process switch feature.

I'll ask Kanru/Paul/Aaron (or whoever awake :p) lately to decide the what we should do right now.

Hope to See you on IRC :)

Comment 69

[Henry Chang [:hchang]]

Attachment: Refined approach 2

Comment 70

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So, without really understanding what the patch does yet (I've just skimmed it), I would say that if it makes things work well enough that we are passing more tests, then we should land it. There's too many moving parts still and this stuff is complicated enough that I don't think any of us have a clear picture of what the real solution will look like. So we need to get there iteratively.

That said, I agree that we need to get bug 1209162 landed as soon as possible. I will stay more on top of reviews and needinfo's there. This might be true for bug 1211590 as well, I need to read up on that bug.

I also suspect that we need to store information in the docshell, but maybe there's some way to avoid it. Here's one case to consider:

+-------------------------------------------+
|    Outermost window <A>                   |
| +----------------------------------------+|
| |    Iframe to randomWebsite.com <B>     ||
| |  +------------------------------------+||
| |  |   Iframe to signed package <C>     |||
| |  |                                    |||
| |  +------------------------------------+||
| +----------------------------------------+|
+-------------------------------------------+

In this case, the document in <A> should have mSignedPkg set to the identifier of the package obviously.

But the document in <B> should not have mSignedPkg set. This because we want the page in <B> to use the same cookies and IndexedDB data as if the user had browsed directly to randomWebsite.com. I.e. it should have the "normal" OriginAttributes so that it gets the "normal" cookiejar.

What might be even more non-obvious is that the page in <C> should not have mSignedPkg set. Even though it is loaded from the same signed package as <A>. The reason for this is that we don't want to enable the page in <C> to get clickjacked.

There's a few ways we can accomplish this.

We could either emulate something like x-frame-options automatically for pages in a signed package. And force those pages to only be iframe-able if the parent page has the correct origin (including OriginAttributes)

Another approach is that we ensure that when the docshell for <C> gets created, it gets an OriginAttributes where mSignedPkg is empty.


The advantage of the latter approach is that I think we'll generally want docshells to have an OriginAttributes. That is needed for things like the "browser context" stuff that the desktop privacy team is working on. Where you can indicate that a given tab should use the "work cookie jar" and a separate tab should use "home cookie jar".

Here it seems like a good solution to have the docshells and documents in the "work cookie jar" tab use an OriginAttributes where mUserContextId is, say, 1. This mUserContextId would be inherited down from topmost docshell, to contained document, to docshell in iframe, etc.

Once we have that inheritance in place, the only thing that we'd need to change to get the mSignedPkg inheritance working correctly, is that we would only inherit mSignedPkg from docshell to document if the loaded document comes from the correct package. Everything else would work as normal.

And yes, this relies on the fact that we switch process when navigating between signed packages. But I think that's fine. It means that things are similar to that you need to switch tab when going from "work cookie jar" to "home cookie jar".

Let me know what you think.


What all this means is that I think we should give docshells an OriginAttributes. And that this should be inherited into the principal of the document. But in that inheritance we need to write a special rule where mSignedPkg is only inherited if the navigated to URL matches that of the package.

Comment 71

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: OriginAttributes_in_iframes.JPG

Comment 72

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8680101 [details] [diff] [review]
approach 3 (create docshell with packageId)

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

I like this patch more than "approach 2"

Comment 73

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jonas Sicking (:sicking) from comment #70)
> 
> +-------------------------------------------+
> |    Outermost window <A>                   |
> | +----------------------------------------+|
> | |    Iframe to randomWebsite.com <B>     ||
> | |  +------------------------------------+||
> | |  |   Iframe to signed package <C>     |||
> | |  |                                    |||
> | |  +------------------------------------+||
> | +----------------------------------------+|
> +-------------------------------------------+
> 
> 
> What might be even more non-obvious is that the page in <C> should not have
> mSignedPkg set. Even though it is loaded from the same signed package as
> <A>. The reason for this is that we don't want to enable the page in <C> to
> get clickjacked.
> 
Short question, check the image from Comment 71, it is what we discussed in Moutain View.

<C> should be mapped to the black iframe (with {A:5, B:T, S:'Foo'}) from that image, right?

In MTV we discussed that black iframe should inherit the mSignedPkg, but you decide *not* to inherit it now?

Comment 74

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There is no frame similar to <C> in the image in comment 71.

<B> is to the purple iframe inside the signed-package frame. I.e. the { A:5, B:t, S:'' } inside the { A:5, B:t, S:'foo' }.

<C> would be an iframe *inside* that purple iframe.

Comment 75

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

<C> should either get { A:5, B:t S:'' }, or it should get blocked from being loaded. Either is fine.

Comment 76

[Henry Chang [:hchang]]

Comment on attachment 8680101 [details] [diff] [review]
approach 3 (create docshell with packageId)

Based on comment 72, asking for a review to Jonas.

Comment 77

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8680101 [details] [diff] [review]
approach 3 (create docshell with packageId)

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

Let me know if you prefer to do any of this as a followup instead.

::: netwerk/ipc/NeckoParent.cpp
@@ +144,5 @@
>          continue;
>        }
>      }
>      aAttrs = OriginAttributes(appId, inBrowserElement);
> +    aAttrs.mSignedPkg = tabContext.OriginAttributesRef().mSignedPkg;

This doesn't look correct. You should get a signedpkg from aSerialized and if it's non-empty, then you should compare it to tabContext.OriginAttributesRef().mSignedPkg. If they are not the same, then you should return an error.

Which also means that you should add a signedpkg getter on loadcontext.

We should also add a similar check to this function: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#241

Comment 78

[Henry Chang [:hchang]]

Thanks Jonas :)

The issue you pointed out is indeed what I totally couldn't figure out. The only reason I change it is I found the mSignedPkg isn't populated to the parent process. I am still trying to understand what |NeckoParent::GetValidatedAppInfo| is intended to do but could you tell me any insight of that function?

Thanks!


(In reply to Jonas Sicking (:sicking) from comment #77)
> Comment on attachment 8680101 [details] [diff] [review]
> approach 3 (create docshell with packageId)
> 
> Review of attachment 8680101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let me know if you prefer to do any of this as a followup instead.
> 
> ::: netwerk/ipc/NeckoParent.cpp
> @@ +144,5 @@
> >          continue;
> >        }
> >      }
> >      aAttrs = OriginAttributes(appId, inBrowserElement);
> > +    aAttrs.mSignedPkg = tabContext.OriginAttributesRef().mSignedPkg;
> 
> This doesn't look correct. You should get a signedpkg from aSerialized and
> if it's non-empty, then you should compare it to
> tabContext.OriginAttributesRef().mSignedPkg. If they are not the same, then
> you should return an error.
> 
> Which also means that you should add a signedpkg getter on loadcontext.
> 
> We should also add a similar check to this function:
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.
> cpp#241

Comment 79

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So, based on a quick look, what I *think* that NeckoParent::GetValidatedAppInfo does is this:

When necko receives a request to do a http request, it needs to first add cookies that request before sending anything to the network.

These cookies are added by the httpchannel implementation itself, and this is done in the parent process. I.e. the child process creates a httpchannel and opens it. The httpchannel implementation sends a message to the parent process. The parent process checks in the cache, and if the request is not there, adds cookies and sends the request to the network.

However it is important that if app A does a network request, that it can't trick the parent process into adding the cookies from app B to the request.

That's where NeckoParent::GetValidatedAppInfo comes it. It checks that the appid/browserflag/originattributes that the child claimed that the request is part of, can actually be correct information and is not a "lie".

We do want the child process to send the appid/browserflag/originattributes along with the request. I.e. we don't want to simply grab that information from the TabContext. The reason for this is that sometimes several different appid/browserflag/originattributes might run in a single child process.

For example, an app which has access to the browser API will run content both with browserflag=true and browserflag=false in the same child process.

And in the new streaming packages, a signed package might open an in-process iframe to unsigned content, which means that the same process will run content with both mSignedPkg=<some identifier> and with mSignedPkg="".

Hence we have the child send the appid/browserflag/originattributes to the parent, but the parent checks that it's accurate information.

NeckoParent::GetValidatedAppInfo handles this for cookies added to network requests. AppProcessChecker.cpp handles this for API access.

Comment 80

[Henry Chang [:hchang]]

Cool! It's pretty clear to me after the explanation!
ni to just let you know that I'd like to follow your suggestion to get the patch landed!
Thanks!

(In reply to Jonas Sicking (:sicking) from comment #79)
> So, based on a quick look, what I *think* that
> NeckoParent::GetValidatedAppInfo does is this:
> 
> When necko receives a request to do a http request, it needs to first add
> cookies that request before sending anything to the network.
> 
> These cookies are added by the httpchannel implementation itself, and this
> is done in the parent process. I.e. the child process creates a httpchannel
> and opens it. The httpchannel implementation sends a message to the parent
> process. The parent process checks in the cache, and if the request is not
> there, adds cookies and sends the request to the network.
> 
> However it is important that if app A does a network request, that it can't
> trick the parent process into adding the cookies from app B to the request.
> 
> That's where NeckoParent::GetValidatedAppInfo comes it. It checks that the
> appid/browserflag/originattributes that the child claimed that the request
> is part of, can actually be correct information and is not a "lie".
> 
> We do want the child process to send the appid/browserflag/originattributes
> along with the request. I.e. we don't want to simply grab that information
> from the TabContext. The reason for this is that sometimes several different
> appid/browserflag/originattributes might run in a single child process.
> 
> For example, an app which has access to the browser API will run content
> both with browserflag=true and browserflag=false in the same child process.
> 
> And in the new streaming packages, a signed package might open an in-process
> iframe to unsigned content, which means that the same process will run
> content with both mSignedPkg=<some identifier> and with mSignedPkg="".
> 
> Hence we have the child send the appid/browserflag/originattributes to the
> parent, but the parent checks that it's accurate information.
> 
> NeckoParent::GetValidatedAppInfo handles this for cookies added to network
> requests. AppProcessChecker.cpp handles this for API access.

Comment 81

[Henry Chang [:hchang]]

BTW, I just found the bug to bring in the relevant code: https://bugzilla.mozilla.org/show_bug.cgi?id=782542 for "Secure necko IPDL usage".

Comment 82

[Henry Chang [:hchang]]

Attachment: approach 3 (create docshell with packageId)

Comment 83

[Henry Chang [:hchang]]

Comment on attachment 8685921 [details] [diff] [review]
approach 3 (create docshell with packageId)

Hi Jonas,

I've added signedPkg comparison between child and parent. Regarding the AppProcessChecker::AssertAppPrincipal, do you think we should add the check in this bug as well? I am not sure what the principal would be passed to that function. For example, maybe the NeworkOriginAttributes would be used to generate the principal and pass to that function. So, could we file a bug to remind the need of addressing the issue?

Thanks!

Comment 84

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8685921 [details] [diff] [review]
approach 3 (create docshell with packageId)

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

::: docshell/base/nsDocShell.cpp
@@ +13690,5 @@
>  
> +NS_IMETHODIMP
> +nsDocShell::SetIsSignedPackage(const nsAString& aPackageId)
> +{
> +  mPackageId = aPackageId;

Can we call this mSignedPkgId or some such? To make it clear that it's not about packages in general but about signed packages specifically.

Ideally we should just have an mOriginAttributes and this code would just do

mOriginAttributes.mSignedPkg = aSignedPkg;

But we can do that as a followup.

::: docshell/base/nsDocShell.h
@@ +996,5 @@
>    nsString GetInheritedPaymentRequestId();
>  
> +  // The packageId for a signed packaged iff this docShell is created
> +  // for a signed package.
> +  nsString mPackageId;

So change this to mSignedPkg or mSignedPkgId

::: netwerk/ipc/NeckoParent.cpp
@@ +144,5 @@
>          continue;
>        }
>      }
> +    if (aSerialized.mOriginAttributes.mSignedPkg != tabContext.OriginAttributesRef().mSignedPkg) {
> +      continue;

This should actually be:

if (!aSerialized.mOriginAttributes.mSignedPkg.IsEmpty() &&
    aSerialized.mOriginAttributes.mSignedPkg !=
      tabContext.OriginAttributesRef().mSignedPkg) {
  ...

Even a tab which is running signed content might create <iframe>s which run unsigned content.

I.e. any process can run unsigned content.

Comment 85

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> I've added signedPkg comparison between child and parent. Regarding the
> AppProcessChecker::AssertAppPrincipal, do you think we should add the check
> in this bug as well?

Either way. Up to you.

> I am not sure what the principal would be passed to
> that function. For example, maybe the NeworkOriginAttributes would be used
> to generate the principal and pass to that function. So, could we file a bug
> to remind the need of addressing the issue?

It's fine that necko is calling the functions that it is calling.

But there are already existing callsites of that function. As well as existing callsites of the other functions in that file. So we need to fix multiple functions in that file to make sure that they not only do security checks based on appid, but also based on mSignedPkg.

Since multiple functions there needs to be fixed, maybe better to do a followup.

Comment 86

[Henry Chang [:hchang]]

Thanks Jonas!

(In reply to Jonas Sicking (:sicking) from comment #84)
> Comment on attachment 8685921 [details] [diff] [review]
> approach 3 (create docshell with packageId)
> 
> Review of attachment 8685921 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: netwerk/ipc/NeckoParent.cpp
> @@ +144,5 @@
> >          continue;
> >        }
> >      }
> > +    if (aSerialized.mOriginAttributes.mSignedPkg != tabContext.OriginAttributesRef().mSignedPkg) {
> > +      continue;
> 
> This should actually be:
> 
> if (!aSerialized.mOriginAttributes.mSignedPkg.IsEmpty() &&
>     aSerialized.mOriginAttributes.mSignedPkg !=
>       tabContext.OriginAttributesRef().mSignedPkg) {
>   ...
> 
> Even a tab which is running signed content might create <iframe>s which run
> unsigned content.
> 
> I.e. any process can run unsigned content.

Got a little confused here: in what case would the result be different? My assumption is there must be a tabContext which has the same mSignedPkg as what the child claims no matter what mSignedPkg is. What if a child lies it's unsigned but IPC to a signed tab? (In that case the child becomes weaker than it's supposed to be.)

Comment 87

[Henry Chang [:hchang]]

Attachment: Patch for check in (r+'d, addressed reviewed comments)

Comment 88

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Consider the case when the user navigates to a signed page, and that page creates an iframe to an unsigned page, and that unsigned page does a network request.

In that case, tabContext.OriginAttributesRef().mSignedPkg will be equal to the signing identifier of the signed page, because that's what we're running in the tab. But aSerialized.mOriginAttributes.mSignedPkg will be empty.

This should be allowed.

Comment 89

[Henry Chang [:hchang]]

(In reply to Jonas Sicking (:sicking) from comment #88)
> Consider the case when the user navigates to a signed page, and that page
> creates an iframe to an unsigned page, and that unsigned page does a network
> request.
> 
> In that case, tabContext.OriginAttributesRef().mSignedPkg will be equal to
> the signing identifier of the signed page, because that's what we're running
> in the tab. But aSerialized.mOriginAttributes.mSignedPkg will be empty.
>

In that case, we will have two TabContext in [1]. The outer one has mSignedPkg=<some-identifier> and the inner one has mSignedPkg="". We are expected to see a TabContext with empty mSignedPkg. This is what I understand. Did I miss something around :(

Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp?from=neckoparent.cpp#124

Comment 90

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdfd4f4cd19d

Comment 91

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think we'll create a TabContext for the inner <iframe>, will we? I think we only create TabContexts for top-level "tabs" (and possibly also for out-of-process <iframe>s, but that's not the case here).

Check with KanRu, he'll know better than me.

However even in the case of a single signed page, if that page makes requests to outside of the package mSignedPkg will be empty for those requests. (Actually, once everything is in place, I think mSignedPkg will only be non-empty in functions that handle network requests, when the browser API is involved).

Comment 92

[Henry Chang [:hchang]]

(In reply to Jonas Sicking (:sicking) from comment #91)
> I don't think we'll create a TabContext for the inner <iframe>, will we? I
> think we only create TabContexts for top-level "tabs" (and possibly also for
> out-of-process <iframe>s, but that's not the case here).
> 
> Check with KanRu, he'll know better than me.
> 

According to my experiment result, you are right! non-top-level, non-remote and non-moz-browser <iframe> will not have a TabParent created.

Thank you Jonas!

Comment 93

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e96f64e2b9d9

Comment 94

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e96f64e2b9d9

Comment 95

[Ethan Tseng [:ethan]]

Thank you all for completing this significant bug!
A big step for NSec!