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

Require-sri-for speciální CSP pravidlo #143

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@peldax
Copy link
Contributor

peldax commented Aug 13, 2018

  • bug fix / new feature - tak trochu obojí
  • BC break? no
  • doc PR: None ATM

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/require-sri-for

Speciální záznam pro CSP, který ukazuje pro který content má prohlížeč vyžadovat subresource integrity.

csp
    require-sri-for: [ script, style ]

Při použití tohoto pravidla v konfigu nette vloží uvozovky kolem script a style - protože se dávají kolem none, self a dalších klíčovách slov. Bohužel očekávané je nekonzistetní bez uvozovek, chrome uvozovky nezkousne.

Přidal jsem kontrolu zdali se jedná o typ require-sri-for a pokud ano nedají se uvozovky.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

Další speciální pravidlo block-all-mixed-content funguje vpořádku při použití konfigurace:

block-all-mixed-content: []
@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

Probably sandbox is the same case.

Can you add a test?

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

I can add a test.

Wouldnt be better to have some kind of whitelist with all the keywords?

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

Maybe yes.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

I'll add whitelist of all NON qouted keywords. Nonces and hashes are qouted.

I dont really understand the reasoning behind the specification...

@peldax peldax referenced this pull request Aug 13, 2018

Closed

Feature-Policy header #142

@harmim

This comment has been minimized.

Copy link
Contributor

harmim commented Aug 13, 2018

@peldax As you mentioned block-all-mixed-content directive. There are other directives without source list: disown-opener, upgrade-insecure-requests and maybe some others. These directives are quite useful by the way.
The thing is you are able to define these directives in HTTP extension somehow like this:

csp:
    upgrade-insecure-requests:
    block-all-mixed-content: []

and it doesn't make sense to me. It seems like these directives are disabled. More meaningful would be something like this:

csp:
    upgrade-insecure-requests: true
    block-all-mixed-content: true

What do you think about it David? @dg

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

This would require special section in config or built in list of those "empty" directives.
It would be nice, but:

  • it would be possible BC break - some apps already implement current state
  • it IS working in current state, this PR intented to fix require-sri-for, which wasnt working at all
@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

@harmim We could create PR with your suggestion or even add it to my PR #142 - which currently by far exceeded its original purpose.

What would you expect to be outcome of:

csp:
    upgrade-insecure-requests: false
    block-all-mixed-content: false

No directive at all?

@harmim

This comment has been minimized.

Copy link
Contributor

harmim commented Aug 13, 2018

Regarding require-sri-for.
There are many others directive values which must not be quoted. Of course it could be URL or just a scheme then it could be asterisk symbol *, then data:, mediastream:, blob: and filesystem:. All of these should work right now. But in CSP you can specify referrer directive which accepts these unquoted values no-referrer, no-referrer-when-downgrade, same-origin, origin, strict-origin, origin-when-cross-origin, strict-origin-when-cross-origin and unsafe-url. Then there could be reflected-xss with values allow, block and filter. report-to accepting token. require-sri-for with values script and filter. sandbox with values sandbox, allow-forms, allow-same-origin, allow-scripts, allow-top-navigation, allow-modals, allow-orientation-lock, allow-popups, allow-presentation, allow-popups-to-escape-sandbox, allow-top-navigation and allow-pointer-lock. report-uri is just unquoted URI. plugin-types is unquoted MIME type and so on...
You are not able to whitelist it.

The behaviour should be right opposite. Something like this:

private const CSP_QUOTED_SOURCE_VALUES = ['none', 'self', 'unsafe-inline', 'unsafe-eval', 'nonce', 'sha256', 'strict-dynamic', 'unsafe-hashed-attributes'];

...

$isQuoted = false;
foreach (self::CSP_QUOTED_SOURCE_VALUES as $value) {
    if (\strpos($item, $value) !== false) {
        $isQuoted = true;
        break;
    }
}
$value .= $isQuoted ? " '$item'" : " $item";
@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

@harmim Please see #142

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

You have to decide to whitelist one list or the other, whatever you decide to whitelist, there is always going to emerge some new keyword which has to be added to the list.

For example hash algorithm doesnt need to be sha256.

@harmim

This comment has been minimized.

Copy link
Contributor

harmim commented Aug 13, 2018

What would you expect to be outcome of:

csp:
   upgrade-insecure-requests: false
   block-all-mixed-content: false

I don't know. Maybe no directive at all, as you said.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

@harmim I agree, its just one type-sensitive check. I suggest we PR it later after this is resolved. Syntactic sugar can wait :).

@harmim

This comment has been minimized.

Copy link
Contributor

harmim commented Aug 13, 2018

Isn't this solution for true/false directives without source list (arguments)?

if (\is_bool($policy)) {
	if ($policy) {
		$value .= $type . '; ';
	}
} else {
	$value .= $type;
	foreach ((array) $policy as $item) {
		$value .= \preg_match('#^[a-z-]+\z#', $item) ? " '$item'" : " $item";
	}
	$value .= '; ';
}
@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

It is, I would just write it differently - without else - with continue;

@dg dg closed this in fcedaef Aug 13, 2018

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