False positive on "content_security_policy" allows remote code execution in manifest.json

[erosman]

ref: https://reviewers.addons.mozilla.org/en-US/firefox/files/browse/1026521/file/manifest.json#top

Is this a false positive?

"content_security_policy": "default-src * 'self' 'unsafe-inline' *; script-src 'self' 'unsafe-eval'; object-src 'self'; child-src * 'self' 'unsafe-inline' 'unsafe-eval';"

┆Issue is synchronized with this Jira Task

[rpl]

In my opinion it is not a false positive, using the modified CSP the extension will be allowed to evaluate strings of code (e.g. using eval or Function), which as a side-effect it also means that the extension can take some external piece of code (e.g. by retrieving it with an HTTP request) and then evaluate it in an extension page (e.g. in a background page using eval).

@erosman Does the extension use some kind of JS template engine that may be using the eval to transpile the templates at runtime? (in that case an alternative to the customized CSP is to "precompile" the templates during a build step).

[erosman]

@rpl In above case, the linter reported that CSP contains "remote code execution" and that is NOT the case. It is falsely reporting that "content_security_policy" allows remote code execution in manifest.json

Warning: "content_security_policy" allows remote code execution in manifest.json

A custom content_security_policy needs additional review.

The case for 'unsafe-eval' (which is also reported by the linter and will be checked by the reviewers) is a separate issue from above and not the subject of this bug report.

@wagnerand should be able to clarify it.

[rpl]

we (@wagnerand and I) took a deeper look at the code that sets this warning and it looks that the part of the CSP string that is triggering the warning is default-src * 'self' 'unsafe-inline' *;, which will set insecureSrcDirective to true here:

• https://github.com/mozilla/addons-linter/blob/a62ed798c55bf100b6bb06862dd64097c6740cef/src/parsers/manifestjson.js#L482-L485

Then insecureSrcDirective can be set back to false only if the parser finds a script-src 'self'; directive, which is checked here:

• https://github.com/mozilla/addons-linter/blob/a62ed798c55bf100b6bb06862dd64097c6740cef/src/parsers/manifestjson.js#L435-L444

And so "content_security_policy": "default-src * 'self' 'unsafe-inline' *; script-src 'self'; object-src 'self'; ..." doesn't trigger the warning, but the warning is always shown if script-src has more then one value.

[EnTeQuAk]

@rpl @wagnerand thanks a lot for looking into that, that should indeed be fixed then

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

[wagnerand]

We should get to this sooner rather than later /cc @willdurand

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.