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

[Panel] CSP errors #1527

Closed
bnomei opened this issue Mar 3, 2019 · 8 comments

Comments

@bnomei
Copy link

@bnomei bnomei commented Mar 3, 2019

Describe the bug
The panel requires CSP with inline styles and inline eval. this would require a rule to allow inline js eval for the complete site just to make the panel work. please find a more secure way.

the following examples are just for the panel login page. there might be more issues past the login.

Content Security Policy: The page’s settings blocked the loading of a resource at inline (“style-src”).

<body>
<svg aria-hidden="true" style="position:absolute;width:0;height:0" xmlns="http://www.w3.org/2000/svg" overflow="hidden">

solution: just add a class instead, right?

Content Security Policy: The page’s settings blocked the loading of a resource at inline (“script-src”).

<script>window.panel = {"url":"https://DOMAIN/panel","site":"https://DOMAIN","api":"https://DOMAIN/api","csrf":"ade9273632029b912a42a7aa9f54b85d7626a24ddd8b97177d05a7eb94ed3116","translation":"en","debug":true,"search":{"limit":10}}</script>

solution: register a nonce using http header and add a nonce for the script element. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#Unsafe_inline_script

To Reproduce
Steps to reproduce the behavior:
setup some kind of secure CSPs (aka not inline, not eval!) a quick test could be done with my plugin https://github.com/bnomei/kirby3-security-headers
please note that i am well aware that in almost all cases the default CSP my plugin provide must be extended. i just want to point out the panel could be made more security friendly in not using inline styles and eval.

Expected behavior
I expeced the panel to not use inline styles and eval because its unsave to do so. I understand kirby should not define csp for the fronend itself, but it should at least make the panel save.

Kirby Version
3.x

@bnomei

This comment has been minimized.

Copy link
Author

@bnomei bnomei commented Mar 3, 2019

furthermore i did not run into the inline scripts error using kirby 3.0.1? so maybe this is new?

@bnomei

This comment has been minimized.

Copy link
Author

@bnomei bnomei commented Mar 3, 2019

trying to login with no CSP rule for the inline script will fail. this is the error message.

Object { status: "error", exception: "Kirby\\Exception\\InvalidArgumentException", message: "Invalid CSRF token", key: "0", file: "y/config/api/routes/auth.php", line: 31, details: [], code: 400 }
app.js:1:236022
@bastianallgeier bastianallgeier added this to the 3.1.0 milestone Mar 4, 2019
@distantnative distantnative changed the title panel CSP errors [Panel] CSP errors Mar 6, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Mar 11, 2019

We had the same setup in the previous versions, so the errors must be the same.

As a first fix I removed the inline style attribute and switched to a class.

Eval isn't used anywhere so that's not a problem.

The inline script tag is still an issue and as you said we can only fix that with a nonce. I'm a bit hesitant to set a default content security policy right now. I don't really know what kind of expectations plugin devs would have. Let's say you create a plugin that loads images from a CDN, you would then also need to change the CSP for the panel.

I was wondering if it made sense to introduce a panel.headers option. This could be a callback that receives the nonce that the panel will use in the template.

return [
  'panel' => [
    'headers' => function ($nonce) {
       // set custom headers here. 
    }
  ]
];

This would offer total freedom to create your own policies for the panel.

Another option would be some global registry for plugins to register "safe" sources and the panel would then take care to set the matching headers.

@bastianallgeier bastianallgeier removed this from the 3.1.0 milestone Mar 11, 2019
@bnomei

This comment has been minimized.

Copy link
Author

@bnomei bnomei commented Mar 11, 2019

ah you are right. the error appeared once i switched my plugin from snippet to route:before hook. the latter is applied to panel as well.

the panel.headers callback seems like a fine idea. but my suggestion would be to make it panel.csp.nonces and return an array. pretty much like my plugins allows setting custom nonces right now.
https://github.com/bnomei/kirby3-security-headers/blob/master/index.php#L17

i could just array_merge that callback in my plugins default implementation and thus still keep it "zero config" for default usecase.
https://github.com/bnomei/kirby3-security-headers/blob/master/classes/SecurityHeaders.php#L84

@bnomei

This comment has been minimized.

Copy link
Author

@bnomei bnomei commented Mar 11, 2019

sorry that might have been confusing. what i do not need is a way to set custom headers. i use my plugin to do that and putting it in another callback in the config does not make it much more readable imho.

i only need the array of all nonces the panel needs. so it does not have to be a callback at all. something like kirby()->system()->nonces():array would be totally fine. that array i can use to build the headers.

@bastianallgeier bastianallgeier added this to the 3.1.1 milestone Mar 12, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.1.2, 3.1.3 Apr 1, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.1.3, 3.1.4 Apr 16, 2019
@distantnative distantnative modified the milestones: 3.1.4, 3.2.0 Apr 16, 2019
@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented May 1, 2019

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Aug 9, 2019

I understand @bastianallgeier's hesitation to set a default content security policy.
What about:

  • We generate and set a nonce for the inline script on the tag
  • We do not set any CSP ourselves
  • We provide that nonce in kirby()->system()->nonces():array

Then everything is prepared. And those who set a CSP can include the nonce to keep the Panel working.

bnomei added a commit to bnomei/kirby3-security-headers that referenced this issue Sep 1, 2019
…as needed.

getkirby/kirby#1527

Signed-off-by: Bruno Meilick <b@bnomei.com>
bastianallgeier added a commit that referenced this issue Oct 15, 2019
…ng” all external and inline sources #1527
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 15, 2019

I finally fixed this by introducing a new $kirby->nonce() method. The nonce is used for all scripts, stylesheets and the svg. I think it's enough that we have one nonce which changes on every request, but please correct me if I'm wrong.

bastianallgeier added a commit that referenced this issue Oct 15, 2019
…ng” all external and inline sources #1527
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.