1178525 - Ensure the package is verified before content is served

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

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

For the new FxOS Security model we need a way to ensure that we have checked the signature of the package prior to content being served, and also the appropriate response if the signature verification fails.

Comment 1

[Henry Chang [:hchang]]

Following are some of my very initial thoughts on this task:

Steps:

1) Use URI to check if it’s a packaged content.

2) If it is a packaged content, instead of feeding content to all listeners, 
   fetch the header of thepackage first.

3) Since the package is streamable, we are supposed be able to seek to the 
   signature and verify it. It requires the header contains the offset of 
   each content and the server supports range request. (What if server doesn't
   support range request? We need to download the potentially huge package
   even we only want the navigate to the very plain front page?)

4) If not verified, report error.

5) If verified, seek to the content in the package and serve it.

Question:

I suppose the package is capable of being seeked to arbitrary resource. In this case, since we may not have the entire package downloaded, how do we check if certain cached packaged content is up-to-date? The solution might be using the package hash/checksum/version inside the stored header to do a conditional request.

Comment 2

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

I think we need to do this without relying on the server supporting range-requests.
The first time we load the package, we store the signatures in the metadata, and check each resource as we load them from the network. That is if we allow each resource in the package to be signed individually.
If we also want to check the signature for the entire package, we need to delay feeding content to the listeners, and at the end of the package, check the signature, then call all of the listeners.

When later loading a resource, we check the package hasn't changed, else we just serve the resource we already have, because it has already been validated.

Comment 3

[Henry Chang [:hchang]]

(In reply to Valentin Gosu [:valentin] from comment #2)
> I think we need to do this without relying on the server supporting
> range-requests.
> The first time we load the package, we store the signatures in the metadata,
> and check each resource as we load them from the network. That is if we
> allow each resource in the package to be signed individually.
> If we also want to check the signature for the entire package, we need to
> delay feeding content to the listeners, and at the end of the package, check
> the signature, then call all of the listeners.
> 
> When later loading a resource, we check the package hasn't changed, else we
> just serve the resource we already have, because it has already been
> validated.

I quickly scanned the formats in w3c package draft [1] and the one proposed by Paul [2],
none of them mentions the index/offset design. So, just forget about it :p

So, we have to download the entire package to get the signature and then the target resource.
(at least the first time). This is much clearer to me now. Thanks!

Since the package format and the URL we are going to use is still under discussion,
I'll just create some stub functions for the uncertain behavior as a start.

Comment 4

[Henry Chang [:hchang]]

Listing the subtasks for this bug:

1. Check the package header to see if it has signature.
   - Bug 1185436 will extract the header for use.

2. Use the signature to verify the manifest (may be app manifest or a separate one)
   - We need a function to load the signature and verify the manifest 
    (need to co-work with jho)
   
3. Parse the manifest and do the integrity check for each subresource.
   - We need to parse the manifest (Hope we already have such utility)
   - We need a function to check the integrity of subresources
     (Might need to co-work with jho, too)

Comment 5

[Jason Duell]

Does this bug also cover actually switching the channel to a new process if that needs to happen?  I suspect we need to open a new bug for that--it's a large code change and will probably need to be coded by different people.

Will the signature extraction & check happen in the parent, or the child?

I can think of two mechanisms off the top of my head that could be useful for storing content while we wait for the verification to happen: 

1) we could prefetch it into the HTTP cache.  This means we'll wind up doing some I/O to fetch it back (but with OS file system caches working the way they do, there's a very good chance that the file will be in RAM)

2) We have existing mechanisms for queuing IPDL messages in the child process.  We use those when we want to Divert a channel back to the parent (we use this for downloads, which we want to guarantee will complete even if the child that initiated them gets killed during the download).  This keeps all the channel payload in RAM, which is fast (but less flexible if the package is huge: would consume lots of memory).

My gut sense is that #1 may be better. The code that wants to verify the package is logically going to want to receive all the usual OnDataAvailable calls, so it can read the signature, right?  If we wind up needing to open the channel in a new process after that, it would be fairly simple to just have it open a  new channel and read the data from cache.

But we could make #2 work too (we could deliver OnData IPDL messages but keep copies of them, and then we'd have them available in the original child process, or could divert them to a new process as needed.).

Comment 6

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

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #5)
> Does this bug also cover actually switching the channel to a new process if
> that needs to happen?  I suspect we need to open a new bug for that--it's a
> large code change and will probably need to be coded by different people.
> 
> Will the signature extraction & check happen in the parent, or the child?

The signature extraction & check will happen in the parent.
 
> I can think of two mechanisms off the top of my head that could be useful
> for storing content while we wait for the verification to happen: 
> 
> 1) we could prefetch it into the HTTP cache.  This means we'll wind up doing
> some I/O to fetch it back (but with OS file system caches working the way
> they do, there's a very good chance that the file will be in RAM)
> 
> 2) We have existing mechanisms for queuing IPDL messages in the child
> process.  We use those when we want to Divert a channel back to the parent
> (we use this for downloads, which we want to guarantee will complete even if
> the child that initiated them gets killed during the download).  This keeps
> all the channel payload in RAM, which is fast (but less flexible if the
> package is huge: would consume lots of memory).
> 
> My gut sense is that #1 may be better. The code that wants to verify the
> package is logically going to want to receive all the usual OnDataAvailable
> calls, so it can read the signature, right?  If we wind up needing to open
> the channel in a new process after that, it would be fairly simple to just
> have it open a  new channel and read the data from cache.

