Verification of Trusted Hosted Apps manifest signature

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mattias.ostergren, Assigned: vlatko.markovic)

Tracking

unspecified
2.1 S5 (26sep)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(3 attachments, 23 obsolete attachments)

11.11 KB, patch
sicking
: review-
Details | Diff | Splinter Review
4.59 KB, patch
sicking
: review+
Details | Diff | Splinter Review
26.99 KB, patch
zoran.jovanovic
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
No description provided.
Reporter

Updated

5 years ago
Assignee: nobody → vlatko.markovic
Blocks: 1016421
Whiteboard: [2.1-feature-qa+]
Target Milestone: --- → 2.1 S4 (12sep)
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.
Attachment #8479818 - Flags: review?(fabrice)
Attachment #8479818 - Flags: review?(dveditz)
Attachment #8479818 - Flags: review+
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.
Attachment #8479819 - Flags: review?(fabrice) → review-
Whiteboard: [2.1-feature-qa+]
Reporter

Updated

5 years ago
Attachment #8479818 - Attachment is obsolete: true
Attachment #8479818 - Flags: review?(dveditz)
Reporter

Updated

5 years ago
Attachment #8479819 - Attachment is obsolete: true
Reporter

Updated

5 years ago
Attachment #8480664 - Flags: review?(fabrice)
Attachment #8480664 - Flags: review?(dveditz)
Reporter

Updated

5 years ago
Attachment #8480665 - Flags: review?(fabrice)
Attachment #8480665 - Flags: review?(dveditz)
Assignee

Comment 7

5 years ago
Attachment #8480665 - Attachment is obsolete: true
Attachment #8480665 - Flags: review?(fabrice)
Attachment #8480665 - Flags: review?(dveditz)
Attachment #8481391 - Flags: review?(fabrice)
Attachment #8481391 - Flags: review?(dveditz)
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
Attachment #8480664 - Flags: review?(fabrice) → review-
Attachment #8481391 - Flags: review?(fabrice) → review+
Assignee

Comment 9

5 years ago
Attachment #8480664 - Attachment is obsolete: true
Attachment #8480664 - Flags: review?(dveditz)
Attachment #8483449 - Flags: review?(fabrice)
Attachment #8483449 - Flags: review?(dveditz)
Assignee

Comment 10

5 years ago
Attachment #8483450 - Flags: review?(fabrice)
Attachment #8483450 - Flags: review?(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]:
-----------------------------------------------------------------

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
Attachment #8483449 - Flags: review?(fabrice) → review+
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.
Attachment #8483450 - Flags: review?(fabrice)
Comment on attachment 8481391 [details] [diff] [review]
Bug_1059216-Verification-of-Trusted-Hosted-Apps-mani_pt2.patch

I assume this is obsolete
Attachment #8481391 - Attachment is obsolete: true
Attachment #8481391 - Flags: review?(dveditz)
Attachment #8483450 - Flags: review?(dveditz) → review+
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 (//)
Attachment #8483449 - Flags: review-
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?
Attachment #8483449 - Flags: review?(dveditz) → review+

Comment 16

5 years ago
Didn't understand the "maybe "mExpectedSignerSPKIDigest"?" comment.
A more secure algorithm than SHA1 is under investigation.
Attachment #8488240 - Flags: review?(dveditz)
Attachment #8488240 - Flags: review?(dkeeler)

Comment 17

5 years ago
Updated according to review comments.
Attachment #8488240 - Attachment is obsolete: true
Attachment #8488240 - Flags: review?(dveditz)
Attachment #8488240 - Flags: review?(dkeeler)
Attachment #8488243 - Flags: review?(dveditz)
Attachment #8488243 - Flags: review?(dkeeler)
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.
Attachment #8488243 - Flags: review?(dkeeler)
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.
Attachment #8488243 - Flags: review?(dveditz) → review+

Comment 20

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

Comment 21

5 years ago
Updated according to last review comment.
Attachment #8488243 - Attachment is obsolete: true
Attachment #8489458 - Flags: review?(fabrice)
Attachment #8489458 - Flags: review?(dveditz)
Attachment #8489458 - Flags: review?(dveditz) → review+
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(() =>) ?
Attachment #8489458 - Flags: review?(fabrice) → review-

Comment 23

5 years ago
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.
Attachment #8489458 - Attachment is obsolete: true
Attachment #8489579 - Flags: review?(fabrice)

Comment 24

5 years ago
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.
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)

