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

Make Content Security Policy (CSP) nonces optional #6845

Closed
wants to merge 3 commits into from

Conversation

Valkhan
Copy link

@Valkhan Valkhan commented Nov 12, 2022

Description

Added a config option to make nonce optional.
There are many situations using third-party libs from google and others that doesn't work with nonces enabled as they inject new scripts or inline styles to their elements breaking their functionality.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Conforms to style guide

Added a config option to make nonce optional.

There are many situations using third-party libs from google and others that doesn't work with nonces enabled.
Added option to make nonce optional with default value set to true due to backwards compatibility.
@victor-devdesign
Copy link

Nice, I was looking for a workaround and this solution worked for me.

@kenjis kenjis added wrong branch PRs sent to wrong branch tests needed Pull requests that need tests docs needed Pull requests needing documentation write-ups and/or revisions. labels Nov 12, 2022
@kenjis
Copy link
Member

kenjis commented Nov 12, 2022

Thank you for contributing!

New features or enhancements need to go to the next minor version.
Unfortunately this PR should go to 4.3 branch, not develop.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#if-you-sent-to-the-wrong-branch

And we need test code.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing

@kenjis
Copy link
Member

kenjis commented Nov 12, 2022

@jonassilvati
Copy link

great, I liked the solution

@lonnieezell
Copy link
Member

Can you clarify the problem with some code examples? Couldn't this be solved by different CSP rules?

@Valkhan
Copy link
Author

Valkhan commented Nov 14, 2022

I recommend you try to use strict-dynamic: https://content-security-policy.com/strict-dynamic/ https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#strict-dynamic

I've read that but I don't think that solves the issue i'm facing.

Thank you for contributing!

New features or enhancements need to go to the next minor version. Unfortunately this PR should go to 4.3 branch, not develop. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#if-you-sent-to-the-wrong-branch

And we need test code. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing

Sorry, first time here, I don't know how i'll put up some tests since this behaviour is validated by the browser, i'm not used to unit testing yet.

Can you clarify the problem with some code examples? Couldn't this be solved by different CSP rules?

It can't be solved by different CSP rules because nonces enforces security rules that otherwise would solve the issue.

# My CSP config file:

<?php

namespace Config;

use CodeIgniter\Config\BaseConfig;

/**
 * Stores the default settings for the ContentSecurityPolicy, if you
 * choose to use it. The values here will be read in and set as defaults
 * for the site. If needed, they can be overridden on a page-by-page basis.
 *
 * Suggested reference for explanations:
 *
 * @see https://www.html5rocks.com/en/tutorials/security/content-security-policy/
 */
class ContentSecurityPolicy extends BaseConfig
{
    //-------------------------------------------------------------------------
    // Broadbrush CSP management
    //-------------------------------------------------------------------------

    /**
     * Default CSP report context
     *
     * @var bool
     */
    public $reportOnly = false;

    /**
     * Specifies a URL where a browser will send reports
     * when a content security policy is violated.
     *
     * @var string|null
     */
    public $reportURI;

    /**
     * Instructs user agents to rewrite URL schemes, changing
     * HTTP to HTTPS. This directive is for websites with
     * large numbers of old URLs that need to be rewritten.
     *
     * @var bool
     */
    public $upgradeInsecureRequests = false;

    //-------------------------------------------------------------------------
    // Sources allowed
    // Note: once you set a policy to 'none', it cannot be further restricted
    //-------------------------------------------------------------------------

    /**
     * Will default to self if not overridden
     *
     * @var string|string[]|null
     */
    public $defaultSrc = 'self';

    /**
     * Lists allowed scripts' URLs.
     *
     * @var string|string[]
     */
    public $scriptSrc = ['self', 'unsafe-inline', 'unsafe-eval', 'apis.google.com', ' www.gstatic.com', 'www.googletagmanager.com', 'accounts.google.com'];

    /**
     * Lists allowed stylesheets' URLs.
     *
     * @var string|string[]
     */
    public $styleSrc = ['self', 'unsafe-inline', 'accounts.google.com'];

    /**
     * Defines the origins from which images can be loaded.
     *
     * @var string|string[]
     */
    public $imageSrc = ['self', 'data:'];