That's also what I think. The new process should be able to open a new channel
and fetch from the cache. But what if the requested resource is still downloading
while the new process requests it? Does necko automatically serve partial content
from the cache and continue with data from network or wait until the content is
fully downloaded? For unsigned package the former is preferred; for signed package
the latter is a must.

> But we could make #2 work too (we could deliver OnData IPDL messages but
> keep copies of them, and then we'd have them available in the original child
> process, or could divert them to a new process as needed.).

Comment 7

[Henry Chang [:hchang]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #5)
> Does this bug also cover actually switching the channel to a new process if
> that needs to happen?  

It didn't in the first place. However, this bug may tightly couple to process
switch so we may have to take the process switch into account.

> I suspect we need to open a new bug for that--it's a
> large code change and will probably need to be coded by different people.
> 
> Will the signature extraction & check happen in the parent, or the child?
> 

Yes, it would happen in the parent. There will be a outer channel for a specific
resource like (http://foo.com/app.pak!//index.html) wrapping around a internal
channel for the package. (http://foo.com/app.pak).

> I can think of two mechanisms off the top of my head that could be useful
> for storing content while we wait for the verification to happen: 
> 
> 1) we could prefetch it into the HTTP cache.  This means we'll wind up doing
> some I/O to fetch it back (but with OS file system caches working the way
> they do, there's a very good chance that the file will be in RAM)
> 
> 2) We have existing mechanisms for queuing IPDL messages in the child
> process.  We use those when we want to Divert a channel back to the parent
> (we use this for downloads, which we want to guarantee will complete even if
> the child that initiated them gets killed during the download).  This keeps
> all the channel payload in RAM, which is fast (but less flexible if the
> package is huge: would consume lots of memory).
> 
> My gut sense is that #1 may be better. The code that wants to verify the
> package is logically going to want to receive all the usual OnDataAvailable
> calls, so it can read the signature, right?  If we wind up needing to open
> the channel in a new process after that, it would be fairly simple to just
> have it open a  new channel and read the data from cache.
> 

It's exact what we proposed :) 

The actual flow may depend on when cache becomes visible. I mean if the cache entry 
would  be found in the middle of caching. Imagine channel A is still caching to 
"http://foo.com/index.html" (no matter in memory or file) while channel B 
is trying to find the entry of "http://foo.com/index.html". I feel the entry
should be found by B so we must be very careful to buffer and delay-serve if it's
from a signed package even a resource is said in the cache.

I might need to have "DivertTo" a quick look to know if it meets our needs.

Thanks!

Comment 8

[Henry Chang [:hchang]]

Attachment: A WIPy WIP patch (very draft)

This is the very very initial patch which includes a part of Bug 1186290 because they have to test together.

Comment 9

[Henry Chang [:hchang]]

Attachment: A WIPy WIP patch (very draft, rebased)

Comment 10

[Henry Chang [:hchang]]

Attachment: Add OnStartSignedPackageRequest API (Bug 1186290)

Comment 11

[Henry Chang [:hchang]]

Attachment: Main patch

Comment 12

[Henry Chang [:hchang]]

Attachment: Main patch

Comment 13

[Henry Chang [:hchang]]

Attachment: PackagedAppVerifier (v1)

Comment 14

[Henry Chang [:hchang]]

Hi Valentin,

I just attached one patch for this bug and another patch for bug 1186290 (OnStartSignedPackageRequest support) along with a diagram describing the flow regarding signature verification. Can I get your very initial feedback? Thanks!

The main changes are:

1) Introduce PackagedAppVerifier to deal with manifest/resource verification. PackagedAppVerifier will delegate the verification tasks to certain XPCOM, which is being implemented in Bug 1178518, and notify its client (PackagedAppDownloader) |OnManifestVerified| and |OnResourceVerified|. Since Bug 1178518 is not finished yet, we just fire a fake success event to the main thread for testing purpose.

2) On every PackagedAppDownloader::OnStopRequest, we close the cache output stream and pass relevant information to the verifier (PackagedAppVerifier::ProcessResourceCache). The rest of work is to listen to OnManifestVerified and OnResourceVerified. |CallCallbacks| and |ClearCallbacks| will be called in OnManifestVerified/OnResourceVerified which would be asynchronously notified by PackagedAppVerifier.

3) When a manifest is still being verified, PackagedAppVerifier will queue the downloaded resource and verify it after the manifest is verified. This is transparent to PackagedAppDownloader so that it can just always pass the downloaded resource to the verifier and wait for |OnResourceVerified| to be notified.

4) While |OnManifestVerified|, if the packaged is signed, PackagedAppDownloader would notify all the requesters that a signed package is about to download. It doesn't mean the process switch is required. 'Someone' (most likely TabParent) will check the origin and decide if it needs to close the outer channel (not affect the inner package downloading channel) and re-make the request in a new process. (However, what may be happening for process switch is not the scope of this bug. This bug will simply notify that the signed package is downloading and that's all)

5) After notifying |OnStartSignedPackageRequest|, we should register the permissions and the system message stuff. This would be happening in |PackagedAppDownloader::InstallSignedPackagedApp|. The implementation is a TODO and supposed to be done by Bug 1178533.

Besides, there are still other TODOs in the patch:

1) If the resource is directly loaded from cache, how could we get the signature? Probably we don't need the signature but we need to know if the resource is from a signed package. One solution is to store 'isSigned' in the meta data of each resource cache.

2) We might need to parse the package header and extract the signature from it. (use mozilla::Tokenizer)

3) The linked list used by PackagedAppVerifier is pointer based, which requires us to manage the life cycle by ourselves.

4) Do we allow people to navigate the manifest? I don't see any apparent reason to say no.

Comment 15

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

