1077529 - Update of Trusted Hosted Apps

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Zoran Jovanovic]

Attachment: 0001-Update-of-Trusted-Hosted-Apps.patch

Verify manifest of trusted hosted app on update instead
of just overwriting it.

Also, since manifest is downloaded twice, once by install
routine and once for verification, make sure that it's
the same manifest that is handled (with MD5 hash).

Comment 1

[Zoran Jovanovic]

Comment on attachment 8499658 [details] [diff] [review]
0001-Update-of-Trusted-Hosted-Apps.patch

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

::: dom/apps/TrustedHostedAppsUtils.jsm
@@ +327,5 @@
>  
>      return deferred.promise;
>    },
>  
> +  verifyManifest: function(aApp, aAppId, aManifest) {

This had to be refactored as aData might not be the same in all use cases, but app object, its ID and manifest to verify are always needed.

Comment 2

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

Comment on attachment 8499658 [details] [diff] [review]
0001-Update-of-Trusted-Hosted-Apps.patch

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

::: dom/apps/TrustedHostedAppsUtils.jsm
@@ +169,5 @@
> +
> +    // Since the manifest file for Trusted Hosted Apps is downloaded
> +    // twice, once by installation routine and once for verification,
> +    // we need to make sure that they are identical, so we will
> +    // compare their MD5 hashes.

MD5 has been broken these days. I think people are able to fairly easily create collisions at will.

At the very least we need to use a stronger hash function. But if we're able to calculate the hash, of the two downloads, why can't we compare the downloaded files themselves?

Or even better, avoid downloading twice?

Comment 3

[Zoran Jovanovic]

(In reply to Jonas Sicking (:sicking) from comment #2)
> Comment on attachment 8499658 [details] [diff] [review]
> 0001-Update-of-Trusted-Hosted-Apps.patch
> 
> Review of attachment 8499658 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/TrustedHostedAppsUtils.jsm
> @@ +169,5 @@
> > +
> > +    // Since the manifest file for Trusted Hosted Apps is downloaded
> > +    // twice, once by installation routine and once for verification,
> > +    // we need to make sure that they are identical, so we will
> > +    // compare their MD5 hashes.
> 
> MD5 has been broken these days. I think people are able to fairly easily
> create collisions at will.
> 
> At the very least we need to use a stronger hash function. 
Agree. However, the idea was to reuse the hash already used to identify the manifest change on update (AppsUtils.computeHash). It's a bit out of scope to fix that here. Create a new bug?

> But if we're able
> to calculate the hash, of the two downloads, why can't we compare the
> downloaded files themselves?
Original code chooses to download the manifest as JSON object for convenience and much of the code depends on that, but to check the signature of the manifest we need to keep the file as is, so the next logical step was to compare them as JSON objects - using hash.

> 
> Or even better, avoid downloading twice?
We thought about that, but then settled that it actually is a good idea to ask the server for manifest and signature to be as expected. (Otherwise, to avoid the two downloads it would require some additional refactoring that seemed out of scope.)

Comment 4

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

Using a weak hash, like MD5, to check if a file has changed is not a security issue. At worst an attacker could upload a changed file which doesn't cause users to receive an update. But that could be accomplished by just uploading a broken manifest too. Either way the user will still be using the previous, safe, version.

However using a weak hash to test that the signed file is the same as the file used to install the update is a security problem. The attacker could serve an attacker generated manifest with a arbitrary contents which is used during the actual installation of the update, then serve a completely different file when we're downloading to test the signature, for example an old version of the manifest which had a correct signature.

Comment 5

[Zoran Jovanovic]

(In reply to Jonas Sicking (:sicking) from comment #4)
> Using a weak hash, like MD5, to check if a file has changed is not a
> security issue. At worst an attacker could upload a changed file which
> doesn't cause users to receive an update. But that could be accomplished by
> just uploading a broken manifest too. Either way the user will still be
> using the previous, safe, version.
> 
> However using a weak hash to test that the signed file is the same as the
> file used to install the update is a security problem. The attacker could
> serve an attacker generated manifest with a arbitrary contents which is used
> during the actual installation of the update, then serve a completely
> different file when we're downloading to test the signature, for example an
> old version of the manifest which had a correct signature.

Agree. I'll separate this into another bug.

Comment 6

[Zoran Jovanovic]

Attachment: Update-Trusted-Hosted-Apps.patch

Split out hash check of manifests and tests. Will create separate bugs

Comment 7

[Zoran Jovanovic]

Bug 1079915 - Test for update of Trusted Hosted Apps
Bug 1079921 - Check hash of manifest of Trusted Hosted App on verification

Comment 8

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/35da15d147c5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35da15d147c5