Updated

5 years ago
Attachment #8488240 - Attachment is obsolete: false

Updated

5 years ago
Attachment #8488240 - Flags: review?(dveditz)
Attachment #8488240 - Flags: review?(dkeeler)

Comment 25

5 years ago
- depends on Fabrice's tests in bug 1059194
- adds signature and certs generated by script in bug 1059208
Attachment #8490091 - Flags: review?(fabrice)

Comment 26

5 years ago
Corrected author.
Attachment #8490091 - Attachment is obsolete: true
Attachment #8490091 - Flags: review?(fabrice)
Attachment #8488240 - Flags: review?(dveditz) → review+

Updated

5 years ago
Attachment #8490094 - Flags: review?(fabrice)
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?
Attachment #8489579 - Flags: review?(fabrice)

Comment 28

5 years ago
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.
(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

5 years ago
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.
Attachment #8489579 - Attachment is obsolete: true
Attachment #8490820 - Flags: review?(fabrice)
Attachment #8490820 - Flags: review?(dveditz)
Attachment #8490820 - Flags: review?(dveditz) → review+

Comment 31

5 years ago
- based on changes in bug 1059204
- obsoletes patch ~pt1
Attachment #8490868 - Flags: review?(fabrice)
Attachment #8490868 - Flags: review?(dveditz)
Attachment #8490868 - Flags: review?(dkeeler)
Zoran, can you mark the old patches obsolete? It's starting to be somewhat confusing.
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
Attachment #8490094 - Flags: review?(fabrice)
Attachment #8490820 - Flags: review?(fabrice) → review+
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.
Attachment #8490868 - Flags: review?(fabrice) → feedback+

Comment 35

5 years ago
(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

5 years ago
(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

5 years ago
(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?
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 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.
Attachment #8490868 - Flags: review?(dkeeler) → review+
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.
Attachment #8488240 - Flags: review?(dkeeler)

Comment 41

5 years ago
Refactoring of ThaUtils.jsm
Attachment #8490868 - Attachment is obsolete: true
Attachment #8490868 - Flags: review?(dveditz)
Attachment #8490957 - Flags: review?(fabrice)

Comment 42

5 years ago
Obsoletes ~pt2 patch
Attachment #8490958 - Flags: review?(fabrice)
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
Attachment #8490957 - Flags: review?(fabrice) → review+
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?
Attachment #8490958 - Flags: review?(fabrice)
Assignee

Updated

5 years ago
Attachment #8483449 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8483450 - Attachment is obsolete: true

Updated

5 years ago
Attachment #8488240 - Attachment is obsolete: true

Updated

5 years ago
Attachment #8490820 - Attachment is obsolete: true

Comment 45

5 years ago
Attachment #8490957 - Attachment is obsolete: true
Attachment #8491739 - Flags: review?(rlb)
Attachment #8491739 - Flags: review?(jonas)
Attachment #8491739 - Flags: review?(fabrice)
Attachment #8491739 - Flags: review?(dveditz)
Attachment #8491739 - Flags: review?(dkeeler)

Comment 46

5 years ago
Attachment #8490958 - Attachment is obsolete: true
Attachment #8491741 - Flags: review?(rlb)
Attachment #8491741 - Flags: review?(jonas)
Attachment #8491741 - Flags: review?(fabrice)
Attachment #8491741 - Flags: review?(dveditz)
Attachment #8491741 - Flags: review?(dkeeler)

Comment 47

5 years ago
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 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.
Attachment #8491739 - Flags: review?(fabrice) → review+
Attachment #8491741 - Flags: review?(fabrice) → review+
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.
Attachment #8491739 - Flags: review?(jonas)

Comment 51

5 years ago
- fixes of Fabrice's nits
- fix for identifying the root in TrustedHostedAppsUtils._verifySignedFile (missed in the previous patch)
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 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?
Attachment #8491741 - Flags: review?(jonas) → review-
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.
Attachment #8491741 - Flags: review?(rlb)
Attachment #8491741 - Flags: review?(dveditz)
Attachment #8491741 - Flags: review?(dkeeler)
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.
Attachment #8491741 - Flags: review+

Comment 56

5 years ago
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
Attachment #8491871 - Flags: review?(rlb)
Attachment #8491871 - Flags: review?(jonas)
Attachment #8491871 - Flags: review?(fabrice)
Attachment #8491871 - Flags: review?(dveditz)
Attachment #8491871 - Flags: review?(dkeeler)

Comment 57

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

Obsoleting on :sicking's request
Attachment #8491739 - Attachment is obsolete: true
Attachment #8491739 - Flags: review?(rlb)
Attachment #8491739 - Flags: review?(dveditz)
Attachment #8491739 - Flags: review?(dkeeler)
Attachment #8491871 - Flags: review?(rlb)
Attachment #8491871 - Flags: review?(dkeeler)
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.
Attachment #8491871 - Flags: review?(rlb) → review-
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.
Attachment #8491871 - Flags: review?(jonas)
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.
Attachment #8491871 - Flags: review?(fabrice)
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

5 years ago
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.

Updated

5 years ago
Attachment #8492200 - Flags: feedback?(rlb)
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
Attachment #8492200 - Flags: feedback?(rlb) → feedback-

Comment 64

5 years ago
Now without the pinning, still without in-memory handling of signature file, though.
Attachment #8492200 - Attachment is obsolete: true
Attachment #8492261 - Flags: feedback?(rlb)

Comment 65

5 years ago
Fixes comments from :sicking - trusted hosted app verification strategy encapsulated in TrustedHostedAppsUtils function.
Attachment #8491741 - Attachment is obsolete: true
Attachment #8492262 - Flags: review?(jonas)

Comment 66

5 years ago
Fixes stale comment and comments from :sicking. Had r+ from :fabrice
Attachment #8492262 - Attachment is obsolete: true
Attachment #8492262 - Flags: review?(jonas)
Attachment #8492276 - Flags: review?(jonas)
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!
Attachment #8492276 - Flags: review?(jonas) → review+

Comment 68

5 years ago
Fixes :rbarnes comments.
Attachment #8491871 - Attachment is obsolete: true
Attachment #8492261 - Attachment is obsolete: true
Attachment #8492261 - Flags: feedback?(rlb)
Attachment #8492356 - Flags: review?(rlb)
Attachment #8492356 - Flags: review?(jonas)
Attachment #8492356 - Flags: review?(fabrice)
Attachment #8492356 - Flags: review?(dveditz)
Attachment #8492356 - Flags: review?(dkeeler)
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.
Attachment #8492356 - Flags: review?(dkeeler) → review+
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?
Attachment #8492356 - Flags: review?(rlb) → review-
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.
Attachment #8492356 - Flags: review?(rlb)
Attachment #8492356 - Flags: review?(jonas)
Attachment #8492356 - Flags: review?(fabrice)
Attachment #8492356 - Flags: review?(dveditz)
Attachment #8492356 - Flags: review-
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-
Attachment #8492356 - Flags: review?(rlb) → review-
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.
Attachment #8492356 - Flags: review- → review+
No longer blocks: 1059204
Depends on: 1059204

Comment 74

5 years ago
- r+ from :rbarnes and :dkeeler
- fixes: global declaration in TrustedHostedAppsUtils.jsm, handling of signature URL and comment on use of test root
Attachment #8492356 - Attachment is obsolete: true
Attachment #8492623 - Flags: review+
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

5 years ago
(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 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.
Attachment #8490094 - Flags: review-
Err.. that should say "We need more tests *than* this"
Comment on attachment 8492276 [details] [diff] [review]
0002-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

got a=bajaj over email.
Attachment #8492276 - Flags: approval-mozilla-aurora+
Comment on attachment 8492623 [details] [diff] [review]
0001-Bug-1059216-Verification-of-Trusted-Hosted-Apps-mani.patch

got a=bajaj over email
Attachment #8492623 - Flags: approval-mozilla-aurora+

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.