Hi Henry,
As I noted in Bug 1178518 comment 13 and 14, I think it's best if we avoided doing any async operations as it complicates the architecture a lot.
I propose the following alternative:

                            PackagedAppDownloader     SignedPackageVerifier 
                                       +                                  +
                                       |    updateManifest(content)       |
                      OnDataAvailable()+---------------------------------->
                                       |    updateManifest(content)       |
                      OnDataAvailable()+---------------------------------->
                                       |       finish()                   |
   <-----------------+OnStopRequest()  +---------------------------------->
OnStartSignedPackageRequest()          |            <- returns immediately|
                                       |                                  |
                                       |                                  |
                                       |                                  |
                                       |                                  |
                      OnDataAvailable()|                                  |
                                       | compute sha1 while downloading   |
                      OnDataAvailable()|                                  |
                                       |       checkIntegrity()           |
   <-----------------+OnStopRequest()  +---------------------------------->
OnResourceVerified()                   |         <- returns immediately   |


(In reply to Henry Chang [:henry] from comment #14)
> Hi Valentin,
> 
> I just attached one patch for this bug and another patch for bug 1186290
> (OnStartSignedPackageRequest support) along with a diagram describing the
> flow regarding signature verification. Can I get your very initial feedback?
> Thanks!
> 
> The main changes are:

The main points of this patch should apply more or less the same if we all approve of a sync way of checking the signature.

> Besides, there are still other TODOs in the patch:
> 
> 1) If the resource is directly loaded from cache, how could we get the
> signature? Probably we don't need the signature but we need to know if the
> resource is from a signed package. One solution is to store 'isSigned' in
> the meta data of each resource cache.

I think this is the best way. Rechecking the manifest/signatures after the first load of the package is unnecessary.

> 2) We might need to parse the package header and extract the signature from
> it. (use mozilla::Tokenizer)

This is preferable.

> 3) The linked list used by PackagedAppVerifier is pointer based, which
> requires us to manage the life cycle by ourselves.

We could use nsTArray<T> if necessary.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Arrays
But if we end up calling to the SignedPackageVerifier directly, this would be unnecessary.

> 4) Do we allow people to navigate the manifest? I don't see any apparent
> reason to say no.

Agreed.

Comment 16

[Henry Chang [:hchang]]

Thanks Valentin!

It seems fine to me for the sync approach. We were planning to use the async and the in-tree implementation. Johnathan will take care of this and I'll just be using it. I personally vote the sync version but don't know if it would meet the schedule (milestone 1 on 9/4).

(In reply to Valentin Gosu [:valentin] from comment #15)
> > 3) The linked list used by PackagedAppVerifier is pointer based, which
> > requires us to manage the life cycle by ourselves.
> 
> We could use nsTArray<T> if necessary.
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Arrays
> But if we end up calling to the SignedPackageVerifier directly, this would
> be unnecessary.
> 

Since I need the operation like 'remove front' and 'push back', nsTArray seems not a efficient implementation.

> > 4) Do we allow people to navigate the manifest? I don't see any apparent
> > reason to say no.
> 
> Agreed.

If we chose the async in the end, do you see any concern of my current implementation? Thanks!

Comment 17

[Honza Bambas (:mayhemer)]

As I understand, one day (or even from the start?) you want to check signatures against a certificate public keys.  If that is so, the cert needs to load from a persistent storage first, right?  If you want the API to be sync then you will probably do a sync main thread IO.  That's not allowed at all.

Comment 18

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

