Closed Bug 1079921 Opened 10 years ago Closed 6 years ago

Check hash of manifest of Trusted Hosted App on verification

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: zoran.jovanovic, Unassigned)

Details

Attachments

(2 files, 3 obsolete files)

Since manifest of Trusted Hosted App is downloaded twice, once by install
routine and once for verification, make sure that it's the same manifest that is handled (using sha256).
Attachment #8501790 - Flags: review?(jonas)
Attachment #8501790 - Flags: review?(fabrice)
Comment on attachment 8501790 [details] [diff] [review]
Double-check-manifest-of-trusted-hosted-app.patch

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

r- because I think that could be simplified a lot by using nsICryptoHash.updateFromStream() instead of what this patch does in _verifySignedFile()

::: dom/apps/AppsUtils.jsm
@@ +701,5 @@
> +      SHA256: Ci.nsICryptoHash.SHA256,
> +      SHA384: Ci.nsICryptoHash.SHA384,
> +      SHA512: Ci.nsICryptoHash.SHA512
> +    };
> +  },

You don't need that to be a function. Callers can just use the Ci.nsICryptoHash values directly.

::: dom/apps/TrustedHostedAppsUtils.jsm
@@ +162,5 @@
>  
> +    // cap manifest to 1MiB
> +    let len = aManifestStream.available();
> +    if (len > 1024 * 1024) {
> +      deferred.reject("MANIFEST_INVALID");

nit: use something more descriptive, like MANIFEST_TOO_LARGE or somesuch
Attachment #8501790 - Flags: review?(fabrice) → review-
(In reply to Fabrice Desré [:fabrice] from comment #1)
> Comment on attachment 8501790 [details] [diff] [review]
> Double-check-manifest-of-trusted-hosted-app.patch
> 
> Review of attachment 8501790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because I think that could be simplified a lot by using
> nsICryptoHash.updateFromStream() instead of what this patch does in
> _verifySignedFile()

Unfortunately, since manifest obtained on installation is an already sanitized JSON object and one obtained for verification is raw file, their hashes would be different. This is why the verification manifest is first transformed to a sanitized JSON object and that in return makes it easy to reuse AppsUtils.computeHash.

> 
> ::: dom/apps/AppsUtils.jsm
> @@ +701,5 @@
> > +      SHA256: Ci.nsICryptoHash.SHA256,
> > +      SHA384: Ci.nsICryptoHash.SHA384,
> > +      SHA512: Ci.nsICryptoHash.SHA512
> > +    };
> > +  },
> 
> You don't need that to be a function. Callers can just use the
> Ci.nsICryptoHash values directly.
> 
> ::: dom/apps/TrustedHostedAppsUtils.jsm
> @@ +162,5 @@
> >  
> > +    // cap manifest to 1MiB
> > +    let len = aManifestStream.available();
> > +    if (len > 1024 * 1024) {
> > +      deferred.reject("MANIFEST_INVALID");
> 
> nit: use something more descriptive, like MANIFEST_TOO_LARGE or somesuch

Ok. Coming with the next patch.
Fixes:
- md5 -> sha256
- hash algorithm check
- file size error
Attachment #8501790 - Attachment is obsolete: true
Attachment #8501790 - Flags: review?(jonas)
Attachment #8502360 - Flags: review?(jonas)
Attachment #8502360 - Flags: review?(fabrice)
It should be quite easy to change the existing code to avoid doing two downloads. It seems much safer too since there's a much smaller risk of bugs like the one we're trying to fix here, and the one I found with MD5 collisions.

All you need to change the 

http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2048
http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2344

to say |xhr.responseType = "arraybuffer";| rather than use "json".

Then in the load handler, change

app.manifest = xhr.response;

to

var td = new TextDecoder();
app.manifest = JSON.parse(td.decode(new Uint8Array(xhr.response)));

You 

Now the unparsed binary data is available in xhr.response and can be used for signature verification without having to redownload the data. So if you just pass that as an additional argument to verifyManifest, you should be able to use that without re-downloading.

Incidentally, I don't see a call to TrustedHostedAppsUtils.verifyManifest in the update code. Just in the install code. I would have expected a call somewhere in

http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#1946

Am I missing it?
(In reply to Jonas Sicking (:sicking) from comment #4)
> It should be quite easy
Famous last words. ;-)

> to change the existing code to avoid doing two
> downloads. It seems much safer too since there's a much smaller risk of bugs
> like the one we're trying to fix here, and the one I found with MD5
> collisions.
> 
> All you need to change the 
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2048
> http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2344
> 
> to say |xhr.responseType = "arraybuffer";| rather than use "json".
> 
> Then in the load handler, change
> 
> app.manifest = xhr.response;
> 
> to
> 
> var td = new TextDecoder();
> app.manifest = JSON.parse(td.decode(new Uint8Array(xhr.response)));
> 
> You 
> 
> Now the unparsed binary data is available in xhr.response and can be used
> for signature verification without having to redownload the data. So if you
> just pass that as an additional argument to verifyManifest, you should be
> able to use that without re-downloading.
Will look into it again. We've thought of these changes before, but settled with doing a cautionary verification re-download of the manifest to double check that manifest and signature are where/what they are supposed to be at any given time. If anything, with one less download installation will be quicker.

> Incidentally, I don't see a call to TrustedHostedAppsUtils.verifyManifest in
> the update code. Just in the install code. I would have expected a call
> somewhere in
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#1946
> 
> Am I missing it?
Bug 1077529
(In reply to Zoran Jovanovic from comment #5)
> (In reply to Jonas Sicking (:sicking) from comment #4)
> > It should be quite easy
> Famous last words. ;-)
> 
> > to change the existing code to avoid doing two
> > downloads. It seems much safer too since there's a much smaller risk of bugs
> > like the one we're trying to fix here, and the one I found with MD5
> > collisions.
> > 
> > All you need to change the 
> > 
> > http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2048
> > http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2344
> > 
> > to say |xhr.responseType = "arraybuffer";| rather than use "json".
> > 
> > Then in the load handler, change
> > 
> > app.manifest = xhr.response;
Right, I almost missed it - then there is

http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2322

Here we already have the manifest. I'm not sure if it makes a lot of sense to store it in raw format, and JSON is convenient for this. That breaks signature check, though, since that has to be byte correct. We do want to check the manifest signature at that point, so we'd best download the matching manifest, check the integrity and verify the signature. (We could fork TrustedHostedAppsUtils.verifyManifest for different input types - if buffer, download only signature; if json, download the manifest again and check the hash...)
(In reply to Zoran Jovanovic from comment #6)
> Right, I almost missed it - then there is
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2322

That's the code path for handling pre-installed apps.

How *do* you want to handle that scenario? Re-downloading the manifest does not seem advisable here since the manifest on the server will not always match the manifest of the preinstalled app. In fact, downloading anything at all here will both give you the problem of getting mismatched data, and also give you the problem that the user might not be online at all, but yet the installation needs to succeed.

So it doesn't seem like the current patch handles this scenario correctly either.

I think we have two options:

1. Assume that pre-installed apps are always correct. We do signature verification on system updates
   already, so I think this should be fine.
2. Bundle the manifest signature with the system update alongside the manifest. In which case you need to
   make sure to load the signature from there.
Something like this?
Attachment #8503181 - Flags: feedback?(jonas)
Attachment #8503181 - Flags: feedback?(fabrice)
Comment on attachment 8502360 [details] [diff] [review]
Double-check-manifest-of-trusted-hosted-app.patch

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

Looks mostly good.

::: dom/apps/Webapps.jsm
@@ +2679,5 @@
>        app.downloadSize = aUpdateManifest.size;
>      }
>  
> +    app.manifestHash = this.computeManifestHash(JSON.stringify(aUpdateManifest ||
> +                                                               aManifest));

computeManifestHash is already stringifying, no need to do it twice ;)
Attachment #8502360 - Flags: review?(jonas)
Attachment #8502360 - Flags: review?(fabrice)
Attachment #8502360 - Flags: feedback+
Fixed some minor thing that cropped up during tests. Passes the tests. If you think this is a good direction, we can close this bug and create one that would reflect better what this patch is doing.
Attachment #8503181 - Attachment is obsolete: true
Attachment #8503181 - Flags: feedback?(jonas)
Attachment #8503181 - Flags: feedback?(fabrice)
Attachment #8508728 - Flags: feedback?(jonas)
Attachment #8508728 - Flags: feedback?(fabrice)
Comment on attachment 8508728 [details] [diff] [review]
WIP-Reuse-downloaded-manifest-on-verification.patch

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

::: dom/apps/TrustedHostedAppsUtils.jsm
@@ +196,1 @@
>      let deferred = Promise.defer();

can we switch to return new Promise() instead? That way we won't forget to return the deferred.

@@ +291,5 @@
> +        return;
> +      }
> +
> +      let td = new TextDecoder();
> +      let manifest = JSON.parse(td.decode(new Uint8Array(aManifest)));