    /**
     * Restricts the URLs that can appear in a page's `<base>` element.
     *
     * Will default to self if not overridden
     *
     * @var string|string[]|null
     */
    public $baseURI = 'self';

    /**
     * Lists the URLs for workers and embedded frame contents
     *
     * @var string|string[]
     */
    public $childSrc = 'self';

    /**
     * Limits the origins that you can connect to (via XHR,
     * WebSockets, and EventSource).
     *
     * @var string|string[]
     */
    public $connectSrc = ['self', 'accounts.google.com'];

    /**
     * Specifies the origins that can serve web fonts.
     *
     * @var string|string[]
     */
    public $fontSrc = ['self', 'data:'];

    /**
     * Lists valid endpoints for submission from `<form>` tags.
     *
     * @var string|string[]
     */
    public $formAction = 'self';

    /**
     * Specifies the sources that can embed the current page.
     * This directive applies to `<frame>`, `<iframe>`, `<embed>`,
     * and `<applet>` tags. This directive can't be used in
     * `<meta>` tags and applies only to non-HTML resources.
     *
     * @var string|string[]|null
     */
    public $frameAncestors = 'self';

    /**
     * The frame-src directive restricts the URLs which may
     * be loaded into nested browsing contexts.
     *
     * @var array|string|null
     */
    public $frameSrc = ['self', 'accounts.google.com'];

    /**
     * Restricts the origins allowed to deliver video and audio.
     *
     * @var string|string[]|null
     */
    public $mediaSrc = 'self';

    /**
     * Allows control over Flash and other plugins.
     *
     * @var string|string[]
     */
    public $objectSrc = 'none';

    /**
     * @var string|string[]|null
     */
    public $manifestSrc = 'self';

    /**
     * Limits the kinds of plugins a page may invoke.
     *
     * @var string|string[]|null
     */
    public $pluginTypes;

    /**
     * List of actions allowed.
     *
     * @var string|string[]|null
     */
    public $sandbox;

    /**
     * Nonce tag for style
     *
     * @var string
     */
    public $styleNonceTag = '{csp-style-nonce}';

    /**
     * Nonce tag for script
     *
     * @var string
     */
    public $scriptNonceTag = '{csp-script-nonce}';

    /**
     * Replace nonce tag automatically
     *
     * @var bool
     */
    public $autoNonce = true;

    /**
     * When enabled will add nonce to script, style and headers.
     *
     * @var bool
     */
    public $nonceEnabled = true;
}
## My HTML file:

<html>
<body>
<!--StartFragment-->

<meta name="google-client-id" content="XXXXXXXX">
--
  |  
  | <script src="https://accounts.google.com/gsi/client"></script>
  | <script src="http://localhost:8080/js/API_google.min.js"></script>
  | <script nonce="828e0e780441ee602344a792">
  | $(function () {
  | window.$GoogleApi = new GoogleApi('login');
  | });
  | </script>

<!--EndFragment-->
</body>
</html>

Browser rejecting google code injection because of the nonce:

image

If i'm doing something wrong or if anyone else knows a solution for this kind of situation please let me know.

I'll look into sending the pull request to the correct branch and i look forward to any other guidance.

@kenjis
Copy link
Member

kenjis commented Nov 16, 2022

@Valkhan Thank you. But your code does not work.
API_google.min.js does not exist.
Can you show minimum working sample code that reproduces your issue?

@Valkhan
Copy link
Author

Valkhan commented Nov 16, 2022

@Valkhan Thank you. But your code does not work. API_google.min.js does not exist. Can you show minimum working sample code that reproduces your issue?

TLDR:

API_google is custom code and the it's not related to the issue in question, the problem is related to CSP concepts not the implementation code.

The issue lies in the nonce presence, it breaks third party code injection like:

  • Google Login Button
  • Zoom Embeded component
  • Any other code injection that is required for those libraries to work, since they add a <script> tag in your code without nonce the browser just deny it's execution

Long description:

