Problem/Motivation

My understanding is that the only valid sources for script-src-attr and style-src-attr are 'none', 'unsafe-inline', or 'unsafe-hashes' (+ corresponding hashes) - any network sources can be removed.

If the *-attr directive matches a fallback it should continue to be omitted from the policy, but if it contains a unique value it should be shortened.

Proposed resolution

Add another helper method for Csp::getHeaderValue() to handle *-attr-specific optimizations that removes any network sources for the policy.
This optimization should only be applied if the sources do not match a fallback directive, which would allow omitting the directive completely.

Input Output Notes
script-src-attr 'self' 'unsafe-inline' https://example.com script-src-attr 'unsafe-inline'
script-src-attr 'self' 'unsafe-hashes' 'sha256-abcde' script-src-attr 'unsafe-hashes' 'sha256-abcde'
script-src-attr 'self' 'unsafe-inline' 'unsafe-hashes' 'sha256-abcde' script-src-attr 'unsafe-hashes' 'sha256-abcde' CSP Level 2 browsers don't support *-attr; hashes should disable 'unsafe-inline' in CSP Level 3 browser
script-src-attr 'self' https://example.com script-src-attr 'none'
script-src-attr * script-src-attr 'none'
script-src-attr 'sha256-abcde' script-src-attr 'none' Omit hashes if 'unsafe-hashes' is not present
script-src 'self' https://example.com; 
script-src-attr  'self' https://example.com
script-src 'self' https://example.com
script-src 'self' https://example.com; 
script-src-attr 'self' 'unsafe-inline' https://example.com
script-src 'self' https://example.com; 
script-src-attr 'unsafe-inline'
script-src 'self' 'unsafe-inline' https://example.com; 
script-src-attr  'self'
script-src 'self' 'unsafe-inline' https://example.com; 
script-src-attr 'none'

Remaining tasks

Comments

gapple created an issue. See original summary.

gapple’s picture

Issue summary: View changes

I think script-src-attr 'self' would also be equivalent to script-src-attr 'none'.

If the site configuration is set to 'none' it will override any dynamic changes (which would currently break CKEditor). If it's set to n/a it may be omitted and fallback, enabling unwanted sources. I think this makes keeping 'self' as an option in the UI necessary, but the resulting policy would not need to include it.

  • gapple committed 20b79b9 on 8.x-1.x
    Issue #3166835: Optimize attribute directives
    
gapple’s picture

I think most cases listed in the description should now be covered, except for 'self'

egranty’s picture

If the *-attr directive matches a fallback it should continue to be omitted from the policy

It case of omitting it's sufficiently reduce the secirity:

script-src 'unsafe-inline'; 
script-src-attr 'unsafe-inline';

does allow only inline event handlers and javascript-navigation in the support browsers, while:

script-src 'unsafe-inline'; 

does allow any inline scripts including <script>...</script> .

but if it contains a unique value it should be shortened.

Yes, you can remove unsupported sources, but who know full list of these?
As for now the 'report-sample' is missed, but what happens in future? Who will track CSP spec changes and do support tests across all browsers?
I think it's better just to throw diag message on invalid sources. Any enforced modifying userd CSP leads to responcibility if something gies wrong.

gapple’s picture

@egranty
In your example:

script-src 'unsafe-inline'; 
script-src-attr 'unsafe-inline';

The script-src-elem directive is not explicitly defined, so it will fall back to the value of the script-src directive. As such the three directives are equivalent to just script-src 'unsafe-inline', and both inline code blocks (<script>inlineFunction()</script>) and event attributes (<a onclick="inlineFunction()">Click me!</a>), will be permitted. In order to only allow inline attributes and not inline code blocks, script-src-elem would need to be defined - in this case with 'none'.

A more expected real world example would include a host-source for script-src-elem (and in script-src for browsers that don't yet support Level 3):

script-src 'self' 'unsafe-inline'; 
script-src-elem 'self';
script-src-attr 'unsafe-inline';

The previous example could not be automatically optimized (since script-src-attr doesn't include 'self'), but would be effectively equivalent to:

script-src 'self' 'unsafe-inline'; 
script-src-elem 'self';

Since the not explicitly defined script-src-attr would fall back to the value of script-src.

----

Your point that spec changes could cause issues is important to consider in what and how optimizations are done. I do watch the CSP spec for changes, and for questions that others have about the spec to ensure I have a solid understanding of it and this module aligns to it as best as possible. Current optimizations are only done where they are reasonably safe, have a clear set of cases to test against, and are actually likely to be encountered and provide a benefit (e.g. I see you've also commented on the issue about optimizing host-source protocols - I don't think that it's something that will typically occur, it's probably something a user should address if it does, and there's not a big benefit compared to the complexity, so it's not likely to be implemented).

The current implementation for optimizing attribute directives only removes values that are known to not have an affect when included in the directive. If new keywords are introduced to the spec they will be allowed by default and it's not possible to introduce a source value to the spec that's not a quoted keyword, so I don't think there's a concern regarding this particular optimization.
https://git.drupalcode.org/project/csp/-/blob/20b79b90120a7e07aed655686d...

The configuration form is a little more restrictive - limiting the options to ones that are know to be valid for a particular directive, and emitting errors where known invalid values are seen. This means that if a new keyword is introduced to the spec the form will need an update - it's still possible to modify the directive directly in configuration (or add it programmatically) and it will be output in the header, but it would be removed from config if the admin form were to be saved while it's not an available option.

gapple’s picture

Status: Active » Fixed

I think the current optimizations are good as-is.
I would like to have a specific, practical use case for further optimizations.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.