(In reply to Henry Chang [:henry] from comment #16)
> Thanks Valentin!
> 
> It seems fine to me for the sync approach. We were planning to use the async
> and the in-tree implementation. Johnathan will take care of this and I'll
> just be using it. I personally vote the sync version but don't know if it
> would meet the schedule (milestone 1 on 9/4).

That's fair.

> Since I need the operation like 'remove front' and 'push back', nsTArray
> seems not a efficient implementation.

I guess it's OK then, as long as it doesn't get too complicated.

> If we chose the async in the end, do you see any concern of my current
> implementation? Thanks!

Without being too thorough in looking at the patch I'd say it looks good.
My main problem with this way is that it delays serving content to the listener by quite a bit. When OnStopRequest is received, we'd have to switch to another thread, do IO in order to read the content of the manifest into a buffer, check the manifest and pass the callback to the main thread.

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #17)
> As I understand, one day (or even from the start?) you want to check
> signatures against a certificate public keys.  If that is so, the cert needs
> to load from a persistent storage first, right?  If you want the API to be
> sync then you will probably do a sync main thread IO.  That's not allowed at
> all.

Maybe we could get the cert in an async way when the service is initialized, and keep that around? Or would there be multiple certificates signing the manifests?

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #18)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #17)
> > As I understand, one day (or even from the start?) you want to check
> > signatures against a certificate public keys.  If that is so, the cert needs
> > to load from a persistent storage first, right?  If you want the API to be
> > sync then you will probably do a sync main thread IO.  That's not allowed at
> > all.
> 
> Maybe we could get the cert in an async way when the service is initialized,
> and keep that around? Or would there be multiple certificates signing the
> manifests?

Just be aware you still have to block any sync operations before the cert is loaded.

Comment 20

[Henry Chang [:hchang]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #17)
> As I understand, one day (or even from the start?) you want to check
> signatures against a certificate public keys.  If that is so, the cert needs
> to load from a persistent storage first, right?  If you want the API to be
> sync then you will probably do a sync main thread IO.  That's not allowed at
> all.

If we only allow [still TDB] the marketplace to do the signing, do we still need to verify the public key? 

It seems not trivial for sync-version manifest verification. However, for the resource integrity check, the incremental hash computation sounds fairly reasonable to me.

Comment 21

[Honza Bambas (:mayhemer)]

If there is no I/O involved you can freely do the hash calculation synchronously.  If there is even just a slight chance you need any persistent data to check your signature, go async.

Comment 22

[Henry Chang [:hchang]]

Attachment: Bug1178525.patch

Comment 23

[Henry Chang [:hchang]]

Comment on attachment 8651748 [details] [diff] [review]
Bug1178525.patch

Hi Valentin,

Can I have your review on this patch? Actually it still depends on Bug 1178518. However we have only two weeks from now for milestone 1 so I may have to ask for the initial review now. 

The new patch removes all the 'OnStartSignedPackageRequest' things (not related to this bug) and online computes the resource hash with nsICryptoHash as you suggested. The computed hash value is yet to used to check the integrity. It will be done with the function implemented in Bug 1178518.

Also, the new patch stores 'is-signed-pak' and 'signed-pak-origin' through nsIPackagedAppCacheInfoChannel to the cache meta data. The stored 'is-signed-pak' and 'signed-pak-origin' will be read and used if the resource is loaded from cache.
The special package identifier for the signed package will be addressed in Bug 1178526. This patch still uses the original origin for the signed package.

I am very glad to do anything to help you review like a flow chart and some class diagram. Just let me know if you need me to explain anything that confusing to you. Thanks!

Comment 24

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

Hi Henry,

I am currently reviewing the patch.
I have just realized something that might be a bit of a problem. It's about the way we compute the hash for headers. The problem is that in OnStartRequest we can get a nsHttpResponseHead, but if we serialize it we are not guaranteed to get the same headers that were in the package, in the same order. It's actually very unlikely they will match.
This is a problem. The easiest way of fixing it, would be to change nsMultiMixedConv, put the headers in a buffer as they come in, then expose the buffer in nsIMultiPartChannel. We would do this only for mPackagedApp == true.
There are probably other ways too, such as computing the hash in nsMultiMixedConv, and exposing that.

Comment 25

[Henry Chang [:hchang]]

(In reply to Valentin Gosu [:valentin] from comment #24)
> Hi Henry,
> 
> I am currently reviewing the patch.
> I have just realized something that might be a bit of a problem. It's about
> the way we compute the hash for headers. The problem is that in
> OnStartRequest we can get a nsHttpResponseHead, but if we serialize it we
> are not guaranteed to get the same headers that were in the package, in the
> same order. It's actually very unlikely they will match.
> This is a problem. The easiest way of fixing it, would be to change
> nsMultiMixedConv, put the headers in a buffer as they come in, then expose
> the buffer in nsIMultiPartChannel. We would do this only for mPackagedApp ==
> true.
> There are probably other ways too, such as computing the hash in
> nsMultiMixedConv, and exposing that.

Thanks Valentin!

Actually I checked the underlying http header storing and serialization code [1][2] and it seems the flattened order will be kept as the order we set. 

That said, we probably shouldn't reply on the implementation since the order is not guaranteed in the API level.

I will be thinking about it but it may be not a issue at the moment.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.cpp#39
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.cpp?offset=400#207

Comment 26

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

> That said, we probably shouldn't reply on the implementation since the order
> is not guaranteed in the API level.
> 
> I will be thinking about it but it may be not a issue at the moment.

There are a lot of ways to break that. Including 2 headers with the same name in the package would definitely do it. But as you say, that may not be an issue right now. You could file a bug for this, or put the nsMultiMixed part in another patch.

Comment 27

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

Comment on attachment 8651748 [details] [diff] [review]
Bug1178525.patch

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

Overall I think this is pretty good.
I found that TDD works great with this feature, and I'd like to see a test for this patch as well. You can use unit/test_packaged_app_service.js or mochitest/test_web_packaged_app.html as a model, even if the test has a lot of stubs and TODOs.

PackagedAppVerifier.cpp is quite big and complex, and I didn't review it properly. Do you want to land this before or after bug 1178518?

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +378,5 @@
> +
> +  // Feed the flattened and original header to the hasher.
> +  nsAutoCString head;
> +  responseHead->Flatten(head, true);
> +  head.Append("\n");

See comment 24.

@@ +406,5 @@
> +
> +  mVerifier = new PackagedAppVerifier(this,
> +                                      mPackageOrigin,
> +                                      signature,
> +                                      cacheInfoChannel);

Get the cache entry from the base channel, and extract the metadata from that (see my last comment). Write helper function(s) to do that, as we may need to do it multiple times.

@@ +543,5 @@
> +  }
> +
> +  if (!aMulitChannel) {
> +    LOG(("The package is either not loaded from cache or malformed."));
> +    return nsCString("");

EmptyCString();

@@ +607,5 @@
> +
> +  // TODO: |info| has to be explicitly deleted in On[Manifest|Resource]Verified.
> +  // Try to avoid this bad practice when the use of ResourceCacheInfo is
> +  // getting complicated.
> +  ResourceCacheInfo* info = new ResourceCacheInfo(uri, entry, aStatusCode, lastPart);

If we could make the JS implemented verifier in bug 1178518 also be blocking, we wouldn't need to use a LinkedList. We could just query the verifier as subresources come in, and that would ensure both the order an signature are correct.

@@ +743,4 @@
>  }
>  
>  nsresult
> +PackagedAppService::PackagedAppDownloader::RemoveCallbacks(nsICacheEntryOpenCallback* aCallback)

Does not appear to be used anywhere.

::: netwerk/protocol/http/PackagedAppVerifier.cpp
@@ +25,5 @@
> +                                         const nsACString& aPackageOrigin,
> +                                         const nsACString& aSignature,
> +                                         nsIPackagedAppCacheInfoChannel* aCacheInfoChannel)
> +  : mListener(aListener)
> +  , mState(STATE_UNKNOWN)

I assume this is a stub for the JS implemented verifier in bug 1178518.
I really really hope we can make that sync, so we don't need all the state machine we have here.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +70,4 @@
>                            , public nsIThreadRetargetableStreamListener
>                            , public nsIDNSListener
>                            , public nsSupportsWeakReference
> +                          , public nsIPackagedAppCacheInfoChannel

You don't really need this.
You can use nsICachingChannel::GetCacheToken to get an nsISupports, then QI that to nsICacheEntry like so:
https://dxr.mozilla.org/mozilla-central/rev/ba43a48d3c528cc956335793e02504e5ca2c149f/uriloader/prefetch/nsOfflineCacheUpdate.cpp#1000

Comment 28

[Henry Chang [:hchang]]

Thanks for the quick review!

(In reply to Valentin Gosu [:valentin] from comment #27)
> Comment on attachment 8651748 [details] [diff] [review]
> Bug1178525.patch
> 
> Review of attachment 8651748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall I think this is pretty good.
> I found that TDD works great with this feature, and I'd like to see a test
> for this patch as well. You can use unit/test_packaged_app_service.js or
> mochitest/test_web_packaged_app.html as a model, even if the test has a lot
> of stubs and TODOs.
> 

Okay I will make a test case for this patch :)

> PackagedAppVerifier.cpp is quite big and complex, and I didn't review it
> properly. Do you want to land this before or after bug 1178518?
> 

Ideally I hope to wait until Bug 1178518 lands but I don't know the exact time frame. What I can do now is to make my patch as flexible and independent as possible to meet the milestone.

> 
> Does not appear to be used anywhere.
> 
> ::: netwerk/protocol/http/PackagedAppVerifier.cpp
> @@ +25,5 @@
> > +                                         const nsACString& aPackageOrigin,
> > +                                         const nsACString& aSignature,
> > +                                         nsIPackagedAppCacheInfoChannel* aCacheInfoChannel)
> > +  : mListener(aListener)
> > +  , mState(STATE_UNKNOWN)
> 
> I assume this is a stub for the JS implemented verifier in bug 1178518.
> I really really hope we can make that sync, so we don't need all the state
> machine we have here.
> 