API_google is a custom wrapper to the basic google auth example by google itself (https://developers.google.com/identity/gsi/web/guides/get-google-api-clientid).

https://developers.google.com/identity/gsi/web/guides/get-google-api-clientid#content_security_policy

If you refer to the link above you'll see that i've added the domains required to work with the CSP that I needed, note that they do not have anything related to nonces.

https://issuetracker.google.com/issues/228527097?pli=1

The above link is reporting the same issue that I have, and the workaround would be to download the files from google and serve from our servers, but I really don't think that this is a great idea.

I can't provide a working example as of now because I would have either to upload in my client server due to domain policies required for the google login to work or create a brand new project just to do that.

The steps to reproduce are:

  1. Enable CSP just like the one i've sent
  2. Add the google script <script src="https://accounts.google.com/gsi/client" async defer></script>
  3. Add the script with nonce to call google button: https://developers.google.com/identity/gsi/web/guides/display-button#button_rendering
  4. You got it, when google tries to initialize the button it'll get blocked.

Conclusion:

I'm just trying to share a solution that I think it makes sense and it's missing in the core framework, the framework should not enforce this rule, but it should give you the option to choose if you want to enable that or not given the situation.

Unfortunately I don't know how to make the unit tests and I don't think they are applicable to the context since it's not very code related, it's a browser thing, but again: i'm completly noob at unit testing, never done it and I feel I need to study that, unfortunately i don't have the time to do that in the near future.

If someone could help me out on trying to pull this solution to the next version i'll be very glad and thankfull, i'm just a guy trying to give back some good karma as a token of gratitude for the project that I love so much, but i've my limits, this is as far as I can go for now.

@kenjis
Copy link
Member

kenjis commented Nov 16, 2022

@Valkhan Thank you for your info. I will check it later.

In either case, developers SHOULD NOT include either 'unsafe-inline', or data: as valid sources in their policies. Both enable XSS attacks by allowing code to be included directly in the document itself; they are best avoided completely.
https://www.w3.org/TR/CSP/#csp-directives

@Valkhan
Copy link
Author

Valkhan commented Nov 16, 2022

@Valkhan Thank you for your info. I will check it later.

In either case, developers SHOULD NOT include either 'unsafe-inline', or data: as valid sources in their policies. Both enable XSS attacks by allowing code to be included directly in the document itself; they are best avoided completely.
https://www.w3.org/TR/CSP/#csp-directives

we're in progress of disabling those, but as of now, it's required for our application to work properly, thanks for pointing that out.

@kenjis
Copy link
Member

kenjis commented Nov 17, 2022

I've got the Google login window:
Screenshot 2022-11-17 9 40 55

But the following error still showed on Console:

Refused to apply inline style because it violates the following Content Security Policy directive: "style-src 'self' https://accounts.google.com/gsi/style 'nonce-f63141a52b5bce62079e96c6'". Either the 'unsafe-inline' keyword, a hash ('sha256-lmto2U1o7YINyHPg9TOCjIt+o5pSFNU/T2oLxDPF+uw='), or a nonce ('nonce-...') is required to enable inline execution.

diff --git a/app/Config/App.php b/app/Config/App.php
index 79e5741b4..7b56cf601 100644
--- a/app/Config/App.php
+++ b/app/Config/App.php
@@ -462,5 +462,5 @@ class App extends BaseConfig
      *
      * @var bool
      */
-    public $CSPEnabled = false;
+    public $CSPEnabled = true;
 }
diff --git a/app/Config/ContentSecurityPolicy.php b/app/Config/ContentSecurityPolicy.php
index 0be616301..1effa5c8f 100644
--- a/app/Config/ContentSecurityPolicy.php
+++ b/app/Config/ContentSecurityPolicy.php
@@ -60,14 +60,14 @@ class ContentSecurityPolicy extends BaseConfig
      *
      * @var string|string[]
      */
-    public $scriptSrc = 'self';
+    public $scriptSrc = ['self', 'https://accounts.google.com/gsi/client'];
 
     /**
      * Lists allowed stylesheets' URLs.
      *
      * @var string|string[]
      */
-    public $styleSrc = 'self';
+    public $styleSrc = ['self', 'https://accounts.google.com/gsi/style'];
 
     /**
      * Defines the origins from which images can be loaded.
@@ -98,7 +98,7 @@ class ContentSecurityPolicy extends BaseConfig
      *
      * @var string|string[]
      */
