1059216 - Verification of Trusted Hosted Apps manifest signature

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Comment 1

[Mattias Östergren]

Attachment: Bug_1016421-0001-SIGNATURE-Implement-signature-verification.patch

Comment 2

[Mattias Östergren]

Attachment: Bug_1016421-0002-SIGNATURE-Enable-signed-manifest-verification.patch

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8479818 [details] [diff] [review]
Bug_1016421-0001-SIGNATURE-Implement-signature-verification.patch

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

Meta-nit: check that comments start with a Capital and end with a full stop.

I only looked at Webapps.jsm, which looks fine. I'm flagging dveditz for the signature code.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8479819 [details] [diff] [review]
Bug_1016421-0002-SIGNATURE-Enable-signed-manifest-verification.patch

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

::: dom/apps/src/Webapps.jsm
@@ +2329,5 @@
>          if ("trusted" == app.manifest.type) {
> +          this.verifySignedManifest(aData.app, aData.appId).then(function() {
> +            // Add the appStatus to the incoming app data to allow proper
> +            // installation of TRUSTED hosted application
> +            aData.app.appStatus = Ci.nsIPrincipal.APP_STATUS_TRUSTED;

We decided to not have an APP_STATUS_TRUSTED, right?

@@ +2366,5 @@
>            if ("trusted" == app.manifest.type) {
> +            this.verifySignedManifest(aData.app, aData.appId).then(function() {
> +              // Add the appStatus to the incoming app data to allow proper
> +              // installation of TRUSTED hosted application
> +              aData.app.appStatus = Ci.nsIPrincipal.APP_STATUS_TRUSTED;

Here too.

Comment 5

[Vlatko Markovic]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt1.patch

Comment 6

[Vlatko Markovic]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

Comment 7

[Vlatko Markovic]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8480664 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt1.patch

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

::: dom/apps/src/StoreTrustAnchor.jsm
@@ +17,5 @@
>                            "AppMarketplaceDevReviewersRoot",
>                            "AppMarketplaceStageRoot",
> +                          "AppXPCShellRoot",
> +                          "AppTrustedPublicRoot",
> +                          "AppTrustedTestRoot"];

nit: keep the XPCShell as the last element.

::: dom/apps/src/Webapps.jsm
@@ +2399,5 @@
> +
> +    aCertDb.verifySignedManifestFileAsync(
> +      root, aManifestFile, aSignatureFile,
> +      function(aRv, aCert) {
> +        deferred.resolve([aRv, aCert]);

can we rather reject if aRv is a failure code?

@@ +2448,5 @@
> +
> +    let mRequestChannel = NetUtil.newChannel(aApp.manifestURL)
> +                                 .QueryInterface(Ci.nsIHttpChannel);
> +    mRequestChannel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
> +    mRequestChannel.notificationCallbacks = requestChannelCallbacks;

I believe you want to use createLoadContext() here instead of creating requestChannelCallbacks.

@@ +2453,5 @@
> +
> +    let sRequestChannel = NetUtil.newChannel(aApp.manifestURL.substr(0, aApp.manifestURL.lastIndexOf(".")) + ".sig")
> +                                 .QueryInterface(Ci.nsIHttpChannel);
> +    sRequestChannel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
> +    sRequestChannel.notificationCallbacks = requestChannelCallbacks;

Here too

Comment 9

[Vlatko Markovic]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt1.patch

Comment 10

[Vlatko Markovic]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 8483449 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt1.patch

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

r=me for the dom/apps part with comments addressed.

::: dom/apps/src/Webapps.jsm
@@ +2509,5 @@
> +
> +    let getFilePromises = [];
> +    let manifestFile, signatureFile;
> +
> +    let removeTempFiles = function removeTempFiles() {

nit: no need for |let removeTempFile =|

@@ +2526,5 @@
> +        }
> +      }
> +    }
> +
> +    getFilePromises.push(this._getFile(mRequestChannel, aAppId, "manifest.webapp").then(mFile => {

nit: s/mFile/aFile

@@ +2532,5 @@
> +    }, function(rejectStatus) {
> +      debug("Failed to download file manifest.webapp: " + rejectStatus.msg);
> +    }));
> +
> +    getFilePromises.push(this._getFile(sRequestChannel, aAppId, "manifest.sig").then(sFile => {

nit: s/sFile/aFile

@@ +2549,5 @@
> +
> +      this._verifySignedFile(manifestFile, signatureFile, certDb).then(function(signerCert) {
> +        removeTempFiles();
> +        deferred.resolve();
> +      }, function(errorResult) {

use (aErrorResult) => {...} instead

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8483450 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

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

::: dom/apps/src/Webapps.jsm
@@ +2406,5 @@
>              return;
>            }
>            debug("Trusted App Host certificate OK");
> +          this.verifySignedManifest(aData.app, aData.appId).then(function() {
> +            installApp();

Use then(installApp, aMessage => { ... })

@@ +2418,1 @@
>          }

We could have less indentation by doing:
if (this.kTrustedHosted !== this.appKind(app, app.manifest)) {
  installApp();
  return;
}
if (!isValidTHACertificate(xhr.channel)) { ...

@@ +2445,5 @@
>                return;
>              }
>              debug("Trusted App Host certificate OK");
> +            this.verifySignedManifest(aData.app, aData.appId).then(function() {
> +              installApp();

Use then(installApp, aMessage => { ... })

@@ +2452,5 @@
> +              sendError(aMessage);
> +              return;
> +            });
> +          } else {
> +            installApp();

And same pattern to do an early installApp() + return.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 8481391 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

I assume this is obsolete

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8483449 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt1.patch

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

::: security/apps/AppManifestVerification.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifdef MOZ_LOGGING
> +#define FORCE_PR_LOG 1

There's no logging in this file

@@ +29,5 @@
> +using namespace mozilla;
> +using namespace mozilla::psm;
> +
> +#ifdef PR_LOGGING
> +extern PRLogModuleInfo* gPIPNSSLog;

This is unused

@@ +63,5 @@
> +
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP

use nsresult - this doesn't implement an interface method

@@ +110,5 @@
> +  if (NS_WARN_IF(!base64EncDigest)) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  // Calculate SHA1 digest of the base64 encoded string

For new code, SHA1 really shouldn't be used. Is there a problem with using a more secure hash function?

::: security/apps/AppTrustDomain.cpp
@@ +42,5 @@
>  
>  AppTrustDomain::AppTrustDomain(ScopedCERTCertList& certChain, void* pinArg)
>    : mCertChain(certChain)
>    , mPinArg(pinArg)
> +  , mCertPinningData(nullptr)

maybe "mExpectedSignerSPKIDigest"?

@@ +113,5 @@
>    return SECSuccess;
>  }
>  
> +SECStatus
> +AppTrustDomain::CheckCertPinningData()

Put this logic in AppTrustDomain::IsChainValid - having an additional function isn't necessary.

::: security/apps/AppTrustDomain.h
@@ +22,5 @@
>    AppTrustDomain(ScopedCERTCertList&, void* pinArg);
>  
>    SECStatus SetTrustedRoot(AppTrustedRoot trustedRoot);
>  
> +  /** Check for valid pins of the signer certificate for trusted hosted apps. */

Use C++ style comments (//)

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 8483449 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt1.patch

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

not much to add on top of what Fabrice and David have said.

::: security/apps/AppManifestVerification.cpp
@@ +40,5 @@
> +LoadFileInBuffer(nsIFile * file,
> +                 /*out*/ SECItem & buf)
> +{
> +  nsAutoString fileName;
> +  file->GetLeafName(fileName);

fileName isn't used anywhere. Either delete this or maybe part of the patch is missing?

Comment 16

[Robin Thunell]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt1.patch

Didn't understand the "maybe "mExpectedSignerSPKIDigest"?" comment.
A more secure algorithm than SHA1 is under investigation.

Comment 17

[Robin Thunell]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

Updated according to review comments.

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8488243 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

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

This doesn't have any PSM-specific code in it, so I'll leave it to other reviewers.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 8488243 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

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

r+ assuming the test-cert pref issue will be straightened out.

::: modules/libpref/init/all.js
@@ +1700,5 @@
>  // Disable pinning checks by default.
>  pref("security.cert_pinning.enforcement_level", 2);
>  
> +// Whether to use manifest signing test root certificate for manifest signature verification
> +pref("dom.mozApps.use_trustedapp_test_certs",true);

you don't want to check this in, do you? I would think the test harness would set this to true but the product default should be false.

Comment 20

[Robin Thunell]

True, it's gonna be removed in the next patch update.

Comment 21

[Robin Thunell]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

Updated according to last review comment.

Comment 22

[[:fabrice] Fabrice Desré]

Comment on attachment 8489458 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

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

I haven't tested locally, but I don't think this code works because of the syntax error.

::: dom/apps/src/Webapps.jsm
@@ +2273,5 @@
>      // in which case we don't need to load it.
>      if (app.manifest) {
>        if (checkManifest()) {
> +        debug("Installed manifest check OK");
> +        if (this.kTrustedHosted !== this.appKind(app, app.manifest)) {

isn't app.kind set properly at this point?

@@ +2324,2 @@
>            app.etag = xhr.getResponseHeader("Etag");
> +          if (this.kTrustedHosted !== this.appKind(app, app.manifest)) {

same here.

@@ +2452,4 @@
>          return;
>        }
>  
> +      this._verifySignedFile(manifestFile, signatureFile, certDb).then(() =>) {

Syntax error at then(() =>) ?

Comment 23

[Robin Thunell]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

The blackout syntax error slip through is gone and regarding you question I checked with a debug printout and it wasn't actually set, so I kept it for now.

Comment 24

[Robin Thunell]

Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt1.patch:
Regarding the SHA algorithm change issue, we discovered that the SHA1 algorithm is hard coded in many of the methods in the reused infrastructure, so it would probably take a lot more time to change and verify this properly.

Comment 25

[Zoran Jovanovic]

Attachment: Bug-1059216-Unit-tests-Manifest-signature-verificati.patch

- depends on Fabrice's tests in bug 1059194
- adds signature and certs generated by script in bug 1059208

Comment 26

[Zoran Jovanovic]

Attachment: Bug-1059216-Unit-tests-Manifest-signature-verificati.patch

Corrected author.

Comment 27

[[:fabrice] Fabrice Desré]

Comment on attachment 8489579 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

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

::: dom/apps/src/Webapps.jsm
@@ -2296,2 @@
>        }
> -      return;

Why did you remove this one?

Comment 28

[Robin Thunell]

If an installed manifest is found and the check fails for some reason we want to move on to the code below that downloads a new manifest.

Comment 29

[[:fabrice] Fabrice Desré]

(In reply to Robin Thunell from comment #28)
> If an installed manifest is found and the check fails for some reason we
> want to move on to the code below that downloads a new manifest.

I understand what the code does, but... why? that's a departure from the previous behavior.

Comment 30

[Robin Thunell]

Attachment: Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

The intention was to return only when the CSP or pinning checks failed,
but after a re-discussion it should also return when manifest parsing fails.
So the previous behavior is back.

Comment 31

[Zoran Jovanovic]

Attachment: Bug-1059216-Verification-of-THA-manifest-1.patch

- based on changes in bug 1059204
- obsoletes patch ~pt1

Comment 32

[[:fabrice] Fabrice Desré]

Zoran, can you mark the old patches obsolete? It's starting to be somewhat confusing.

Comment 33

[[:fabrice] Fabrice Desré]

Comment on attachment 8490094 [details] [diff] [review]
Bug-1059216-Unit-tests-Manifest-signature-verificati.patch

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

There's one missing file, and I don't see where the .h files are used, so I can't test that patch locally.

::: dom/apps/tests/mochitest.ini
@@ +2,4 @@
>  skip-if = e10s
>  support-files =
>    chromeAddCert.js
> +  file_app.sig

this file is missing from the patch

Comment 34

[[:fabrice] Fabrice Desré]

Comment on attachment 8490868 [details] [diff] [review]
Bug-1059216-Verification-of-THA-manifest-1.patch

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

I only checked dom/apps and I'd like to see a new version with code moved to ThaUtils.jsm

::: dom/apps/Webapps.jsm
@@ +2345,4 @@
>      xhr.send(null);
>    },
>  
> +  _verifySignedFile: function(aManifestFile, aSignatureFile, aCertDb) {

can we move this method to ThaUtils.jsm ?

@@ +2376,5 @@
> +
> +    return deferred.promise;
> +  },
> +
> +  verifySignedManifest: function(aApp, aAppId) {

And this one too? We'll need to move _getFile to AppsUtils.jsm for that I think.

Comment 35

[Zoran Jovanovic]

(In reply to Fabrice Desré [:fabrice] from comment #32)
> Zoran, can you mark the old patches obsolete? It's starting to be somewhat
> confusing.
I can't obsolete patches I don't own. I asked owners to do that first thing tomorrow morning (it's kind of late here).

Comment 36

[Zoran Jovanovic]

(In reply to Fabrice Desré [:fabrice] from comment #33)
> Comment on attachment 8490094 [details] [diff] [review]
> Bug-1059216-Unit-tests-Manifest-signature-verificati.patch
> 
> Review of attachment 8490094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's one missing file, and I don't see where the .h files are used, so I
> can't test that patch locally.
> 
> ::: dom/apps/tests/mochitest.ini
> @@ +2,4 @@
> >  skip-if = e10s
> >  support-files =
> >    chromeAddCert.js
> > +  file_app.sig
> 
> this file is missing from the patch
The missing file is signature file (will add it in a next patch). Headers are used in security/apps/AppTrustDomain.cpp to build the cert chain. (`./mach build` in gecko takes the headers).

Comment 37

[Zoran Jovanovic]

(In reply to Fabrice Desré [:fabrice] from comment #34)
> Comment on attachment 8490868 [details] [diff] [review]
> Bug-1059216-Verification-of-THA-manifest-1.patch
> 
> Review of attachment 8490868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I only checked dom/apps and I'd like to see a new version with code moved to
> ThaUtils.jsm
> 
> ::: dom/apps/Webapps.jsm
> @@ +2345,4 @@
> >      xhr.send(null);
> >    },
> >  
> > +  _verifySignedFile: function(aManifestFile, aSignatureFile, aCertDb) {
> 
> can we move this method to ThaUtils.jsm ?
> 
> @@ +2376,5 @@
> > +
> > +    return deferred.promise;
> > +  },
> > +
> > +  verifySignedManifest: function(aApp, aAppId) {
> 
> And this one too? We'll need to move _getFile to AppsUtils.jsm for that I
> think.
How will createLoadContext call in verifySignedManifest behave when invoked from ThaUtils.jsm instead of Webapps.jsm?

Comment 38

[[:fabrice] Fabrice Desré]

Yep, we need to move it to AppsUtils too. There are only 3 call sites out of this patch (http://mxr.mozilla.org/mozilla-central/search?string=createLoadContext).

Comment 39

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8490868 [details] [diff] [review]
Bug-1059216-Verification-of-THA-manifest-1.patch

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

I reviewed the changes in security/
Looks good to me with comments addressed (although this will need rebasing with respect to bug 1059204).

::: security/apps/AppSignatureVerification.cpp
@@ +792,5 @@
>  
> +// Load file in the buffer.
> +nsresult
> +LoadFileInBuffer(nsIFile * file,
> +                 /*out*/ SECItem & buf)

nit: 'nsIFile* file', 'SECItem& buf', etc.

@@ +794,5 @@
> +nsresult
> +LoadFileInBuffer(nsIFile * file,
> +                 /*out*/ SECItem & buf)
> +{
> +

nit: unnecessary blank line

@@ +803,5 @@
> +    return rv;
> +  }
> +
> +  uint64_t streamLength;
> +  rv = GetStreamLength(stream, &streamLength);

See bug 1059204 comment 15.

@@ +823,5 @@
> +                     nsIFile* aSignatureFile,
> +                     /*out, optional */ nsIX509Cert** aSignerCert)
> +{
> +  NS_ENSURE_ARG_POINTER(aManifestFile);
> +  NS_ENSURE_ARG_POINTER(aSignatureFile);

nit: I think NS_ENSURE_ARG is more appropriate for both of these

@@ +829,5 @@
> +  if (aSignerCert) {
> +    *aSignerCert = nullptr;
> +  }
> +
> +  nsresult rv;

nit: declare this where it's first used

@@ +837,5 @@
> +  rv = LoadFileInBuffer(aSignatureFile, signatureBuffer);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  signatureBuffer.type = siBuffer;

Does verification fail if this isn't set? If so, this should be set where the buffer is created, in ReadStream.

@@ +855,5 @@
> +    return rv;
> +  }
> +
> +  // Get base64 encoded string from manifest buffer digest
> +  char *base64EncDigest = NULL;

%s/NULL/nullptr/g
Also, I would recommend using a scoped pointer for this: ScopedPtr<char, PORT_Free_string>)

@@ +858,5 @@
> +  // Get base64 encoded string from manifest buffer digest
> +  char *base64EncDigest = NULL;
> +  base64EncDigest =
> +   NSSBase64_EncodeItem(NULL, NULL, 0,
> +                        const_cast<SECItem *>(&manifestCalculatedDigest.get()));

nit: 'SECItem*'

@@ +864,5 @@
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  // Calculate SHA1 digest of the base64 encoded string
> +  Digest doubleDigest;

This seems unnecessary, but if that's how this works, then I guess that's what we have to do...

@@ +865,5 @@
> +  }
> +
> +  // Calculate SHA1 digest of the base64 encoded string
> +  Digest doubleDigest;
> +  rv = doubleDigest.DigestBuf(SEC_OID_SHA1, (uint8_t *)base64EncDigest,

nit: 'reinterpret_cast<uint8_t*>(base64EncDigest)'

@@ +883,5 @@
> +  }
> +
> +  // Return the signer's certificate to the reader if they want it.
> +  // XXX: We should return an nsIX509CertList with the whole validated chain,
> +  //      but we can't do that until we switch to libpkix.

This comment is out of date - I would just remove it.

@@ +986,5 @@
>                                                                 aCallback));
>    return task->Dispatch("SignedJAR");
>  }
> +
> +nsresult

NS_IMETHODIMP

::: security/apps/AppTrustDomain.cpp
@@ +123,5 @@
> +    PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
> +    return SECFailure;
> +  }
> +
> +  if (!mCertPinningData) {

So, just to be clear: if a root other than AppTrustedPublic/TestRoot is specified, this will return SECSuccess. Is this the desired behavior?

@@ +130,5 @@
> +
> +  nsCOMPtr<nsIX509Cert> signerCert =
> +    nsNSSCertificate::Create(CERT_LIST_HEAD(mCertChain)->cert);
> +  if (NS_WARN_IF(!signerCert)) {
> +    PR_SetError(SEC_ERROR_INVALID_ARGS, 0);

This would probably be an out-of-memory error, actually.

@@ +136,5 @@
> +  }
> +
> +  nsAutoCString Sha256SPKIDigest;
> +  signerCert->GetSha256SubjectPublicKeyInfoDigest(Sha256SPKIDigest);
> +  signerCert.forget();

This will cause a leak, I believe. It's already a scoped pointer, so nothing needs to be done for it to be cleaned up at the appropriate time.

@@ +139,5 @@
> +  signerCert->GetSha256SubjectPublicKeyInfoDigest(Sha256SPKIDigest);
> +  signerCert.forget();
> +
> +  if (!Sha256SPKIDigest.EqualsASCII(mCertPinningData)) {
> +    PR_SetError(SEC_ERROR_CERT_NOT_VALID, 0);

We've been using MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE for pinning failures. I'm not sure SEC_ERROR_CERT_NOT_VALID is appropriate for this situation.

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +315,5 @@
> +  /**
> +   *  Verifies the signature on the given Manifest file to verify that it has a
> +   *  valid signature.  To be considered valid, the supplied signature file
> +   *  and the generated signature must match. Further, the signature must come
> +   *  from a certificate that is trusted for code signing.

How about something like this:

Given a file containing a signature and a manifest file, verifies that the signature is valid for the manifest. The signature must come from a certificate that is trusted for code signing and that was issued by the given trusted root.

@@ +323,5 @@
> +   *
> +   *  On failure, an error code is returned.
> +   */
> +  const AppTrustedRoot AppTrustedPublicRoot = 7;
> +  const AppTrustedRoot AppTrustedTestRoot = 8;

Aren't these for 'Trusted Hosted Apps'? The names should probably reflect that (e.g. 'TrustedHostedAppPublicRoot')
Also, these should go where the others are declared.

Comment 40

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8488240 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt1.patch

Cancelling review as this appears to be obsoleted by a newer patch.

Comment 41

[Zoran Jovanovic]

Attachment: 0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

Refactoring of ThaUtils.jsm

Comment 42

[Zoran Jovanovic]

Attachment: 0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

Obsoletes ~pt2 patch

Comment 43

[[:fabrice] Fabrice Desré]

Comment on attachment 8490957 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

r=me for the dom/apps files

Comment 44

[[:fabrice] Fabrice Desré]

Comment on attachment 8490958 [details] [diff] [review]
0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

::: dom/apps/Webapps.jsm
@@ +2320,5 @@
> +               sendError(aMessage);
> +            });
> +          return;
> +        } else {
> +          debug("Downloaded manifest check failed");

don't we need a sendError() in this branch too?

Comment 45

[Zoran Jovanovic]

Attachment: 0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

Comment 46

[Zoran Jovanovic]

Attachment: 0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

Comment 47

[Robin Thunell]

Comment on attachment 8490820 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

New updates in https://bugzilla.mozilla.org/show_bug.cgi?id=1059216 required a new patch.

Comment 48

[Robin Thunell]

...in https://bugzilla.mozilla.org/show_bug.cgi?id=1059208

Comment 49

[[:fabrice] Fabrice Desré]

Comment on attachment 8491739 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

r=me for the dom/apps part. Thanks!

::: dom/apps/AppsUtils.jsm
@@ +116,4 @@
>      return obj;
>    },
>  
> +    // Creates a nsILoadContext object with a given appId and isBrowser flag.

nit: the indentation of the comment is wrong.

::: dom/apps/Webapps.jsm
@@ +2280,1 @@
>                                                                 aData.isBrowser);

nit: re-align with the other parameters

@@ +2393,1 @@
>                                                                 aData.isBrowser);

here too.

Comment 50

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

Comment on attachment 8491739 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

I don't think my review is needed here. Fabrice review for the webapps/appsutils/THAutils changes and the security guys review of the security changes is enough.

Comment 51

[Zoran Jovanovic]

Attachment: 0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

- fixes of Fabrice's nits
- fix for identifying the root in TrustedHostedAppsUtils._verifySignedFile (missed in the previous patch)

Comment 52

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

Comment on attachment 8491741 [details] [diff] [review]
0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

::: dom/apps/Webapps.jsm
@@ +2256,5 @@
> +          return;
> +        }
> +        // sanity check on manifest host's CA
> +        // (proper CA check with pinning is done by regular networking code)
> +        if (!TrustedHostedAppsUtils.isHostPinned(app.manifestURL)) {

I would have added another utility function to TrustedHostedAppsUtils which would wrap the various steps here (manifest-host pinning, manifest signature verification, csp host pinning) and then use a promise to signal success/failure.

But definitely not critical.

@@ +2302,2 @@
>            app.etag = xhr.getResponseHeader("Etag");
> +          if (this.kTrustedHosted !== this.appKind(app, app.manifest)) {

That way you could reuse that here. Shouldn't you check that the manifest domain uses a pinned certificate here too?

Comment 53

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

Comment on attachment 8491741 [details] [diff] [review]
0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

I really think you want the missing host-pinning check here. Not having that would allow all HTML to be loaded through a non-pinned connection for dynamically installed THAs.

::: dom/apps/Webapps.jsm
@@ +2256,5 @@
> +          return;
> +        }
> +        // sanity check on manifest host's CA
> +        // (proper CA check with pinning is done by regular networking code)
> +        if (!TrustedHostedAppsUtils.isHostPinned(app.manifestURL)) {

I would have added another utility function to TrustedHostedAppsUtils which would wrap the various steps here (manifest-host pinning, manifest signature verification, csp host pinning) and then use a promise to signal success/failure.

But definitely not critical.

@@ +2302,2 @@
>            app.etag = xhr.getResponseHeader("Etag");
> +          if (this.kTrustedHosted !== this.appKind(app, app.manifest)) {

That way you could reuse that here. Shouldn't you check that the manifest domain uses a pinned certificate here too?

Comment 54

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

Comment on attachment 8491741 [details] [diff] [review]
0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

Removing other review requests here since I think this needs an update.

Once there's an updated patch, I think I can cover the full review on this one and let others focus on the other patches.

Comment 55

[Richard Barnes [:rbarnes]]

Comment on attachment 8491741 [details] [diff] [review]
0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

::: dom/apps/Webapps.jsm
@@ +2263,5 @@
>          }
>  
> +        TrustedHostedAppsUtils.verifySignedManifest(aData.app, aData.appId)
> +          .then(() => {
> +            if (!TrustedHostedAppsUtils.verifyCSPWhiteList(app.manifest.csp)) {

Is there a reason the verifyCSPWhiteList() call needs to come after the signature verification?  If you move that up to before verifySignedManifest(), then your promise will be a little cleaner. 

> .then( installApp, ... )

@@ +2314,5 @@
> +              if (!TrustedHostedAppsUtils.verifyCSPWhiteList(app.manifest.csp)) {
> +                sendError("TRUSTED_APPLICATION_WHITELIST_VALIDATION_FAILED");
> +                return;
> +              }
> +              installApp();

Likewise here.

Comment 56

[Zoran Jovanovic]

Comment on attachment 8491871 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

- got r+ from :fabrice
- sorry for review spam

Comment 57

[Zoran Jovanovic]

Comment on attachment 8491739 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

Obsoleting on :sicking's request

Comment 58

[Richard Barnes [:rbarnes]]

Comment on attachment 8491871 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

High-level points:
* Pass buffers / nsCStrings, not files
* Don't roll your own pinning checks, use PublicKeyPinningService
* In fact, we don't really need pinning here, just delete it

::: dom/apps/AppsUtils.jsm
@@ +22,4 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
>    "resource://gre/modules/NetUtil.jsm");
>  
> +// Shared code for AppsServiceChild.jsm, TrustedHostedAppsUtils.jsm, Webapps.jsm and Webapps.js

Probably want a line break in here to bring it within 80 columns.

@@ +140,5 @@
> +       }
> +     }
> +  },
> +
> +  getFile: function(aRequestChannel, aId, aFileName) {

It would be helpful to add a comment that explains what this function does, e.g.:

// Sends data downloaded from aRequestChannel to a file
// identified by aId and aFileName.

::: dom/apps/TrustedHostedAppsUtils.jsm
@@ +202,5 @@
> +                                 .QueryInterface(Ci.nsIHttpChannel);
> +    mRequestChannel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
> +    mRequestChannel.notificationCallbacks = AppsUtils.createLoadContext(aAppId, false);
> +
> +    let sRequestChannel = NetUtil.newChannel(aApp.manifestURL.substr(0, aApp.manifestURL.lastIndexOf(".")) + ".sig")

This needs a line break somewhere.

Please make ".sig" a constant, and make sure it is documented somewhere (even if only in a comment) that the signature for a manifest is downloaded from a URL constructed from the manifest URL in this way.

It seems like it would be cleaner to just append ".sig" to the filename, so that you didn't have to mess with substr and the ambiguities of string pattern matching.

@@ +237,5 @@
> +    getFilePromises.push(AppsUtils.getFile(sRequestChannel, aAppId, "manifest.sig").then(aFile => {
> +      signatureFile = aFile;
> +    }, (rejectStatus) => {
> +      debug("Failed to download file manifest.sig: " + rejectStatus.msg);
> +    }));

Please add some line breaks to make this section more readable.

Is it really necessary to download these files to disk?  Couldn't we just read them into in-memory buffers?  I'm not an XPCOM wizard, but it seems like you could buffer them into JS strings, then pass them as nsCStrings into nsIX509CertDB.  (It is not uncommon for binary data to be passed as nsCString.)

@@ +239,5 @@
> +    }, (rejectStatus) => {
> +      debug("Failed to download file manifest.sig: " + rejectStatus.msg);
> +    }));
> +
> +    Promise.all(getFilePromises).then(() => {

Wouldn't it be simpler to skip the push business and just  say

Promises.all([
  AppsUtils.getFile(...).then(...),
  AppsUtils.getFile(...).then(...)
]).then(...)

::: js/xpconnect/src/xpc.msg
@@ +203,5 @@
>  XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_ENTRY_INVALID       , "An entry in the JAR is invalid.")
>  XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_MANIFEST_INVALID    , "The JAR's manifest or signature file is invalid.")
>  
> +/* Codes related to signed manifests */
> +XPC_MSG_DEF(NS_ERROR_SIGNED_MANIFEST_FILE_INVALID   , "The manifest or signature file is invalid.")

For consistency with NS_ERROR_SIGNED_JAR_MANIFEST_INVALID, I would call this error NS_ERROR_SIGNED_APP_MANIFEST_INVALID.  Likewise in the comment ("related to signed app manifests").

::: security/apps/AppSignatureVerification.cpp
@@ +577,5 @@
>    }
>  
> +  if (trustDomain.CheckCertPinningData() != SECSuccess) {
> +    return MapSECStatus(SECFailure);
> +  }

Don't roll your own pinning check.  Delete CheckCertPinningData and instead call PublicKeyPinningService::ChainHasValidPins().  You should have all the arguments you need.

I have to say, I'm a little queasy about overloading pinning in this way.  Pinning was intended as a web-facing feature, and people might not realize the interaction.  The same database is used for both HTTPS key pinning and app key pinning, so if a web site installs a pin for itself, it is also limiting what keys can be used to sign apps.  

I would prefer if we could remove this check, and just rely on the fact that the certs used to verify THA manifests chain to the THA root.

@@ +766,5 @@
>  }
>  
> +// Load file in the buffer.
> +nsresult
> +LoadFileInBuffer(nsIFile* file,

Remove this and just pass buffers directly.

@@ +810,5 @@
> +  ScopedAutoSECItem manifestBuffer;
> +  rv = LoadFileInBuffer(aManifestFile, manifestBuffer);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }

Again, just pass the buffers.

@@ +835,5 @@
> +                              reinterpret_cast<uint8_t*>(base64EncDigest.get()),
> +                              strlen(base64EncDigest.get()));
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

I'm assuming that this ridiculously convoluted signing scheme is some legacy cruft that we have to support?

::: security/apps/AppTrustDomain.cpp
@@ +29,5 @@
> +// Trusted Hosted Apps Certificates
> +#include "manifest-signing-root.inc"
> +#include "manifest-signing-cert-pinning.inc"
> +#include "manifest-signing-test-root.inc"
> +#include "manifest-signing-test-cert-pinning.inc"

What are these files, and where do they come from?

@@ +43,4 @@
>  AppTrustDomain::AppTrustDomain(ScopedCERTCertList& certChain, void* pinArg)
>    : mCertChain(certChain)
>    , mPinArg(pinArg)
> +  , mCertPinningData(nullptr)

Remove

@@ +96,5 @@
> +    case nsIX509CertDB::TrustedHostedAppTestRoot:
> +      trustedDER.data = const_cast<uint8_t*>(trustedAppTestRoot);
> +      trustedDER.len = mozilla::ArrayLength(trustedAppTestRoot);
> +      mCertPinningData = trustedAppTestSignerSpkiDigest;
> +      break;

Where is the patch that adds these roots?

@@ +113,5 @@
>    return SECSuccess;
>  }
>  
> +SECStatus
> +AppTrustDomain::CheckCertPinningData()

Delete this method.

::: security/apps/AppTrustDomain.h
@@ +23,5 @@
>  
>    SECStatus SetTrustedRoot(AppTrustedRoot trustedRoot);
>  
> +  // Check for valid pins of the signer certificate for trusted hosted apps.
> +  SECStatus CheckCertPinningData();

Delete this method.

@@ +53,4 @@
>  private:
>    /*out*/ ScopedCERTCertList& mCertChain;
>    void* mPinArg; // non-owning!
> +  const char* mCertPinningData;

Delete.

::: xpcom/base/ErrorList.h
@@ +866,5 @@
>    /* ======================================================================= */
> +  /* 38: NS_ERROR_MODULE_SIGNED_MANIFEST */
> +  /* ======================================================================= */
> +#define MODULE NS_ERROR_MODULE_SIGNED_MANIFEST
> +  ERROR(NS_ERROR_SIGNED_MANIFEST_FILE_INVALID,   FAILURE(1)),

See above on error naming.

::: xpcom/base/nsError.h
@@ +72,4 @@
>  #define NS_ERROR_MODULE_SIGNED_JAR 35
>  #define NS_ERROR_MODULE_DOM_FILESYSTEM 36
>  #define NS_ERROR_MODULE_DOM_BLUETOOTH 37
> +#define NS_ERROR_MODULE_SIGNED_MANIFEST 38

See above on error naming.

Comment 59

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

Comment on attachment 8491871 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

I think rbarnes and fabrice's reviews are enough here. Unless Dan wants to review too.

Comment 60

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

Comment on attachment 8491871 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

Oh, looks like fabrice already did r+ his parts. So removing his request too.

Comment 61

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

Ok, so action here is that we need a new version of the 0001 patch which addresses rbarnes review comments. Rbarnes should review that updated patch.

We also need a new version of the 0002 patch which addresses my review comment.

Comment 62

[Zoran Jovanovic]

Attachment: 0001-WIP-Bug-1059216-Verification-of-Trusted-Hosted-Apps.patch

Fixes all comments from rbarnes on patch 1 except handling signature files in memory instead of on disk. We couldn't find appropriate interfaces in X509Cert* for munging binary data and to use a string for that sounds risky, e.g. due to character width, encoding and similar that will be both very difficult to get right and subject to change.

Comment 63

[Richard Barnes [:rbarnes]]

Comment on attachment 8492200 [details] [diff] [review]
0001-WIP-Bug-1059216-Verification-of-Trusted-Hosted-Apps.patch

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

Two things:

1. This still does custom pinning checks.  Please use PublicKeyPinningService.

2. For passing the signature and manifest files around, it looks to me like the easiest thing to do is just pass around input streams.
  * Have _getFile resolve with an nsIInputStream instead of an nsIFile
  * Pass that through all the layers, until ...
  * In the ctor for VerifySignedmanifestFileTask, read the file into a buffer

For (2), nsCryptoHash illustrates some good design patterns:
http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsICryptoHash.idl#85
http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsCryptoHash.cpp#140

Comment 64

[Zoran Jovanovic]

Attachment: 0001-WIP-Bug-1059216-Verification-of-Trusted-Hosted-Apps.patch

Now without the pinning, still without in-memory handling of signature file, though.

Comment 65

[Zoran Jovanovic]

Attachment: 0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

Fixes comments from :sicking - trusted hosted app verification strategy encapsulated in TrustedHostedAppsUtils function.

Comment 66

[Zoran Jovanovic]

Attachment: 0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

Fixes stale comment and comments from :sicking. Had r+ from :fabrice

Comment 67

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

Comment on attachment 8492276 [details] [diff] [review]
0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

Looks great!

Comment 68

[Zoran Jovanovic]

Attachment: 0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

Fixes :rbarnes comments.

Comment 69

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8492356 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

security/apps and security/manager changes look good to me.

Comment 70

[Richard Barnes [:rbarnes]]

Comment on attachment 8492356 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

This is much better.  My only major concern is the ReadStream method doesn't seem to be defined.  If someone can point out to me where that lives, I'll switch to r+

::: dom/apps/TrustedHostedAppsUtils.jsm
@@ +2,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* global Components, Services, dump, AppsUtils, NetUtil */

Promise?  XPCOMUtils?

@@ +162,5 @@
> +    let useTrustedAppTestCerts = false;
> +    try {
> +      useTrustedAppTestCerts = Services.prefs.
> +                           getBoolPref("dom.mozApps.use_trustedapp_test_certs");
> +    } catch (ex) { }

It seems like there's some security concern about switching between the test and real roots in this way.  It's fine as long as there's no way to change prefs on a device.  But as soon as that's false, this creates a way for an attacker to use the test root for real apps.  Maybe just add a comment here.

@@ +209,5 @@
> +
> +    // The manifest signature is located at the same path as the
> +    // manifest, it has the same file name, only the file extension
> +    // differs.
> +    let sFileChannel = NetUtil.newChannel(aApp.manifestURL

This still makes me a little nervous.  E.g., what if the manifestURL has a query parameter?  Maybe add a comment of the form:

// Note that this implies that the manifest URL must be 
// of the form https://example.com/path/file.ext -- it
// must have a file extension at the end and it must not 
// have a fragment or query component.

Now that I think of it, you could test those latter two properties by just parsing the URL, as an extra sanity check.

::: security/apps/AppSignatureVerification.cpp
@@ +787,5 @@
> +  }
> +
> +  // Load signature file in buffer
> +  ScopedAutoSECItem signatureBuffer;
> +  nsresult rv = ReadStream(aSignatureStream, signatureBuffer);

Where did ReadStream come from?  Did you forget to include it in the patch, or is it in one of the other patches?

Comment 71

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

Comment on attachment 8492356 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

I don't think my or Dan's reviews are needed here. And Fabrice already reviewed his parts.

Comment 72

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

Comment on attachment 8492356 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

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

restoring the r-

Comment 73

[Richard Barnes [:rbarnes]]

Comment on attachment 8492356 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

Jonas reminded me that ReadStream was in bug 1059204.

Comment 74

[Zoran Jovanovic]

Attachment: 0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

- r+ from :rbarnes and :dkeeler
- fixes: global declaration in TrustedHostedAppsUtils.jsm, handling of signature URL and comment on use of test root

Comment 75

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

This still doesn't build:

make[5]: *** No rule to make target `/Users/sicking/trees/b2ginbound/src/security/apps/../manager/ssl/tests/unit/test_signed_manifest/trusted_ca1.der', needed by `manifest-signing-test-root.inc'.  Stop.

Comment 76

[Zoran Jovanovic]

(In reply to Jonas Sicking (:sicking) from comment #75)
> This still doesn't build:
> 
> make[5]: *** No rule to make target
> `/Users/sicking/trees/b2ginbound/src/security/apps/../manager/ssl/tests/unit/
> test_signed_manifest/trusted_ca1.der', needed by
> `manifest-signing-test-root.inc'.  Stop.

Depends on 1059208 . Include files are generated in a makefile in that patch.

Comment 77

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

Comment on attachment 8490094 [details] [diff] [review]
Bug-1059216-Unit-tests-Manifest-signature-verificati.patch

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

We need more tests like this. The most important thing that we don't want to break is that a wrongly signed manifest should result in installation failing. As should a completely unsigned manifest. And it please do test that a manifest signed with the test cert does not get installed if the dom.mozApps.use_trustedapp_test_certs pref has not been set.

::: dom/apps/tests/file_app.sjs
@@ +85,4 @@
>      return;
>    }
>  
> +  if ('gettrustedmanifest' in query) {

Shouldn't some file in this test set this query parameter? If not this code won't get executed.

Comment 78

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

Err.. that should say "We need more tests *than* this"

Comment 79

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/c1a81857b963
https://hg.mozilla.org/integration/b2g-inbound/rev/7cbad2b6f34e

Comment 80

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

Comment on attachment 8492276 [details] [diff] [review]
0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

got a=bajaj over email.

Comment 81

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

Comment on attachment 8492623 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

got a=bajaj over email

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1a81857b963
https://hg.mozilla.org/mozilla-central/rev/7cbad2b6f34e

Comment 83

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c4b8e3f6b53
https://hg.mozilla.org/releases/mozilla-aurora/rev/9396b6408686