PackagedAppVerifier is a lean wrapper of Bug 1178518 plus any important information regarding the verification. So I might still need this object take over some effort from PackagedAppService to avoid it to get more and more complicated.

Regarding the sync/async issue, it seems to me that the manifest verification is hard to be sync as discussed earlier in this bug. So queuing resource information until the manifest is verified might be still needed.

Comment 29

[Henry Chang [:hchang]]

The resource hash computation is already synchronous in the patch. We feed data to update the hash on every PackagedAppDownloader::OnDataAvailable. The computed hashes will be stored in PackagedAppVerifier::mResourceHashHash. Once the manifest is verified, we can then compare the computed hashes with the expected ones in the manifest.

By the way, as Paul suggested, I tend to add some pref like developer mode flag for milestone 1:

When developer mode is enabled, we will just bypass the signature check and treat it signed. (we can still compute the hash but not use it.) When developer is OFF, the verification will still be skipped at the moment until Bug 1178518 lands and the package will be treated unsigned. 

What do you think, Valentin? Thanks :)

Comment 30

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

Comment on attachment 8651748 [details] [diff] [review]
Bug1178525.patch

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

I took a look at PackagedAppVerifier and it looks good.
It feels a bit complicated, but if we add unit tests, we could simplify stuff at any moment.

