Closed Bug 1077529 Opened 5 years ago Closed 5 years ago

Update of Trusted Hosted Apps

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: zoran.jovanovic, Assigned: zoran.jovanovic)

References

Details

(Whiteboard: [Tako_Blocker])

Attachments

(1 file, 1 obsolete file)

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).
Depends on: 1071085
Attachment #8499658 - Flags: review?(jonas)
Attachment #8499658 - Flags: review?(fabrice)
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 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?
Attachment #8499658 - Flags: review?(jonas) → review-
(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.)
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.
(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.
Split out hash check of manifests and tests. Will create separate bugs
Attachment #8499658 - Attachment is obsolete: true
Attachment #8499658 - Flags: review?(fabrice)
Attachment #8501781 - Flags: review?(jonas)
Attachment #8501781 - Flags: review?(fabrice)
No longer depends on: 1071085
Blocks: 1079915
Bug 1079915 - Test for update of Trusted Hosted Apps
Bug 1079921 - Check hash of manifest of Trusted Hosted App on verification
Attachment #8501781 - Flags: review?(fabrice) → review+
Attachment #8501781 - Flags: review?(jonas)
https://hg.mozilla.org/mozilla-central/rev/35da15d147c5
Assignee: nobody → zoran.jovanovic
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Whiteboard: [Tako_Blocker]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.