-    public $connectSrc = 'self';
+    public $connectSrc = ['self', 'https://accounts.google.com/gsi/'];
 
     /**
      * Specifies the origins that can serve web fonts.
@@ -130,7 +130,7 @@ class ContentSecurityPolicy extends BaseConfig
      *
      * @var array|string|null
      */
-    public $frameSrc;
+    public $frameSrc = ['self', 'https://accounts.google.com/gsi/'];
 
     /**
      * Restricts the origins allowed to deliver video and audio.
<html>
  <body>
      <script src="https://accounts.google.com/gsi/client" async defer></script>
      <div id="g_id_onload"
         data-client_id="YOUR_GOOGLE_CLIENT_ID"
         data-login_uri="https://your.domain/your_login_endpoint"
         data-auto_prompt="false">
      </div>
      <div class="g_id_signin"
         data-type="standard"
         data-size="large"
         data-theme="outline"
         data-text="sign_in_with"
         data-shape="rectangular"
         data-logo_alignment="left">
      </div>
  </body>
</html>

@kenjis
Copy link
Member

kenjis commented Nov 17, 2022

This solved the CSP error above:

<script <?= csp_style_nonce() ?> src="https://accounts.google.com/gsi/client" async defer></script>

@ddevsr
Copy link
Collaborator

ddevsr commented Nov 17, 2022

By the way, we already have option in PR #6570. You can set $autoNonce to false to skip generateNonces. Or you can set true and set manually csp_style_nonce() suggestion @kenjis above in your <script>.

@Valkhan
Copy link
Author

Valkhan commented Nov 17, 2022

By the way, we already have option in PR #6570. You can set $autoNonce to false to skip generateNonces. Or you can set true and set manually csp_style_nonce() suggestion @kenjis above in your <script>.

Yeah, i've noticed that, but I tought it was more related to the views, the config documentation wasn't clear that besides replacing nonce tags in the views it does anything else, do you know if it also skip the headers output?

Because if it does skip headers as well, I just need to know how to change the config of CSP on a specific route to disable it and the issue i'm trying to solve can be handled without modifications. Can you help me on this one?

@Valkhan
Copy link
Author

Valkhan commented Nov 17, 2022

https://accounts.google.com/gsi/client

I'm very curious about that... I've changed the config as you presented, and in my front-end I've added the nonces.

First of all, I find it curious that you've inserted the STYLE nonce into a SCRIPT tag, it never crossed my mind.

Second, it works, but the browser keeps telling me that google script violated the rule, that means that it violated but wasn't blocked, that's very weird.

image

Third: why do I have to I have to add a nonce to a script src since its already allowed in the domain list it doesn't make sense to me.

Anyhow, I belive that those questions are not related to the framework itself and it's about CSP concepts that should be discussed in proper forums.

I'll wait for @ddevsr if he can point me out on how to change a config in an specifc route so I may test using the autoNonce config. My first thoughts is to create a filter to disable that.

@kenjis
Copy link
Member

kenjis commented Nov 17, 2022

Second, it works, but the browser keeps telling me that google script violated the rule, that means that it violated but wasn't blocked, that's very weird.

It is the last error that I had.
Probably the style that js adds dynamically is blocked.
But in my case, adding <?= csp_style_nonce() ?> removed the error.
I don't know why you have still the error in your environment.

@kenjis
Copy link
Member

kenjis commented Nov 17, 2022

Only the CSP headers that you set will be sent.
But Kint uses CSP nonce, so nonce-* is always sent.

In 4.3 branch, when you set production environment, and you don't call csp_*_nonce() or use {csp-*-nonce},
nonce-* will not be sent.

@kenjis kenjis added the stale Pull requests with conflicts label Jan 10, 2023
@lonnieezell
Copy link
Member

With the auto-nonce and nonceEnabled options, and the fact that this PR is over year old, I'm going to say this issue is dead. Closing for now.

If anyone feels strongly it can be reopened.

@kenjis kenjis changed the title Make Content Security Policy nonces optional Make Content Security Policy (CSP) nonces optional Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs needed Pull requests needing documentation write-ups and/or revisions. stale Pull requests with conflicts tests needed Pull requests that need tests wrong branch PRs sent to wrong branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants