Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSP without default-src is incorrectly detected as secure #4578

Closed
Rob--W opened this issue Nov 8, 2022 · 2 comments · Fixed by #4594
Closed

CSP without default-src is incorrectly detected as secure #4578

Rob--W opened this issue Nov 8, 2022 · 2 comments · Fixed by #4594

Comments

@Rob--W
Copy link
Member

Rob--W commented Nov 8, 2022

Describe the problem and steps to reproduce it:

The CSP validator's primary purpose is to reject CSP policies that permit remote code.
script-src is the primary directive to control that, and if that is not specified, default-src.
The validator correctly handles these two cases. But if default-src is missing, the default fallback is to be very permissive. This should be flagged as insecure CSP.

Test case, create manifest.json and lint the directory containing the following:

{
  "name": "x",
  "version": "1",
  "manifest_version": 2,
  "content_security_policy": "img-src 'none'"
}

What happened?

No warnings.

What did you expect to happen?

MANIFEST_CSP warning should be emitted, because this policy allows remote code.

Note: Firefox has a different CSP parser. Its current behavior given the above CSP is to ignore it and use the default CSP that rejects remote scripts. While that matches our intent, the rejection of the CSP means that the extension developer's intent to reject images (img-src 'none') is not respected.

Anything else we should know?

This issue happens because the implementation assumes that the CSP is secure by default, and rejects when an insecure directive is encountered. An empty CSP is not secure by default, which results in this bug.

@AlexandraMoga
Copy link

What happened?

MANIFEST_CSP warning should be emitted, because this policy allows remote code.

What did you expect to happen?

No warnings.

@Rob--W I think the expected vs actual results are inverted in your STR.

The old behavior, as seen on prod currently, is to emit no warnings for the mentioned CSP:
image

After the fix (linter 5.22.0), we are raising a warning, which should be the new expected behavior:
image

@Rob--W
Copy link
Member Author

Rob--W commented Nov 16, 2022

What happened?

MANIFEST_CSP warning should be emitted, because this policy allows remote code.

What did you expect to happen?

No warnings.

@Rob--W I think the expected vs actual results are inverted in your STR.

Thanks, I have edited the post and swapped the two. The verified behavior that you have observed is as desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants