1059202 - Certificate verfication for Trusted Hosted Apps

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Mattias Östergren]

Implement function for verification of host TLS/SSL certificates and enable this functionality for trusted hosted application hosts only.

Comment 1

[Vlatko Markovic]

Attachment: Bug_1059202-Certificate-verfication-for-Trusted-Host_pt1.patch

Comment 2

[Vlatko Markovic]

Attachment: Bug_1059202-Certificate-verfication-for-Trusted-Host_pt2.patch

Comment 3

[Vlatko Markovic]

Attachment: Bug_1059202-Certificate-verfication-for-Trusted-Host_pt3.patch

Comment 4

[Vlatko Markovic]

Attachment: Bug_1059202-Certificate-verfication-for-Trusted-Host_pt4.patch

Comment 5

[Vlatko Markovic]

Attachment: Bug_1059202-Certificate-verfication-for-Trusted-Host_pt2.patch

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8480655 [details] [diff] [review]
Bug_1059202-Certificate-verfication-for-Trusted-Host_pt1.patch

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

r-  -- I really want to see the complete function, not try to put several patches together in my head and then guess whether I've done that correctly or if more things are coming. The threat model says we're going to check that the certificate is pinned, and I see in a later patch you add support for the pinning but the later patch to this function does not make use of it. Are there more patches to this function in other bugs?

::: dom/apps/src/Webapps.jsm
@@ +12,5 @@
>  // Possible errors thrown by the signature verifier.
>  const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE;
>  const SEC_ERROR_EXPIRED_CERTIFICATE = (SEC_ERROR_BASE + 11);
>  
> +function checkCertificate(channel) {

This function needs a more descriptive name. This isn't a general-purpose certificate checker. The first thing that comes to my mind is "isCertificateValidForTHA()" or "isValidTHACertificate()"  (or drop the "is" part depending on flow where it's used).

It also needs a header-comment explaining what we think we're checking for.

The version in this patch is only checking whether it's a valid TLS connection, which is unnecessary if you've got a valid connection. Other checks seem to be added in future patches, but without that comment saying what we think we're looking for how do we know when we're done?

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 8481390 [details] [diff] [review]
Bug_1059202-Certificate-verfication-for-Trusted-Host_pt2.patch

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

r+ with the requested changes.

