Skip to content

CSP "none" combined with other sources #913

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.

Sign up for GitHub

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

Jump to bottom
Closed
baknu opened this issue Mar 24, 2023 · 7 comments · Fixed by #993
Closed

CSP "none" combined with other sources #913

baknu opened this issue Mar 24, 2023 · 7 comments · Fixed by #993
Assignees
@mxsasha @baknu
Milestone
v1.8

Comments

@baknu
Copy link
Contributor

baknu commented Mar 24, 2023 •
edited
Loading

Item 2 of the following comment states how Internet.nl now handles CSP "none" when combined with other sources, according to @gthess: #325 (comment)
Below is the relevant quote:

Technically no. But browsers do not raise an error on that. The existence of 'none' guarantees that the directive will result in an empty set of sources, regardless of what else is specified in the directive. We follow that and also allow 'none' to coexist with other syntactically.

In the CSP specification, however, I read the following:

NOTE: The 'none' keyword has no effect when other source expressions are present. That is, the list « 'none' » does not match any URL. A list consisting of « 'none', https://example.com/ », on the other hand, would match https://example.com/.

Source: https://www.w3.org/TR/CSP3/#match-url-to-source-list

So our CSP subtest does not seem to be fully in line with the CSP specification.
@baknu baknu added this to the v1.8 milestone Mar 24, 2023
@baknu baknu changed the title CSP "none" combined with orther sources CSP "none" combined with other sources Mar 24, 2023
@baknu
Copy link
Contributor Author

baknu commented Mar 26, 2023

See also: https://csplite.com/csp101/

@gthess
Copy link
Collaborator

gthess commented Mar 27, 2023

Indeed, in version 3 they have more clear/explicit text about it.
It seems I misread version 2

If source list is an ASCII case-insensitive match for the string 'none' (including the quotation marks), return the empty set.

and understood that matching none returns the empty set, which is wrong now that I read it again.

@baknu
Copy link
Contributor Author

baknu commented Mar 27, 2023

@gthess: Thanks for your double checking!

@bwbroersma
Copy link
Collaborator

browsers do not raise an error on that

After a discrepancy between specs & implementation of IPv4 parsing I nowadays always check the CSP implementations in Mozilla & Chromium .

<head>
<meta http-equiv="Content-Security-Policy" content="default-src 'none' 'self'">
</head>

Chromium:

❌ The Content-Security-Policy directive 'default-src' contains the keyword 'none' alongside with other source expressions. The keyword 'none' must be the only source expression in the directive value, otherwise it is ignored.

The parseSource call will return false if there is more than 'none', and then error.

Mozilla:

⚠️ Content Security Policy: Ignoring unknown option 'none'

The sourceList has a special case for handling none.

So the main browsers do conform to the spec here (triple check ✔️).

@mxsasha
Copy link
Collaborator

mxsasha commented May 1, 2023

I think our code may already do this. I can not reproduce the behavior where we the presence of none with other, invalid things, incorrectly approves the policy.

Valid: form-action 'none'; base-uri 'none'; default-src 'self'; frame-ancestors 'self'

Invalid variations:

  • form-action 'none' example.nl; base-uri 'none'; default-src 'self'; frame-ancestors 'self'
  • form-action 'none'; base-uri 'none example.nl'; default-src 'self'; frame-ancestors 'self'
  • form-action 'none'; base-uri 'none'; default-src 'none' example.nl; frame-ancestors 'self'

@mxsasha mxsasha added the discuss Requires further team discussion and decisions label May 1, 2023
@mxsasha
Copy link
Collaborator

mxsasha commented May 2, 2023

Agreed that this now works how it should, I will check whether we cover this in the tests.

@mxsasha mxsasha removed the discuss Requires further team discussion and decisions label May 16, 2023
@mxsasha
Copy link
Collaborator

mxsasha commented Jun 7, 2023

I think our code may already do this. I can not reproduce the behavior where we the presence of none with other, invalid things, incorrectly approves the policy.

I think this was a incorrect and misinterpreted based on testing the code with #810. Anyways, #993 adds both this feature and a test for it.

@mxsasha mxsasha linked a pull request Jun 7, 2023 that will close this issue
Fix #913 - Handle 'none' mixed with other sources correctly in CSP test #993
Merged
mxsasha added a commit that referenced this issue Jun 7, 2023
@mxsasha
Fix #913 - Handle 'none' mixed with other sources correctly in CSP test
e2bd955
mxsasha added a commit that referenced this issue Jun 7, 2023
@mxsasha
Fix #913 - Handle 'none' mixed with other sources correctly in CSP te…
14bcd46 
…st (#993)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mxsasha mxsasha

@baknu baknu

Labels
None yet
Milestone
v1.8
4 participants
@mxsasha @bwbroersma @baknu @gthess