1059221 - CSP policies in Trusted Hosted Apps

Status: RESOLVED INVALID

(Core Graveyard :: DOM: Apps, defect)

Description

[Mattias Östergren]

A trusted hosted app has a default CSP (allow all content, reject all objects, reject inlining, reject eval) and MUST declare a csp element in the manifest.

Comment 1

[Mattias Östergren]

Implement and enable CSP policy checks for THA.

A trusted hosted app has a default CSP (allow all content, reject all objects, reject inlining, reject eval) and MUST declare a csp element in the manifest.

Comment 2

[Vlatko Markovic]

Attachment: Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt1.patch

Comment 3

[Vlatko Markovic]

Attachment: Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt2.patch

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8481397 [details] [diff] [review]
Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt1.patch

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

::: dom/apps/src/Webapps.jsm
@@ +4580,5 @@
> +    let requiredDirectives = [ "script-src" , "style-src" ];
> +
> +    if (aCsp) {
> +      status = requiredDirectives.every(function(element) {
> +        return (-1 != aCsp.indexOf(element));

That will also match if the csp is something like:
|script-src "http://foo.com/style-src"|

I don't believe that parsing the CSP like this is done here is sufficient.

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8481398 [details] [diff] [review]
Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt2.patch

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

Clearing review until we have a clearer idea about how to get the csp whitelist.

Comment 6

[Vlatko Markovic]

Attachment: Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt1.patch

Comment 7

[Vlatko Markovic]

Attachment: Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt2.patch

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8483452 [details] [diff] [review]
Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt2.patch

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

These 3 blocks are very similar. Can you refactor to make them share a common function?

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8483451 [details] [diff] [review]
Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt1.patch

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

I'd like to see an updated version, and also unit tests for verifyCSPWhiteList().
Since these functions are THA specific and don't rely on anything in Webapps.jsm, we can move them to their own jsm (eg. ThaUtils.jsm).

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

nit: full stop at the end of the comment.

::: dom/apps/src/Webapps.jsm
@@ +4564,5 @@
> +  /**
> +   * Returns Promise which resolves if host was successfully validated.
> +   * The Promise rejects on any error.
> +   */
> +  verifyURL: function verifyURL(url) {

nit: s/url/aUrl

@@ +4628,5 @@
> +  verifyCSPWhiteList: function(aWhiteList) {
> +    let resultArray = [];
> +    let verifyPromises = [];
> +
> +    for (let i = 0; i < aWhiteList.length; i++) {

aWhiteList.forEach(...)

@@ +4650,5 @@
> +   */
> +  getCSPWhiteList: function(aCsp) {
> +    let status = false;
> +    let whiteList = new Array();
> +    let requiredDirectives = [ "script-src" , "style-src" ];

nit: there's an extra space before ','

@@ +4654,5 @@
> +    let requiredDirectives = [ "script-src" , "style-src" ];
> +
> +    if (aCsp) {
> +      let directives = new Array();
> +      let directive = aCsp.split(";");

the naming here is a bit confusing. What about:
directives -> validDirectives
directive -> directives

@@ +4655,5 @@
> +
> +    if (aCsp) {
> +      let directives = new Array();
> +      let directive = aCsp.split(";");
> +      for (let i = 0; i < directive.length; i++) {

directive.forEach(...)

@@ +4659,5 @@
> +      for (let i = 0; i < directive.length; i++) {
> +        let sourceList = directive[i].trim().split(" ");
> +
> +        /* sourceList[0] should contain the directive name.
> +         * Check if the lentgh is > 1 (contain at least directive and source)

nit: length.

@@ +4676,5 @@
> +        }
> +      }
> +
> +      /* Check if all required directives are present. */
> +      status = requiredDirectives.every(function(element) {

nit: every(aElement => {...})

@@ +4684,5 @@
> +      if (!status) {
> +        debug("White list doesn't contain all required directives!");
> +        while(whiteList.length > 0) {
> +          whiteList.pop();
> +        }

whiteList = [];

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 8483452 [details] [diff] [review]
Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt2.patch

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

r=dveditz

Comment 11

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

Comment on attachment 8483451 [details] [diff] [review]
Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt1.patch

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

::: dom/apps/src/Webapps.jsm
@@ +4624,5 @@
> +  /**
> +   * Returns Promise which resolves if all hosts in the whitelist were
> +   * successfully validated. Otherwise it will reject.
> +   */
> +  verifyCSPWhiteList: function(aWhiteList) {

Repeating review comment from bug 1016421 comment 112. Though hopefully this function can be greatly simplified, and made synchronous, if we can peek directly into the certificate pinning database instead.


This whole function can be written as:

return Promise.all(aWhiteList.map(this.verifyURL.bind(this)));

Technically you can even leave out ".bind(this)", but that might result in surprising behavior if we make further changes to verifyURL later.

Actually, it might be better to make verifyURL be a scoped function inside verifyCSPWhiteList since it's behavior is quite specific to verifyCSPWhiteList, but its current name is not. That way you can also leave out the .bind() call.

@@ +4670,5 @@
> +        /* Start getting sources with index 1. */
> +        for (let j = 1; j < sourceList.length; j++) {
> +          /* Add only if the host is not in the list already. */
> +          if (-1 == whiteList.indexOf(sourceList[j])) {
> +            whiteList.push(sourceList[j]);

This gives a bit of a strange behavior.

It means that if you do specify an img-src rule, that it enforces that images are only loaded from certificate-pinned sources. However if you don't specify an img-src rule, images can be loaded from anywhere.

Another effect if that if you restrict image loading through the manifest CSP, that it enforces that images are only loaded from pinned websites.

However if you restrict image loading through a http header, you do not get that behavior.

Is this expected? Same thing goes with not just images, but also data connections (such as XMLHttpRequest), fonts, etc.

I'm fine with this behavior, but I wanted to point it out in case it wasn't expected.

You should probably also filter out '*', 'self', 'unsafe-eval' and 'unsafe-inline'. Obviously treating those as domains will not work.

Comment 12

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

Comment on attachment 8483452 [details] [diff] [review]
Bug_1059221-CSP-policies-in-Trusted-Hosted-Apps-part_pt2.patch

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

This doesn't seem to have addressed the comments from bug 1016421 comment 113

Comment 13

[Mattias Östergren]

The code of these patches are squashed into patch of bug 1059202.