::: netwerk/protocol/http/PackagedAppVerifier.cpp
@@ +32,5 @@
> +  , mIsPackageSigned(false)
> +  , mCacheInfoChannel(aCacheInfoChannel)
> +{
> +  nsresult rv;
> +  mHasher = do_CreateInstance("@mozilla.org/security/hash;1", &rv);

Initialize mHasher in BeginResourceHash, since it could fail.
Verify that mHasher exists before using it.

@@ +206,5 @@
> +{
> +  ResourceCacheInfo* info = mPendingResourceCacheInfoList.popFirst();
> +  MOZ_ASSERT(info);
> +
> +  // Must be called on main thread.

Assert main thread here too.

::: netwerk/protocol/http/PackagedAppVerifier.h
@@ +115,5 @@
> +  // TODO: Remove this after Bug 1178518 is landed.
> +  void FireFakeSuccessEvent(bool aForManifest);
> +
> +  PackagedAppVerifierListener* mListener;
> +  mozilla::LinkedList<ResourceCacheInfo> mPendingResourceCacheInfoList;

Is there any chance it could be non-empty when the verifier goes away? Maybe use AutoCleanLinkedList instead so we don't leak?

Comment 31

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

(In reply to Henry Chang [:henry] from comment #29)
> The resource hash computation is already synchronous in the patch. We feed
> data to update the hash on every PackagedAppDownloader::OnDataAvailable. The
> computed hashes will be stored in PackagedAppVerifier::mResourceHashHash.
> Once the manifest is verified, we can then compare the computed hashes with
> the expected ones in the manifest.

If the manifest verification were synchronous too, we wouldn't need to store the hashes/entries, and wouldn't have to fire any events.

> 
> By the way, as Paul suggested, I tend to add some pref like developer mode
> flag for milestone 1:
> 
> When developer mode is enabled, we will just bypass the signature check and
> treat it signed. (we can still compute the hash but not use it.) When
> developer is OFF, the verification will still be skipped at the moment until
> Bug 1178518 lands and the package will be treated unsigned. 
> 
> What do you think, Valentin? Thanks :)

Definitely! That sounds great!

Comment 32

[Henry Chang [:hchang]]

(In reply to Valentin Gosu [:valentin] from comment #31)
> (In reply to Henry Chang [:henry] from comment #29)
> > The resource hash computation is already synchronous in the patch. We feed
> > data to update the hash on every PackagedAppDownloader::OnDataAvailable. The
> > computed hashes will be stored in PackagedAppVerifier::mResourceHashHash.
> > Once the manifest is verified, we can then compare the computed hashes with
> > the expected ones in the manifest.
> 
> If the manifest verification were synchronous too, we wouldn't need to store
> the hashes/entries, and wouldn't have to fire any events.
> 

Indeed. ni Johnathan to see his thought regarding the chance of making the manifest verification sync.

> > 
> > By the way, as Paul suggested, I tend to add some pref like developer mode
> > flag for milestone 1:
> > 
> > When developer mode is enabled, we will just bypass the signature check and
> > treat it signed. (we can still compute the hash but not use it.) When
> > developer is OFF, the verification will still be skipped at the moment until
> > Bug 1178518 lands and the package will be treated unsigned. 
> > 
> > What do you think, Valentin? Thanks :)
> 
> Definitely! That sounds great!

Great! Starting working on it!

Comment 33

[Jonathan Hao (inactive) [:jhao]]

It seems that synchronous verification benefits a lot.

The problem with nsDataSignatureVerifier::VerifySignature is that it doesn't not allow specifying trustedRoot.

nsNSSCertificateDB::VerifySignedManifestAsync allows specifying trustedRoot but is asynchronous. However, I think I can use existing code to write a VerifySignedManifestSync for it.

Comment 34

[Henry Chang [:hchang]]

Attachment: Bug1178525_r2.patch

Comment 35

[Henry Chang [:hchang]]

Attached patch v2 with the following changes:

1) Addressed comments in the last review
2) Added developer mode. In developer mode, no matter the signature is correct or even no signature, the    package will be considered signed.
3) Removed the use of the resource cache info buffer since the verification is supposed to be synchronous now.
4) Remove the use of nsIPackagedAppCacheInfoChannel.

Valentin, would you mind reviewing the new patch for now or want to wait for the test case? Thanks.

Comment 36

[Henry Chang [:hchang]]

Hi Valentin,

Some update that I have to let you:

1) According to the review comment in 1178518, the manifest verification would be better of being async. This will lead big change to me since all the interaction between PackagedAppService and the PackagedAppVerifier is callback based.

2) In need of testing PackageAppVerifier and other reasons, Johnathan and I decide to combine his work   with PackagedAppService. The overall flow will be still similar except for:

   a) We will have new interface nsIPackagedAppVerifier having similar operations 
      like OnStartRequest/OnDataAvailable/OnStartRequest. (but the args may be 
      different so it may not inherit nsIStreamListener)
   b) All the signature/certificate things will be done in PackagedAppVerifier.cpp 
      instead of another JS XPCOM.

3) My test case will focus on the behavior of nsIPackagedAppVerifer and some cache meta data check.

It sounds a major change but I think it should be just some sections of code moving around. Wish I could get them done in a few hours. Thanks!

Comment 37

[Henry Chang [:hchang]]

Attachment: Bug1178525_r3.patch

Comment 38

[Henry Chang [:hchang]]

Patch r3 is to make PackagedAppVerifier a XPCOM and unit testable. Not a big change. [1] is the interdiff if needed.
 
[1] https://github.com/elefant/gecko-dev/compare/Bug1178525-delay-serve-content-r1...elefant:Bug1178525-delay-serve-content-r3?diff=split&name=Bug1178525-delay-serve-content-r3

Comment 39

[Henry Chang [:hchang]]

A quick introduction to nsIPackagedAppVerifier:

1) It inherits from nsIStreamListener.

2) PackagedAppDownloader will delegate each OnStart/Data/Stop to both 
   the CacheWriter and the verifier accordingly. (the order matters.)

3) nsIPackagedAppVerifierListener is how the verifier client gets the 
   verification result. It's sync now but will be async according to 
   the latest comment in Bug 1178518.

Comment 40

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

(In reply to Henry Chang [:henry] from comment #36)
> Hi Valentin,
> 
> Some update that I have to let you:
> 
> 1) According to the review comment in 1178518, the manifest verification
> would be better of being async. This will lead big change to me since all
> the interaction between PackagedAppService and the PackagedAppVerifier is
> callback based.
> 

That's ok. I would have preferred it to be sync, making the code clearer, but if it's a big performance problem, async will work :) Your previous implementation with callbacks wasn't bad.

> 2) In need of testing PackageAppVerifier and other reasons, Johnathan and I
> decide to combine his work   with PackagedAppService. The overall flow will
> be still similar except for:
> 
>    a) We will have new interface nsIPackagedAppVerifier having similar
> operations 
>       like OnStartRequest/OnDataAvailable/OnStartRequest. (but the args may
> be 
>       different so it may not inherit nsIStreamListener)
>    b) All the signature/certificate things will be done in
> PackagedAppVerifier.cpp 
>       instead of another JS XPCOM.

Sounds good. I'll try to provide some feedback on the patch tonight.

Comment 41

[Henry Chang [:hchang]]

Attachment: Bug1178525_r4.diff (added test case for PackagedAppVerifier)

Comment 42

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

Comment on attachment 8653997 [details] [diff] [review]
Bug1178525_r4.diff (added test case for PackagedAppVerifier)

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