::: dom/apps/src/Webapps.jsm
@@ +1427,5 @@
> +      aOnSuccess();
> +    }
> +
> +    // Launching Trusted Hosted App?
> +    if (this.kTrustedHosted == app.kind) {

This buries the "normal case" at the end. Maybe reverse the sense

  if (this.kTrustedHosted != app.kind) {
    finalizeLaunch();
  } else {
    // do additional checks before launching a trusted-hosted app

@@ +1452,5 @@
> +            aOnFailure("TRUSTED_APPLICATION_HOST_CERTIFICATE_INVALID");
> +            return;
> +          } else {
> +            debug("Trusted App Host certificate OK");
> +            finalizeLaunch();

You don't need the "else" after a bail-out return, just put the code at the same level. (this isn't just a personal preference, this appears to be such a major pet peeve around here that it's the first rule in the coding guidelines: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style )

@@ +2352,5 @@
> +            debug("Trusted App Host certificate OK");
> +            installApp();
> +          }
> +        } else {
> +          installApp();

You don't need the }else{ after the return. It is more readable if you combined the two calls to installApp() and put them at the same levels as the "if (this.kTrustedHosted..." line. That is

  if (this.kTrustedHosted == this.appKind(app, app.manifest)) {
    if (!checkCertificate(xhr.channel)) {
      sendError("TRUSTED_APPLICATION_HOST_CERTIFICATE_INVALID");
      return;
    } else {
      debug("Trusted App Host certificate OK");
    }
  }
  installApp();

@@ +2381,5 @@
> +          if (this.kTrustedHosted == this.appKind(app, app.manifest)) {
> +            if (!checkCertificate(xhr.channel)) {
> +              sendError("TRUSTED_APPLICATION_HOST_CERTIFICATE_INVALID");
> +              return;
> +            } else {

Same here.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 8480658 [details] [diff] [review]
Bug_1059202-Certificate-verfication-for-Trusted-Host_pt3.patch

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

Changes to the pinning code need to be checked by Camilo.

Given the threat model paper I thought these sites were simply pinned using the existing pin mechanism. Instead it appears you're creating a separate kind of pin just for manifests, such that the sites will NOT be pinned for normal web browser visits or in other apps but only checked for THA app manifests. That's certainly a valid thing to do but that design was not clear to me from anything I've seen describing THA. I feel like I'm missing some design document somewhere, and if there is one it would really help in these reviews.

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +403,5 @@
> +   *
> +   * @return  true  If the certificate pinning is found and valid.
> +   *          false If there is no pinning information or pinning doesn't match.
> +   */
> +  boolean isThaHostPinValid(in nsIX509Cert aCert, in string aHostname);

I'm certain we don't want to add this to this interface which is used by a lot of code (some in Firefox add-ons). Unless this change is only going to land on the b2g 2.1 branch and never be incorporated into future versions this is not going to pass review. 

If THA pins are a separate kettle of fish and not simply pinned sites then you need a separate interface for it.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 8480660 [details] [diff] [review]
Bug_1059202-Certificate-verfication-for-Trusted-Host_pt4.patch

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

The CertVerifier changes need to go by Camilo or Keeler

::: dom/apps/src/Webapps.jsm
@@ +40,5 @@
> +    if (!valid) {
> +      return valid;
> +    } else {
> +      // Also check the certificate pinning.
> +      valid = false;

else-after-return again. Maybe beef up the comment ("reset valid while we check certificate pinning"?) so setting valid to the value you just checked against doesn't look so odd.

@@ +47,1 @@
>      // Print SSL certificate details

This comment is very misleading -- the following block is now doing the all-important pin checking. Actually since the pin-checking doesn't depend on secInfo in any way it probably shouldn't be in that block: close the debug printing block and check the pin at the same level as "if (secInfo"

Comment 10

[Zoran Jovanovic]

(In reply to Daniel Veditz [:dveditz] from comment #8)
> Comment on attachment 8480658 [details] [diff] [review]
> Bug_1059202-Certificate-verfication-for-Trusted-Host_pt3.patch
> 
> Review of attachment 8480658 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes to the pinning code need to be checked by Camilo.
> 
> Given the threat model paper I thought these sites were simply pinned using
> the existing pin mechanism. Instead it appears you're creating a separate
> kind of pin just for manifests, such that the sites will NOT be pinned for
> normal web browser visits or in other apps but only checked for THA app
> manifests. That's certainly a valid thing to do but that design was not
> clear to me from anything I've seen describing THA. I feel like I'm missing
> some design document somewhere, and if there is one it would really help in
> these reviews.
> 
> ::: security/manager/ssl/public/nsIX509CertDB.idl
> @@ +403,5 @@
> > +   *
> > +   * @return  true  If the certificate pinning is found and valid.
> > +   *          false If there is no pinning information or pinning doesn't match.
> > +   */
> > +  boolean isThaHostPinValid(in nsIX509Cert aCert, in string aHostname);
> 
> I'm certain we don't want to add this to this interface which is used by a
> lot of code (some in Firefox add-ons). Unless this change is only going to
> land on the b2g 2.1 branch and never be incorporated into future versions
> this is not going to pass review. 
> 
> If THA pins are a separate kettle of fish and not simply pinned sites then
> you need a separate interface for it.

We intentionally kept the spec simple and as aligned to current app types as possible - so the additional requirement is just "always apply CA pinning".

We implemented it this way for several reasons:
1. Pinning can be turned on or off in configuration. For THA, it has to be always on.
2. To check pinning on install/launch in case pinning is configured away, there needs to be an API in .jsm.
3. Existing pinning implementation allows access to domains that don't have pins, and for THA access to domains without pins should be rejected. (THA can only access domains with pins.)
4. We wanted to minimize changes in pinning code and reduce chance of regressions.

Provided that pinning is always 'on' AND domains without pinning data are rejected, then the interface we added could most probably go away.

Comment 11

[Vlatko Markovic]

Attachment: Bug_1059202-Certificate-verfication-for-Trusted-Host_pt1.patch

Comment 12

[Vlatko Markovic]

Attachment: Bug_1059202-Certificate-verfication-for-Trusted-Host_pt2.patch

Comment 13

[Vlatko Markovic]

Attachment: Bug_1059202-Certificate-verfication-for-Trusted-Host_pt3.patch

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 8483437 [details] [diff] [review]
Bug_1059202-Certificate-verfication-for-Trusted-Host_pt2.patch

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

r=dveditz

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 8483442 [details] [diff] [review]
Bug_1059202-Certificate-verfication-for-Trusted-Host_pt3.patch

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

r=dveditz

Jonas suggested it would be better performance if we could call NSS directly to test whether a site has a pin set in the database rather than having to make a connection to do it (especially for the CSP policy test in another bug). I agree that makes more sense, but in the meantime if we stick to the probing approach this patch is fine.

Comment 16

[Richard Barnes [:rbarnes]]

Comment on attachment 8483434 [details] [diff] [review]
Bug_1059202-Certificate-verfication-for-Trusted-Host_pt1.patch

Clearing review flag on this for now, since my impression from the call this morning was that there will need to be some significant changes.  Please re-send review request after edits.

Comment 17

[Zoran Jovanovic]

Attachment: Bug_1059202-WIP-Run-local-check-for-pinning-on-launch.patch

WIP patch to show how things might work once correct interface to query pinning database lands in bug 1063226. I'm using a hack on X509CertDB perusing old patches just to make things work, so don't be alarmed - as soon as interface is ready we take that.

This now works with our test apps. I will spend some time during tomorrow to double check some offline use cases, but it looks OK so far.

(Please disregard the style... ;-)

Comment 18

[Zoran Jovanovic]

(In reply to Zoran Jovanovic from comment #17)
> Created attachment 8486443 [details] [diff] [review]
> Bug_1059202-WIP-Run-local-check-for-pinning-on-launch.patch
> 
> WIP patch to show how things might work once correct interface to query
> pinning database lands in bug 1063226. I'm using a hack on X509CertDB
> perusing old patches just to make things work, so don't be alarmed - as soon
> as interface is ready we take that.
> 
> This now works with our test apps. I will spend some time during tomorrow to
> double check some offline use cases, but it looks OK so far.
> 
> (Please disregard the style... ;-)

And also, it's a diff from the original Vlatko's patch for comparison...

Comment 19

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

Comment on attachment 8486443 [details] [diff] [review]
Bug_1059202-WIP-Run-local-check-for-pinning-on-launch.patch

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

This is starting to look great, but I think there's more simplifications that can be done now that we don't need asynchronous checks at launch time.

I think most of the changes in the "pt3" patch are reverted by this patch, so it might make sense to combine this patch and pt3.

::: dom/apps/src/Webapps.jsm
@@ +16,5 @@
>  /**
> + * Check if the given host is pinned in the CA pinning database.
> + *
> + */
> +function isHostPinned(aUri) {

It might be simpler to make this function take a string rather than an nsIURI. That way you can make this function return false if the string doesn't parse as an nsIURI (which is a case you'll hit if someone specifies a csp policy which allows connecting to a domain without a scheme, and where returning false is the right thing to do)

@@ -66,4 @@
>        debug("\tValid from " + validity.notBeforeGMT + "\n");
>        debug("\tValid until " + validity.notAfterGMT + "\n");
>  
> -      // Check certificate pinning

Do we need this function (isValidTHACertificate) at all now?

@@ +1503,5 @@
>  
> +      // sanity check on manifest host's CA
> +      // (proper CA check with pinning is done by regular networking code)
> +      let manifestURI = Services.io.newURI(aManifestURL, null, null);
> +      if (!isHostPinned(manifestURI)) {

I mentioned this to Vlatko in the review of an earlier patch, but I think it might be cleaner to simply combine this with the pinning checks from the csp. I.e. simply add the manifest origin to the list created by getCSPWhiteList below.

@@ +1515,5 @@
> +      if (domainWhitelist.valid) {
> +        let selfId = domainWhitelist.list.indexOf("'self'");
> +        if (-1 != selfId) {
> +          // replace 'self' with app.origin URL in the whitelist
> +          domainWhitelist.list.splice(selfId, 1, app.origin);

That way you can simply do

domainWhitelist.list = domainWhiteList.filter((v) => v != "'self'");

here.

@@ +1527,5 @@
>            return;
>          }
> +      } else {
> +        debug("CSP Whitelist parsing failed!");
> +        aOnFailure("TRUSTED_APPLICATION_WHITELIST_PARSING_FAILED");

It'd be cleaner to add a separate "verifyTHAforLaunch(manifest)" (or similarly named) function which returns a boolean to indicate success or not. That way you don't have to have separate error handlers for the saparate error conditions here.

Also, that function can be synchronous now that we go directly to the pinning database. This means that the finalizeLaunch function created in an earlier patch can be removed.

@@ +4598,5 @@
>  
> +      try {
> +        uri = Services.io.newURI(aUrl, null, null);
> +      } catch(e) {
> +        debug("Host parsing failed: " + e);

This

Comment 20

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

Comment on attachment 8483442 [details] [diff] [review]
Bug_1059202-Certificate-verfication-for-Trusted-Host_pt3.patch

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

Reverting Dan's review here given that we decided to go with the pinning check approach instead.

::: dom/apps/src/Webapps.jsm
@@ +2399,5 @@
>      // in which case we don't need to load it.
>      if (app.manifest) {
>        if (checkManifest()) {
> +        if (this.kTrustedHosted == this.appKind(app, app.manifest)) {
> +          if (!isValidTHACertificate(xhr.channel)) {

Couldn't you simply check "isHostPinned" before starting this request? Or even after the request if that's simpler.

@@ +2430,5 @@
>          app.manifest = xhr.response;
>          if (checkManifest()) {
>            app.etag = xhr.getResponseHeader("Etag");
> +          if (this.kTrustedHosted == this.appKind(app, app.manifest)) {
> +            if (!isValidTHACertificate(xhr.channel)) {

Same here?

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to Jonas Sicking (:sicking) from comment #19)
> It might be simpler to make this function take a string rather than an
> nsIURI. That way you can make this function return false if the string
> doesn't parse as an nsIURI (which is a case you'll hit if someone specifies
> a csp policy which allows connecting to a domain without a scheme, and where
> returning false is the right thing to do)

What is a trusted hosted app's 'self'? If we're following the spec a domain without a scheme is perfectly fine and the scheme of 'self' should be inferred. If 'self' is the manifest's URI (which we've elsewhere ensured is HTTPS and pinned) then we're fine with schemeless hosts. Of course adding the scheme explicitly is always nice for clarity.

There are other strings that won't parse as nsIURI but should be allowed. "data:" would be perfectly safe for image-src, for example, but would be the equivalent of 'unsafe-inline' if used for script-src (or default-src if the policy doesn't explicitly set script-src).

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 8486443 [details] [diff] [review]
Bug_1059202-WIP-Run-local-check-for-pinning-on-launch.patch

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

Not really a review because I expect things to change, but this looks like a better approach.

::: b2g/app/b2g.js
@@ +246,5 @@
>  
>  pref("security.warn_viewing_mixed", false); // Warning is disabled.  See Bug 616712.
>  
> +// 2 = strict certificate pinning checks.
> +pref("security.cert_pinning.enforcement_level", 2);

I'm all for strict checking, but be aware this affects the entire FirefoxOS. I don't know if there's a way to install a corporate MITM root on a (non-rooted) Firefox OS device anyway so maybe the distinction between 1 and 2 is moot.

Comment 23

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

A domain without a scheme is certainly fine in CSP, however we know that such a rule would allow connections that don't use a pinned certificate, since such a rule allows non-https connections. I.e. such a rule should fail the "is guaranteed to use a pinned certificate" check.

self' for a THA is the origin of the manifest.

Comment 24

[Zoran Jovanovic]

(In reply to Jonas Sicking (:sicking) from comment #20)
> Comment on attachment 8483442 [details] [diff] [review]
> Bug_1059202-Certificate-verfication-for-Trusted-Host_pt3.patch
> 
> Review of attachment 8483442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Reverting Dan's review here given that we decided to go with the pinning
> check approach instead.
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +2399,5 @@
> >      // in which case we don't need to load it.
> >      if (app.manifest) {
> >        if (checkManifest()) {
> > +        if (this.kTrustedHosted == this.appKind(app, app.manifest)) {
> > +          if (!isValidTHACertificate(xhr.channel)) {
> 
> Couldn't you simply check "isHostPinned" before starting this request? Or
> even after the request if that's simpler.
> 
> @@ +2430,5 @@
> >          app.manifest = xhr.response;
> >          if (checkManifest()) {
> >            app.etag = xhr.getResponseHeader("Etag");
> > +          if (this.kTrustedHosted == this.appKind(app, app.manifest)) {
> > +            if (!isValidTHACertificate(xhr.channel)) {
> 
> Same here?

Absolutely. Will do.

Comment 25

[Zoran Jovanovic]

Attachment: 0001-WIP-Run-local-check-for-pinning-on-launch.patch

* Still using our crutch X509CertDB interface (will move to the proper one soon).
* Fixes most (if not all) comments here AND in Bug 1059221 (with help from Markus Nilsson), since the whole "verify CSP white list" thing is very much affected by this offline approach.

Comment 26

[Zoran Jovanovic]

Attachment: 0001-WIP-Run-local-check-for-pinning-on-launch.patch

Sorry, wrong WIP patch the last time.

Comment 27

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

Comment on attachment 8487298 [details] [diff] [review]
0001-WIP-Run-local-check-for-pinning-on-launch.patch

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

This is starting to look great.

For the final review, I think it'd be useful to just review a combined patch. And I think I'd prefer to have fabrice review at least the WebApps.jsm/ThaUtils.jsm changes, and Dan or Rbarnes review isHostPinned.

::: dom/apps/src/ThaUtils.jsm
@@ +48,5 @@
> +      debug("Host parsing failed: " + e);
> +      return false;
> +    }
> +
> +    if (!uri && !uri.host && "https" != uri.scheme) return false;

Maybe |if (!uri || !uri.host || uri.scheme != "https")|. I.e. fail safe rather than open.

Put the 'return' on a newline to follow gecko conventions.

@@ +90,5 @@
> +      directives
> +        .map(aDirective => aDirective.trim().split(" "))
> +        .filter(aList => aList.length > 1)
> +        // we only restrict on requiredDirectives
> +        .filter(aList => (-1 != requiredDirectives.indexOf(aList[0])))

I don't know if we have strict rules for this in Gecko, but I know most Gecko developers, me included, prefer

requiredDirectives.indexOf(aList[0]) != -1

But it's up to you. Same applies in a few other places below.

@@ +95,5 @@
> +        .forEach(aList => {
> +          // aList[0] contains the directive name.
> +          // aList[1..n] contains sources.
> +          let directiveName = aList[0];
> +          let sources = aList.slice(1);

Maybe:

let directiveName = aList.shift();
let sources = aList;

@@ +102,5 @@
> +            validDirectives.push(directiveName);
> +          }
> +          whiteList.push(...sources.filter(
> +             // 'self' is checked separately during manifest check
> +            aSource => (aSource !="'self'" && -1 == whiteList.indexOf(aSource))

This doesn't matter much either way. However this does not guarantee that the resulting whitelist only contains a source once. This due to that a source can in theory appear more than ones in a single directive. Though this is of course unlikely.

However this is fine since at worst we'd check for pinning of the same host twice. But since that's a fast operation I don't think that's a big deal.

@@ +108,5 @@
> +        });
> +
> +      // Check if all required directives are present.
> +      isValid = requiredDirectives
> +			  .every(aElement => (-1 != validDirectives.indexOf(aElement)));

Since you're already making sure to not add duplicates to validDirectives, you could just do

isValid = requiredDirectives.length === validDirectives.length;

Or you could remove the de-duplication logic above. Or let it be as-is. Up to you.

::: dom/apps/src/Webapps.jsm
@@ +1421,4 @@
>        }
>      }
> +
> +      // We have to clone the app object as nsIDOMApplication objects are

Indentation here doesn't look entirely right.

Comment 28

[Zoran Jovanovic]

Attachment: 0001-Bug-1059202-CSP-and-CA-verification-for-Trusted-Host.patch

* Obsoletes all other patches in this bug AND bug 1059221
* depends on bug 787133

Comment 29

[Zoran Jovanovic]

Also unit tests are coming up (tomorrow).

Comment 30

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

Comment 31

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

* replaced tako.com (existing domain) with example.com

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 8487987 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verification-for-Trusted-Host.patch

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

I may be misunderstanding some context, but given the apparently missing signature checks and the CSP problems I have to give this an r-.

::: b2g/app/b2g.js
@@ +246,5 @@
>  
>  pref("security.warn_viewing_mixed", false); // Warning is disabled.  See Bug 616712.
>  
> +// 2 = strict certificate pinning checks.
> +pref("security.cert_pinning.enforcement_level", 2);

This affects the entire device including user browsing. I suspect there's currently no UI for adding additional roots in which case this setting is equivalent to the existing defalt. But if people try to use their devices on a managed corporate network then pinned sites may not work.

::: dom/apps/ThaUtils.jsm
@@ +75,5 @@
> +   */
> +  getCSPWhiteList: function(aCsp) {
> +    let isValid = false;
> +    let whiteList = [];
> +    let requiredDirectives = [ "script-src", "style-src" ];

One gaping hole here: you don't require frame-src 'none'. This means the trusted app can frame anything, and if it frames a page on its own site that page will not be subject to the THA restrictions. It may not have a CSP, and even if it does the hosts mentioned in it will not be checked for pinning. Scripts in that frame can then reach up into the parent app and do whatever badness we were trying to prevent with pinning.

Does the actual CSP inherit the pref default CSP of "object-src 'none'"? I assume so, but if not then we need to verify that the manifest CSP contains it (and that's not done here).

@@ +126,5 @@
> +      debug("TRUSTED_APPLICATION_WHITELIST_PARSING_FAILED");
> +      return false;
> +    }
> +
> +    if (!domainWhitelist.list.every(aUrl => this.isHostPinned(aUrl))) {

If any of the csp directives are not https:// hosts then the whitelist will fail. There might be harmless ones like font-src 'none' in the list, or image-src which contains data: because there are some generated images. On the other hand if you don't fail on the non-https:// ones you could let script-src 'unsafe-inline' through. But while script-src 'unsafe-inline' or 'unsafe-eval' would be terrible, we've had to default our own CSP to allow STYLE-src 'unsafe-inline'. Unless your source is cleaner than ours you'll probably want to do that too, and these checks will fail that.

::: dom/apps/Webapps.jsm
@@ +2327,5 @@
> +            sendError("TRUSTED_APPLICATION_HOST_CERTIFICATE_INVALID");
> +            return;
> +          }
> +
> +          //if (!this.verifySignedManifest(aData.app, aData.appId)) {

Is this removing the manifest signature checks? If they're not here which patch now has them?

@@ +2367,5 @@
> +          if (this.kTrustedHosted == this.appKind(app, app.manifest)) {
> +            // checking trusted host for pinning is not needed here,
> +            // since network code will have already done that, but
> +            // verify signature of the manifest:
> +            //if (!this.verifySignedManifest(aData.app, aData.appId)) {

ditto.

Comment 33

[Zoran Jovanovic]

(In reply to Daniel Veditz [:dveditz] from comment #32)
> Comment on attachment 8487987 [details] [diff] [review]
> 0001-Bug-1059202-CSP-and-CA-verification-for-Trusted-Host.patch
> 
> Review of attachment 8487987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I may be misunderstanding some context, but given the apparently missing
> signature checks and the CSP problems I have to give this an r-.
> 
> ::: b2g/app/b2g.js
> @@ +246,5 @@
> >  
> >  pref("security.warn_viewing_mixed", false); // Warning is disabled.  See Bug 616712.
> >  
> > +// 2 = strict certificate pinning checks.
> > +pref("security.cert_pinning.enforcement_level", 2);
> 
> This affects the entire device including user browsing. I suspect there's
> currently no UI for adding additional roots in which case this setting is
> equivalent to the existing defalt. But if people try to use their devices on
> a managed corporate network then pinned sites may not work.

Still "man in the middle" enforcement level doesn't seem safe. Should we rely on default? It's important that the pins work for THA domains.

> ::: dom/apps/ThaUtils.jsm
> @@ +75,5 @@
> > +   */
> > +  getCSPWhiteList: function(aCsp) {
> > +    let isValid = false;
> > +    let whiteList = [];
> > +    let requiredDirectives = [ "script-src", "style-src" ];
> 
> One gaping hole here: you don't require frame-src 'none'. This means the
> trusted app can frame anything, and if it frames a page on its own site that
> page will not be subject to the THA restrictions. It may not have a CSP, and
> even if it does the hosts mentioned in it will not be checked for pinning.
> Scripts in that frame can then reach up into the parent app and do whatever
> badness we were trying to prevent with pinning.
Will add 'frame-src' to default csp.

> 
> Does the actual CSP inherit the pref default CSP of "object-src 'none'"? I
> assume so, but if not then we need to verify that the manifest CSP contains
> it (and that's not done here).
Yes, it does. Bug 1059194, patch 2.
> 
> @@ +126,5 @@
> > +      debug("TRUSTED_APPLICATION_WHITELIST_PARSING_FAILED");
> > +      return false;
> > +    }
> > +
> > +    if (!domainWhitelist.list.every(aUrl => this.isHostPinned(aUrl))) {
> 
> If any of the csp directives are not https:// hosts then the whitelist will
> fail. There might be harmless ones like font-src 'none' in the list, or
> image-src which contains data: because there are some generated images. On
> the other hand if you don't fail on the non-https:// ones you could let
> script-src 'unsafe-inline' through. But while script-src 'unsafe-inline' or
> 'unsafe-eval' would be terrible, we've had to default our own CSP to allow
> STYLE-src 'unsafe-inline'. Unless your source is cleaner than ours you'll
> probably want to do that too, and these checks will fail that.
Only 'script-src' and 'style-src' are checked here. All others fall through and are subject to default csp as declared in bug 1059194.
> 
> ::: dom/apps/Webapps.jsm
> @@ +2327,5 @@
> > +            sendError("TRUSTED_APPLICATION_HOST_CERTIFICATE_INVALID");
> > +            return;
> > +          }
> > +
> > +          //if (!this.verifySignedManifest(aData.app, aData.appId)) {
> 
> Is this removing the manifest signature checks? If they're not here which
> patch now has them?
Bug 1059216
> 
> @@ +2367,5 @@
> > +          if (this.kTrustedHosted == this.appKind(app, app.manifest)) {
> > +            // checking trusted host for pinning is not needed here,
> > +            // since network code will have already done that, but
> > +            // verify signature of the manifest:
> > +            //if (!this.verifySignedManifest(aData.app, aData.appId)) {
> 
> ditto.
Ditto.

Comment 34

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

> ::: b2g/app/b2g.js
> @@ +246,5 @@
> >  
> >  pref("security.warn_viewing_mixed", false); // Warning is disabled.  See Bug 616712.
> >  
> > +// 2 = strict certificate pinning checks.
> > +pref("security.cert_pinning.enforcement_level", 2);
> 
> This affects the entire device including user browsing. I suspect there's
> currently no UI for adding additional roots in which case this setting is
> equivalent to the existing defalt. But if people try to use their devices on
> a managed corporate network then pinned sites may not work.

If I understand it correctly, setting this to 2 just affects behavior if the user has installed additional roots on the device, is that correct?

If so, I think this is fine. We don't have any UI for installing additional roots as far as I know.

> ::: dom/apps/ThaUtils.jsm
> @@ +75,5 @@
> > +   */
> > +  getCSPWhiteList: function(aCsp) {
> > +    let isValid = false;
> > +    let whiteList = [];
> > +    let requiredDirectives = [ "script-src", "style-src" ];
> 
> One gaping hole here: you don't require frame-src 'none'. This means the
> trusted app can frame anything, and if it frames a page on its own site that
> page will not be subject to the THA restrictions. It may not have a CSP, and
> even if it does the hosts mentioned in it will not be checked for pinning.
> Scripts in that frame can then reach up into the parent app and do whatever
> badness we were trying to prevent with pinning.

I actually think we're fine here. If you <iframe> a same-origin page, we'll still consider that page to be part of the app, and so we'll still apply the default app CSP.

This would be very good to write a test for though to make sure it works as intended and doesn't regress in the future.

That said, adding a frame-src might be a good idea since it adds further protection from an attacker being able to inject UI. But it would also limit things like ads to only be loaded from pinned servers.

Comment 35

[Zoran Jovanovic]

> > ::: dom/apps/ThaUtils.jsm
> > @@ +75,5 @@
> > > +   */
> > > +  getCSPWhiteList: function(aCsp) {
> > > +    let isValid = false;
> > > +    let whiteList = [];
> > > +    let requiredDirectives = [ "script-src", "style-src" ];
> > 
> > One gaping hole here: you don't require frame-src 'none'. This means the
> > trusted app can frame anything, and if it frames a page on its own site that
> > page will not be subject to the THA restrictions. It may not have a CSP, and
> > even if it does the hosts mentioned in it will not be checked for pinning.
> > Scripts in that frame can then reach up into the parent app and do whatever
> > badness we were trying to prevent with pinning.
> 
> I actually think we're fine here. If you <iframe> a same-origin page, we'll
> still consider that page to be part of the app, and so we'll still apply the
> default app CSP.
> 
> This would be very good to write a test for though to make sure it works as
> intended and doesn't regress in the future.
> 
> That said, adding a frame-src might be a good idea since it adds further
> protection from an attacker being able to inject UI. But it would also limit
> things like ads to only be loaded from pinned servers.
Will add frame-src to default as soon as Fabrice OKs it (he's working on tests...).

Comment 36

[Zoran Jovanovic]

> > If any of the csp directives are not https:// hosts then the whitelist will
> > fail. There might be harmless ones like font-src 'none' in the list, or
> > image-src which contains data: because there are some generated images. On
> > the other hand if you don't fail on the non-https:// ones you could let
> > script-src 'unsafe-inline' through. But while script-src 'unsafe-inline' or
> > 'unsafe-eval' would be terrible, we've had to default our own CSP to allow
> > STYLE-src 'unsafe-inline'. Unless your source is cleaner than ours you'll
> > probably want to do that too, and these checks will fail that.
> Only 'script-src' and 'style-src' are checked here. All others fall through
> and are subject to default csp as declared in bug 1059194.
Will add more unit tests around this.

Comment 37

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

Improved a test case for optional directives.

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 8489282 [details] [diff] [review]
0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

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

r+ assuming you fix the trivial missing quotes

::: dom/apps/tests/test_tha_utils.html
@@ +146,5 @@
> +    }
> +  },{
> +    key: "isHostPinned doesn't allow 'unsafe-eval'",
> +    func: function test12() {
> +      let isHostPinned = ThaUtils.isHostPinned("unsafe-eval");

Missing the single quotes:  isHostPinned("'unsafe-eval'")
Ditto the next test for 'unsafe-inline'. Not that I expect it to change the test result here, but if we special-case one of these in the future it would matter.

Comment 39

[[:fabrice] Fabrice Desré]

Comment on attachment 8487987 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verification-for-Trusted-Host.patch

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

lgtm if Jonas & Dan agree on the security bits.

Comment 40

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

Single quote fix according to comment.

Comment 41

[[:fabrice] Fabrice Desré]

Zoran, no need to re-request review when you just fix the review comments and already got r+. (in this case, only Jonas didn't review).

Comment 42

[Zoran Jovanovic]

It's good to get another pair of eyes one more time. (Sorry for spam, though.)

Comment 43

[Richard Barnes [:rbarnes]]

Comment on attachment 8487987 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verification-for-Trusted-Host.patch

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

Overall:
* It would be better if THAs were identified by their properties rather than a token
* Don't do custom CSP parsing

::: b2g/app/b2g.js
@@ +246,5 @@
>  
>  pref("security.warn_viewing_mixed", false); // Warning is disabled.  See Bug 616712.
>  
> +// 2 = strict certificate pinning checks.
> +pref("security.cert_pinning.enforcement_level", 2);

I agree that as long as B2G doesn't have UI for installing certs, this probably isn't a big deal.  But if that ever changes, this should probably be revisited.  Should that be noted in a comment here?

::: dom/apps/AppsUtils.jsm
@@ +382,5 @@
>        return false;
>      }
> +
> +    // The 'csp' field is mandatory for Trusted Hosted Apps
> +    if (aManifest.type && (aManifest.type === "trusted") && !aManifest.csp) {

This code needs to identify hosted apps by virtue of the fact that they are hosted, not by identifier.  How does the app loader identify that an app is a THA, and thus load it from a remote host rather than locally?  Whatever triggers that behavior should also trigger this CSP check.

If that's not feasible, using the token "trusted" as the type value is insufficiently descriptive, since there are other apps that run at the same privilege level.  I would suggest taking the current value that corresponds to the level of privilege being granted, and decorate that with something like "_hosted".

::: dom/apps/ThaUtils.jsm
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please give this file a more descriptive name, or include its functions elsewhere (e.g., HostedCertifiedAppUtils.jsm).  As with AppsUtils.jsm, I would prefer if these were not treated as a separate category, but rather as a sub-class of the privilege class they belong to.

@@ +77,5 @@
> +    let isValid = false;
> +    let whiteList = [];
> +    let requiredDirectives = [ "script-src", "style-src" ];
> +
> +    if (aCsp) {

Custom parsing of the CSP directives is bad.  I haven't reviewed the below in detail, but it looks like it's got at least a couple problems (e.g., looking for the directive name at positions other than the start).  I've added an r? to ckershb in case he has a better ideas.

::: dom/apps/Webapps.jsm
@@ +1415,5 @@
>        return;
>      }
>  
> +    // Check if launching trusted hosted app
> +    if (this.kTrustedHosted == app.kind) {

Again, would prefer not to have this as a separate class, but rather do something like:

> if ((app.kind == this.kHosted) &&
>     (app.type == .appStatus >= Ci.nsIPrincipal.APP_STATUS_PRIVILEGED) {...}

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8487987 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verification-for-Trusted-Host.patch

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

> Custom parsing of the CSP directives is bad.  I haven't reviewed the below
> in detail, but it looks like it's got at least a couple problems (e.g.,
> looking for the directive name at positions other than the start).  I've
> added an r? to ckershb in case he has a better ideas.

Since we already have a CSP parser you can easily make use of it by calling appendPolicy().
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContentSecurityPolicy.idl#50

From what I read in the bug, you just need to check if you have valid 'script-src' and 'style-src' directive, right?
If so, we can extend the idl to include something like:

> boolean restrictsDirective(in AString aCSPDirective);

which returns whether the policy restricts that directive; meaning that directive is present and therefore valid in the policy.
Let me know, and I can incorporate that feature for you.

Comment 45

[[:fabrice] Fabrice Desré]

(In reply to Richard Barnes [:rbarnes] from comment #43)

> ::: dom/apps/AppsUtils.jsm
> @@ +382,5 @@
> >        return false;
> >      }
> > +
> > +    // The 'csp' field is mandatory for Trusted Hosted Apps
> > +    if (aManifest.type && (aManifest.type === "trusted") && !aManifest.csp) {
> 
> This code needs to identify hosted apps by virtue of the fact that they are
> hosted, not by identifier.  How does the app loader identify that an app is
> a THA, and thus load it from a remote host rather than locally?  Whatever
> triggers that behavior should also trigger this CSP check.

The manifest is the first thing we get when installing an app (ie, this is the parameter to mozApps.install()) so at this point we don't have any other information. This function is doing some sanity verification to prevent installation of apps with incorrect manifests - ie. we're assessing that the manifest is self consistent.

> If that's not feasible, using the token "trusted" as the type value is
> insufficiently descriptive, since there are other apps that run at the same
> privilege level.  I would suggest taking the current value that corresponds
> to the level of privilege being granted, and decorate that with something
> like "_hosted".

Trusted apps have the same privilege level as "normal" web apps from a nsIPrincipal point of view. So you would like to call them "web_trusted" ? I don't think that would be a big win, but I'm notoriously bad at naming things.


> ::: dom/apps/Webapps.jsm
> @@ +1415,5 @@
> >        return;
> >      }
> >  
> > +    // Check if launching trusted hosted app
> > +    if (this.kTrustedHosted == app.kind) {
> 
> Again, would prefer not to have this as a separate class, but rather do
> something like:
> 
> > if ((app.kind == this.kHosted) &&
> >     (app.type == .appStatus >= Ci.nsIPrincipal.APP_STATUS_PRIVILEGED) {...}

Trusted apps get an appStatus of Ci.nsIPrincipal.APP_STATUS_INSTALLED, so you can't tell them appart from "simple" hosted apps doing that. Initially there were patches adding a new status type, but we decided against doing that because it was very invasive in the codebase overall. But I agree that what we have is not idea either.

Comment 46

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

Comment on attachment 8489481 [details] [diff] [review]
0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

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

r=me with that fixed.

::: dom/apps/tests/test_tha_utils.html
@@ +55,5 @@
> +      let cspWhiteList = ThaUtils.getCSPWhiteList(
> +        "script-src https://script.example.com; style-src https://style.example.com"
> +      );
> +      ok(cspWhiteList.valid, "Should be valid");
> +      ok(cspWhiteList.list.length === 2, "List should have two sources");

Check that you are getting the domains you are expecting too. You might want to add a function which compares the contents of two arrays and checks that they are the same ignoring order. I.e. something like

ok(sameArray(cspWhiteList.list, ["https://script.example.com", "https://style.example.com"]), ...)

Same for most of the tests below too.

@@ +161,5 @@
> +  },{
> +    key: "isHostPinned doesn't allow foobar",
> +    func: function test14() {
> +      let isHostPinned = ThaUtils.isHostPinned("foobar");
> +      ok(!isHostPinned, "Should not be pinned:(" + isHostPinned + ") foobar");

Test with values like "https://*.example.com" and "example.com" and "http://example.com" to make sure that all of those are also considered non-pinned.

Comment 47

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

Comment on attachment 8487987 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verification-for-Trusted-Host.patch

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

Dan's and Fabrice's reviews are enough here I think.

Comment 48

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

- adds test for exact domains in the whitelist
- adds test for schema-less domain (although this was covered in 'foobar' test)

Comment 49

[Zoran Jovanovic]

(In reply to Jonas Sicking (:sicking) from comment #46)
> Comment on attachment 8489481 [details] [diff] [review]
> 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch
> 
> Review of attachment 8489481 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that fixed.
Done, well sort of, see below.

> ::: dom/apps/tests/test_tha_utils.html
> @@ +55,5 @@
> > +      let cspWhiteList = ThaUtils.getCSPWhiteList(
> > +        "script-src https://script.example.com; style-src https://style.example.com"
> > +      );
> > +      ok(cspWhiteList.valid, "Should be valid");
> > +      ok(cspWhiteList.list.length === 2, "List should have two sources");
> 
> Check that you are getting the domains you are expecting too. You might want
> to add a function which compares the contents of two arrays and checks that
> they are the same ignoring order. I.e. something like
> 
> ok(sameArray(cspWhiteList.list, ["https://script.example.com",
> "https://style.example.com"]), ...)
> 
> Same for most of the tests below too.
Done.

> @@ +161,5 @@
> > +  },{
> > +    key: "isHostPinned doesn't allow foobar",
> > +    func: function test14() {
> > +      let isHostPinned = ThaUtils.isHostPinned("foobar");
> > +      ok(!isHostPinned, "Should not be pinned:(" + isHostPinned + ") foobar");
> 
> Test with values like "https://*.example.com" and "example.com" and
> "http://example.com" to make sure that all of those are also considered
> non-pinned.
1. example.com was already verified with 'foobar' test, but I added it anyway.
2. test for http://example.com was already there
3. verification of https://*.example.com is done in pinning code that isHostPinned relies on, so we would need to setup a database with e.g. a pin for https://example.com and test against that database. Is that necessary? If so, I could use some guidance on how to set that test up.

Comment 50

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

Sorry for spam. Wrong patch in previous upload.

Comment 51

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

Minor fix: confusing message issued in some test cases

Comment 52

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

Ok, now it's the correct patch.

Comment 53

[[:fabrice] Fabrice Desré]

Comment on attachment 8487987 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verification-for-Trusted-Host.patch

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

Can you post a new version addressing the various comments?

::: dom/apps/Webapps.jsm
@@ +42,4 @@
>  Cu.import("resource://gre/modules/osfile.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://gre/modules/ThaUtils.jsm");

Switch to defineLazyModuleGetter here.

Comment 54

[Zoran Jovanovic]

Attachment: 0001-Bug-1059202-CSP-and-CA-verfication-for-Trusted-Hoste.patch

Refactoring of ThaUtils.jsm

Comment 55

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

Comment 56

[[:fabrice] Fabrice Desré]

Comment on attachment 8490953 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verfication-for-Trusted-Hoste.patch

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

r=me with nits fixed.

::: dom/apps/Webapps.jsm
@@ +2285,5 @@
> +//          if (!this.verifySignedManifest(aData.app, aData.appId)) {
> +//            debug("Manifest signature verification failed!");
> +//            sendError(aMessage);
> +//            return;
> +//          }

Don't put commented code there, but a comment pointing to the bug that adds the verification.

@@ +2324,5 @@
> +//            if (!this.verifySignedManifest(aData.app, aData.appId)) {
> +//              debug("Manifest signature verification failed!");
> +//              sendError(aMessage);
> +//              return;
> +//            }

here too.

Comment 57

[[:fabrice] Fabrice Desré]

Comment on attachment 8490953 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verfication-for-Trusted-Hoste.patch

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

What about https://bugzilla.mozilla.org/show_bug.cgi?id=1059202#c44 ?

Comment 58

[[:fabrice] Fabrice Desré]

Comment on attachment 8490954 [details] [diff] [review]
0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

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

Sorry for not catching the s/ok/is earlier. That's great if you can make the change, but not the highest priority.

::: dom/apps/tests/test_tha_utils.html
@@ +15,5 @@
> +    key: "getCSPWhiteList with no argument",
> +    func: function test1() {
> +      let cspWhiteList = TrustedHostedAppsUtils.getCSPWhiteList();
> +      ok(!cspWhiteList.valid, "Should be invalid");
> +      ok(cspWhiteList.list.length === 0, "List should be empty");

it's slightly better to do is(cspWhiteList.list.length, 0, "List should be empty") because the test harness prints the observed and the expected value if the test fails.

Comment 59

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

Comment on attachment 8490954 [details] [diff] [review]
0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

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

r=me with the added test.

::: dom/apps/tests/test_tha_utils.html
@@ +159,5 @@
> +    }
> +  },{
> +    key: "isHostPinned doesn't allow shema-less urls",
> +    func: function test13() {
> +      let isHostPinned = TrustedHostedAppsUtils.isHostPinned("example.com");

Still missing a test for "https://*.example.com" since that is valid CSP syntax, but that we should always return false for.

Comment 60

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

Something else that I realized is that CSP rules like "https://example.com:*" is valid CSP, and something that we could actually verify pinning for. But I think the current isHostPinned() code will always return false for.

This isn't a security problem. It just means that a CSP rule that contains such an entry will cause installation to fail.

I think this is fine, but I wanted to call it out in case it was something that you guys had planned to use.

Comment 61

[Zoran Jovanovic]

(In reply to Jonas Sicking (:sicking) from comment #59)
> Comment on attachment 8490954 [details] [diff] [review]
> 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch
> 
> Review of attachment 8490954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the added test.
> 
> ::: dom/apps/tests/test_tha_utils.html
> @@ +159,5 @@
> > +    }
> > +  },{
> > +    key: "isHostPinned doesn't allow shema-less urls",
> > +    func: function test13() {
> > +      let isHostPinned = TrustedHostedAppsUtils.isHostPinned("example.com");
> 
> Still missing a test for "https://*.example.com" since that is valid CSP
> syntax, but that we should always return false for.
No reason if it's pinnable. If not pinnable, tests for pinning should catch it.

Comment 62

[Zoran Jovanovic]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #44)
> Comment on attachment 8487987 [details] [diff] [review]
> 0001-Bug-1059202-CSP-and-CA-verification-for-Trusted-Host.patch
> 
> Review of attachment 8487987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > Custom parsing of the CSP directives is bad.  I haven't reviewed the below
> > in detail, but it looks like it's got at least a couple problems (e.g.,
> > looking for the directive name at positions other than the start).  I've
> > added an r? to ckershb in case he has a better ideas.
> 
> Since we already have a CSP parser you can easily make use of it by calling
> appendPolicy().
> http://mxr.mozilla.org/mozilla-central/source/content/base/public/
> nsIContentSecurityPolicy.idl#50
> 
> From what I read in the bug, you just need to check if you have valid
> 'script-src' and 'style-src' directive, right?
> If so, we can extend the idl to include something like:
> 
> > boolean restrictsDirective(in AString aCSPDirective);
> 
> which returns whether the policy restricts that directive; meaning that
> directive is present and therefore valid in the policy.
> Let me know, and I can incorporate that feature for you.
We have tried that approach but were advised against as it was deemed too intrusive, since this restriction is very specific - the required directives 'script-src' and 'style-src' can only contain https domains with CA pins and 'self', which itself MUST be a pinned https domain. This means that we would still need to extract the domains out of csp element and check if they are pinned.
(Sure, we can do that, but let's do it in next iteration and work on it for the next landing window.)

Comment 63

[Zoran Jovanovic]

(In reply to Zoran Jovanovic from comment #61)
> (In reply to Jonas Sicking (:sicking) from comment #59)
> > Comment on attachment 8490954 [details] [diff] [review]
> > 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch
> > 
> > Review of attachment 8490954 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with the added test.
> > 
> > ::: dom/apps/tests/test_tha_utils.html
> > @@ +159,5 @@
> > > +    }
> > > +  },{
> > > +    key: "isHostPinned doesn't allow shema-less urls",
> > > +    func: function test13() {
> > > +      let isHostPinned = TrustedHostedAppsUtils.isHostPinned("example.com");
> > 
> > Still missing a test for "https://*.example.com" since that is valid CSP
> > syntax, but that we should always return false for.
> No reason if it's pinnable. If not pinnable, tests for pinning should catch
> it.
And apparently I should brush up on my URI syntax, since Services.io.newURI(aUrl, null, null) call on this and the other URI you pointed out throws NS_ERROR_MALFORMED_URI. Couple of tests coming right up.

Comment 64

[Zoran Jovanovic]

Attachment: 0001-Bug-1059202-CSP-and-CA-verfication-for-Trusted-Hoste.patch

Comment 65

[Zoran Jovanovic]

Attachment: 0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

Comment 66

[Richard Barnes [:rbarnes]]

Comment on attachment 8491488 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verfication-for-Trusted-Hoste.patch

Zoran: Please do not drop reviewers on updated patches.  Especially when the reviewer previously set r-, it looks like evasion.

Comment 67

[Zoran Jovanovic]

(In reply to Richard Barnes [:rbarnes] from comment #66)
> Comment on attachment 8491488 [details] [diff] [review]
> 0001-Bug-1059202-CSP-and-CA-verfication-for-Trusted-Hoste.patch
> 
> Zoran: Please do not drop reviewers on updated patches.  Especially when the
> reviewer previously set r-, it looks like evasion.

I apologize. It wasn't intentional. (Obsoleting a patch removes all, so then I must manually add reviewers back and that is error prone - not that it's a good excuse, but still...)

Comment 68

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #44)
> From what I read in the bug, you just need to check if you have valid
> 'script-src' and 'style-src' directive, right?
> If so, we can extend the idl to include something like:

No, what we need to do is to extract a list of all the domains that a given CSP policy allows scripts/css to be loaded from. And what protocols can be used to those domains. So if a CSP directive says:

"script-src: https://example.com http://*.bar.com; font-src: wherever.com http://example.com"

Then we want to know that scripts can be loaded from "example.com" using https and from "*.bar.com" using http. And that there are no restrictions on where css can be loaded from.

The current approach doesn't do this perfectly, but where it fails, it does so in a restrictive way so that's fine for now.

I do agree that this could be accomplished by adding new API to nsIContentSecurityPolicy, however I think it require a fair amount of complexity to do so. Given that this capability is something that's only needed by the THA code, I'd rather not add too much complexity to nsIContentSecurityPolicy and its implementation.

The other thing to keep in mind here is that we don't need to be able to parse any policies that can be expressed. It's fine if only a subset works as long as we fail safe (which I believe the current code does). The only policies that we'll handle here are ones that appear in the manifest of THAs, so if we have some limitations on what policies can be expressed there then that's fine.

Comment 69

[Richard Barnes [:rbarnes]]

Comment on attachment 8491488 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verfication-for-Trusted-Hoste.patch

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

::: b2g/app/b2g.js
@@ +246,5 @@
>  
>  pref("security.warn_viewing_mixed", false); // Warning is disabled.  See Bug 616712.
>  
> +// 2 = strict certificate pinning checks.
> +pref("security.cert_pinning.enforcement_level", 2);

Please add a comment here of the following character:
"""
This default preference is more strict than Firefox because B2G currently does not have a way to install local root certificates.  Strict checking is effectively equivalent to non-strict checking as long as that is true.  If an ability to add local certificates is added, there may be a need to change this pref.
"""

::: dom/apps/TrustedHostedAppsUtils.jsm
@@ +48,5 @@
> +      debug("Host parsing failed: " + e);
> +      return false;
> +    }
> +
> +    if (!uri || !uri.host || "https" != uri.scheme) {

Is it possible for !uri to be true if you get to here?  That is, will Services.io.newURI() return null without raising an exception?

Also, in the long run this should probably call nsSiteSecurityService.isSecureURI().  (Doesn't make sense to do it now, because that method doesn't yet support HPKP.)  Please add a TODO comment and file a bug.

@@ +68,5 @@
> +      debug("\tvalid certificate pinning for host: " + uri.host + "\n");
> +      return true;
> +    }
> +
> +    debug("\Host NOT pinned: " + uri.host + "\n");

"\tHost"

@@ +94,5 @@
> +        .map(aDirective => aDirective.trim().split(" "))
> +        .filter(aList => aList.length > 1)
> +        // we only restrict on requiredDirectives
> +        .filter(aList => (requiredDirectives.indexOf(aList[0]) != -1))
> +        .forEach(aList => {

As I said in my previous review, having independent CSP parsing here is really hard to maintain.  However, nsIContentSecurityPolicy doesn't quite do what you need, so it's not possible to use that either.  Please add a comment "TODO: Use nsIContentSecurityPolicy" and open a bug.

::: dom/apps/Webapps.jsm
@@ +1403,5 @@
> +        aOnFailure("TRUSTED_APPLICATION_HOST_CERTIFICATE_INVALID");
> +        return;
> +      }
> +
> +      debug("Trusted App Host certificate OK");

This is not what you just checked.  "Trusted App Host pins exist."  Likewise for the debug statement and comments above.

Comment 70

[[:fabrice] Fabrice Desré]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=2b858842646e

Comment 71

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

(In reply to Richard Barnes [:rbarnes] from comment #69)
> ::: dom/apps/TrustedHostedAppsUtils.jsm
> @@ +48,5 @@
> > +      debug("Host parsing failed: " + e);
> > +      return false;
> > +    }
> > +
> > +    if (!uri || !uri.host || "https" != uri.scheme) {
> 
> Is it possible for !uri to be true if you get to here?  That is, will
> Services.io.newURI() return null without raising an exception?

I think we can remove the !uri check here yes.

Comment 72

[Zoran Jovanovic]

Attachment: 0001-Bug-1059202-CSP-and-CA-verfication-for-Trusted-Hoste.patch

r+ by :jonas and :rbarnes

Comment 73

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #68)
> I do agree that this could be accomplished by adding new API to
> nsIContentSecurityPolicy, however I think it require a fair amount of
> complexity to do so. Given that this capability is something that's only
> needed by the THA code, I'd rather not add too much complexity to
> nsIContentSecurityPolicy and its implementation.

Probably you are right, and it adds too much complexity to nsIContentSecurityPolicy, but we could introduce a new idl 'CSPforTHA' that allows you:
* parse a Policy
* query https script-srcs
* query http script-srcs
* etc.
that is customized for THA code and can internally use the underlying CSP parser and custom classes. From what I read, the current CSP implementation provides all the information in an encapsulated way. We just need an interface to query it.

> The other thing to keep in mind here is that we don't need to be able to
> parse any policies that can be expressed. It's fine if only a subset works
> as long as we fail safe (which I believe the current code does).

Duplicating logic always sounds scary to me and parsing CSP is not as trivial as it looks at first. We already invested the time to establish a comprehensive testsuite, see:
http://mxr.mozilla.org/mozilla-central/source/content/base/test/TestCSPParser.cpp#173

Why not invest the time now to establish that interface now?

Also adding Sid, to get his opinion.

Comment 74

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/07cdb131dccb
https://hg.mozilla.org/integration/b2g-inbound/rev/898325f3fac6

Comment 75

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #73)

> Probably you are right, and it adds too much complexity to
> nsIContentSecurityPolicy, but we could introduce a new idl 'CSPforTHA' that
> allows you:
> * parse a Policy
> * query https script-srcs
> * query http script-srcs
> * etc.
> that is customized for THA code and can internally use the underlying CSP
> parser and custom classes. From what I read, the current CSP implementation
> provides all the information in an encapsulated way. We just need an
> interface to query it.

It's not clear to me that this would be less complexity than duplicating the parsing logic. At the very least it doesn't seem better enough that we should hold the initial landing. Especially given that this is blocking the 2.1 release which means that we're already uplifting uncomfortably much code to the aurora branch.

Comment 76

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/6249ca86187a

Comment 77

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07cdb131dccb
https://hg.mozilla.org/mozilla-central/rev/898325f3fac6
https://hg.mozilla.org/mozilla-central/rev/6249ca86187a

Comment 78

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/63d172ff422b
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/be73288ec122
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b96413e1b921

Comment 79

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

Comment on attachment 8491489 [details] [diff] [review]
0002-Bug-1059202-Unit-test-CSP-and-CA-verfication-for-Tru.patch

got a=bajaj over email

Though note that an updated patch is needed here to fix the problem in chrome.ini

Comment 80

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

Comment on attachment 8491865 [details] [diff] [review]
0001-Bug-1059202-CSP-and-CA-verfication-for-Trusted-Hoste.patch

got a=bajaj over email

But don't check in the all.js change here.