We already do that in the caller in Webapps.jsm. We could send the csp field directly instead since this is the only one we are using here.

::: dom/apps/Webapps.jsm
@@ +1904,5 @@
>            sendError("MANIFEST_PARSE_ERROR");
>            return;
>          }
>  
> +        let td = new TextDecoder();

nit: s/td/decoder
Attachment #8508728 - Flags: feedback?(fabrice) → feedback+
(In reply to Fabrice Desré [:fabrice] from comment #11)
> Comment on attachment 8508728 [details] [diff] [review]
> WIP-Reuse-downloaded-manifest-on-verification.patch
> 
> Review of attachment 8508728 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/TrustedHostedAppsUtils.jsm
> @@ +196,1 @@
> >      let deferred = Promise.defer();
> 
> can we switch to return new Promise() instead? That way we won't forget to
> return the deferred.
Should be OK.

> 
> @@ +291,5 @@
> > +        return;
> > +      }
> > +
> > +      let td = new TextDecoder();
> > +      let manifest = JSON.parse(td.decode(new Uint8Array(aManifest)));
> 
> We already do that in the caller in Webapps.jsm. We could send the csp field
> directly instead since this is the only one we are using here.
Nope. aManifest is used in more places.

> 
> ::: dom/apps/Webapps.jsm
> @@ +1904,5 @@
> >            sendError("MANIFEST_PARSE_ERROR");
> >            return;
> >          }
> >  
> > +        let td = new TextDecoder();
> 
> nit: s/td/decoder
Will do.

Also, seems like Bug 1097079 fixes an issue introduced with this patch. Should be squashed here.
* fixes Fabrice's comments
* fixes Bug 1097079
* We should probably submit a new bug to describe the problem that this patch solves in a better way. Thoughts?
Attachment #8508728 - Attachment is obsolete: true
Attachment #8508728 - Flags: feedback?(jonas)
Attachment #8522158 - Flags: review?(jonas)
Attachment #8522158 - Flags: review?(fabrice)
Comment on attachment 8522158 [details] [diff] [review]
WIP-Reuse-downloaded-manifest-on-verification.patch

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

Not sure if there's currently interest in driving this into the tree, or if we should wait until someone is planning to pick this feature up.

r=me either way.
Attachment #8522158 - Flags: review?(jonas)
Attachment #8522158 - Flags: review?(fabrice)
Attachment #8522158 - Flags: review+
Product: Core → Core Graveyard
Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: