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.

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 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 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 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 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 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 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 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

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