It looks pretty good to me. I'd love it if Honza gave it a look as well (since I'm not a necko peer), but I think we're on the right track here.

::: modules/libpref/init/all.js
@@ +1438,5 @@
>  // See http://www.w3.org/TR/web-packaging/#streamable-package-format
>  pref("network.http.enable-packaged-apps", false);
>  
> +// Enable this pref to skip verification process. The packaged app
> +// will be considered signed no matter the package has a valid/invalid 

Trailing white space.

::: netwerk/protocol/http/PackagedAppVerifier.cpp
@@ +69,5 @@
> +  mPackageCacheEntry = aPackageCacheEntry;
> +
> +  if (gDeveloperMode && mSignature.IsEmpty()) {
> +    LOG(("No signature but in developer mode ==> Assign a testing signature."));
> +    mSignature.Assign(kTestingSignature);

Why is this needed? Don't we just bypass the check if we are in developer mode?
What is this used for?

@@ +288,5 @@
> +    new ResourceCacheInfo(aUri, aCacheEntry, aStatusCode, aIsLastPart);
> +
> +  *aReturn = info;
> +
> +  NS_IF_ADDREF(*aReturn);

use info.forget(aReturn); instead of manual addreffing.

Comment 43

[Henry Chang [:hchang]]

(In reply to Valentin Gosu [:valentin] from comment #42)
> Comment on attachment 8653997 [details] [diff] [review]
> Bug1178525_r4.diff (added test case for PackagedAppVerifier)
> 
> Review of attachment 8653997 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks pretty good to me. I'd love it if Honza gave it a look as well
> (since I'm not a necko peer), but I think we're on the right track here.
> 

Thanks :)

> ::: modules/libpref/init/all.js
> @@ +1438,5 @@
> >  // See http://www.w3.org/TR/web-packaging/#streamable-package-format
> >  pref("network.http.enable-packaged-apps", false);
> >  
> > +// Enable this pref to skip verification process. The packaged app
> > +// will be considered signed no matter the package has a valid/invalid 
> 
> Trailing white space.
> 
> ::: netwerk/protocol/http/PackagedAppVerifier.cpp
> @@ +69,5 @@
> > +  mPackageCacheEntry = aPackageCacheEntry;
> > +
> > +  if (gDeveloperMode && mSignature.IsEmpty()) {
> > +    LOG(("No signature but in developer mode ==> Assign a testing signature."));
> > +    mSignature.Assign(kTestingSignature);
> 
> Why is this needed? Don't we just bypass the check if we are in developer
> mode?
> What is this used for?
> 

This is for 

PackagedAppVerifier::OnManifestVerified() {

// ...

// Only when the manifest verified and package has signature would we
// regard this package is signed.
mIsPackageSigned = (aSuccess && !mSignature.IsEmpty());

// ...
 
}

Since we need to ensure the package is considered signed in the developer mode, 
we have to assign a testing signature or change the above statement to

mIsPackageSigned = (aSuccess && !mSignature.IsEmpty()) || gDeveloperMode;

Both seems good to me.

> @@ +288,5 @@
> > +    new ResourceCacheInfo(aUri, aCacheEntry, aStatusCode, aIsLastPart);
> > +
> > +  *aReturn = info;
> > +
> > +  NS_IF_ADDREF(*aReturn);
> 
> use info.forget(aReturn); instead of manual addreffing.


Hi Honza, are you available to review this patch? If not, who do you think 
might be familiar with packaged app service enough to review? 

Thanks!

Comment 44

[Honza Bambas (:mayhemer)]

If you want a review from me then few things:
- in any IDL change you must add proper doxygen style comments including argument with a reasonably rich description (think of those who never seen your code)
- comments should be added reasonably anywhere around your changes (mainly for non-obvious stuff)
- the patch must have 8 lines context
- you must describe the patch in a bugzilla comment, best in few points: what it does, how, why exactly the way it is, how is it structured, whatever makes the review easy


Anyway, I think Valentin know the code around.  If he doesn't feel fit I can definitely take the review.

Comment 45

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #44)
> If you want a review from me then few things:
> - in any IDL change you must add proper doxygen style comments including
> argument 

(I mean to also describe each argument)

Comment 46

[Henry Chang [:hchang]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #44)
> If you want a review from me then few things:
> - in any IDL change you must add proper doxygen style comments including
> argument with a reasonably rich description (think of those who never seen
> your code)
> - comments should be added reasonably anywhere around your changes (mainly
> for non-obvious stuff)
> - the patch must have 8 lines context
> - you must describe the patch in a bugzilla comment, best in few points:
> what it does, how, why exactly the way it is, how is it structured, whatever
> makes the review easy
> 

Thanks Honza!

I'll add comments to the IDL changes and more comment to the code.
(and sorry I forgot to dump the diff with -U8). The patch description
is scattered in the previous bug comments and I can sum them up definitely.

> 
> Anyway, I think Valentin know the code around.  If he doesn't feel fit I can
> definitely take the review.

So what do you think, Valentin? I am updating the patch as Honza's suggestion
no matter what. Thanks~

Comment 47

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

That's great. Update the path and I'll do a thorough review. Thanks!

Comment 48

[Henry Chang [:hchang]]

Attachment: Bug1178525_r5.patch (rebased, added more comments)

Comment 49

[Henry Chang [:hchang]]

Comment on attachment 8656994 [details] [diff] [review]
Bug1178525_r5.patch (rebased, added more comments)

Hi Valentin, 

I've updated the patch. If you need anything else, please let me know and I will provide as soon as possible :) Thanks!

Comment 50

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

Comment on attachment 8656994 [details] [diff] [review]
Bug1178525_r5.patch (rebased, added more comments)

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

The patch looks very nice.
I just have a few nits, and I'd love a unit test for the channel with a signed package (see below).
r=me

::: netwerk/base/nsIPackagedAppVerifier.idl
@@ +19,5 @@
> + */
> +[scriptable, uuid(16419a80-4cc3-11e5-b970-0800200c9a66)]
> +interface nsIPackagedAppVerifier : nsIStreamListener
> +{
> +  // The package origin of either signed or unsigned package.

of either a signed

@@ +65,5 @@
> +   *
> +   * @param aIsLastPart
> +   *    whether this resource is the last one in the package.
> +   *
> +   * Create a object that we will pass to the verifier as a user context

an object

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +560,5 @@
> +void
> +PackagedAppService::PackagedAppDownloader::OnError(EErrorType aError)
> +{
> +  // TODO: Handler verification error properly.
> +  LOG(("PackagedAppDownloader::OnError > %d", aError));

Since this is not implemented, the other unit tests will timeout if given a signature and not in dev-mode.
Maybe you could call FinalizeDownload(NS_ERROR_SIGNED_APP_MANIFEST_INVALID); here, and add a unit test similar to test_packaged_app_channel.js (for signed and unsigned packages, in dev/non-dev mode) ?

::: netwerk/protocol/http/PackagedAppVerifier.cpp
@@ +210,5 @@
> +  LOG(("PackagedAppVerifier::OnManifestVerified: %d", aSuccess));
> +
> +  // Only when the manifest verified and package has signature would we
> +  // regard this package is signed.
> +  mIsPackageSigned = (aSuccess && !mSignature.IsEmpty()) || gDeveloperMode;

I think we can lose the || gDeveloperMode here.
We probably want the ability to test unsigned packages in developer mode too.
For signed packages, the developers can simply add a dummy signature, which we ignore in dev mode.
What do you think?

::: netwerk/protocol/http/PackagedAppVerifier.h
@@ +87,5 @@
> +  //
> +  // 1) PackagedAppVerifierListener::OnManifestVerified:
> +  //    ------------------------------------------------------------------------
> +  //    If the resource is the first one in the package, it will be called
> +  //    back in OnManifestVerified no matter this package has signature or not.

no matter if this package has a signature or not.

::: netwerk/test/unit/test_packaged_app_verifier.js
@@ +138,5 @@
> +    [kOrigin + '/4.html',   Cr.NS_OK,    true],
> +    [kOrigin + '/5.css',    Cr.NS_OK,    true],
> +  ];
> +
> +  let isPackageSigned = aDeveloperMode; // Package is always considered as signed in developer mode.

I think unsigned packages should have the same behaviour in dev/non-dev mode.
test_invalid_signature should handle the signed package scenario.

Comment 51

[Henry Chang [:hchang]]

(In reply to Valentin Gosu [:valentin] from comment #50)
> Comment on attachment 8656994 [details] [diff] [review]
> Bug1178525_r5.patch (rebased, added more comments)
> 
> Review of attachment 8656994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks very nice.
> I just have a few nits, and I'd love a unit test for the channel with a
> signed package (see below).
> r=me

Really thanks for the review :)

> 
> ::: netwerk/protocol/http/PackagedAppService.cpp
> @@ +560,5 @@
> > +void
> > +PackagedAppService::PackagedAppDownloader::OnError(EErrorType aError)
> > +{
> > +  // TODO: Handler verification error properly.
> > +  LOG(("PackagedAppDownloader::OnError > %d", aError));
> 
> Since this is not implemented, the other unit tests will timeout if given a
> signature and not in dev-mode.
> Maybe you could call FinalizeDownload(NS_ERROR_SIGNED_APP_MANIFEST_INVALID);
> here, and add a unit test similar to test_packaged_app_channel.js (for
> signed and unsigned packages, in dev/non-dev mode) ?

Good idea!

> ::: netwerk/protocol/http/PackagedAppVerifier.cpp
> @@ +210,5 @@
> > +  LOG(("PackagedAppVerifier::OnManifestVerified: %d", aSuccess));
> > +
> > +  // Only when the manifest verified and package has signature would we
> > +  // regard this package is signed.
> > +  mIsPackageSigned = (aSuccess && !mSignature.IsEmpty()) || gDeveloperMode;
> 
> I think we can lose the || gDeveloperMode here.
> We probably want the ability to test unsigned packages in developer mode too.
> For signed packages, the developers can simply add a dummy signature, which
> we ignore in dev mode.
> What do you think?

> ::: netwerk/test/unit/test_packaged_app_verifier.js
> @@ +138,5 @@
> > +    [kOrigin + '/4.html',   Cr.NS_OK,    true],
> > +    [kOrigin + '/5.css',    Cr.NS_OK,    true],
> > +  ];
> > +
> > +  let isPackageSigned = aDeveloperMode; // Package is always considered as signed in developer mode.
> 
> I think unsigned packages should have the same behaviour in dev/non-dev mode.
> test_invalid_signature should handle the signed package scenario.

I'm fine to it. In this case I need to slightly change the definition of
dev mode :)

Thank you Valentin!

Comment 52

[Henry Chang [:hchang]]

Attachment: Bug1178525_r6.patch (r+'d, added test case, addressed review comments)

Comment 53

[Henry Chang [:hchang]]

Attachment: Bug1178525_r6.patch (r+'d, added test case, addressed review comments)

Comment 54

[Henry Chang [:hchang]]

Attached patch r6 with all the comments addressed and rebased. Waiting for try result!

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

Comment 55

[Pulsebot]

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

Comment 56

[Carsten Book [:Tomcat]]

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