[Bug]: strictDynamicAllowedOnScripts cannot be set to false

[dtamajon]

⚠️ This issue respects the following points: ⚠️

• This is a bug, not a question or a configuration/webserver/proxy issue.
• This issue is not already reported on Github OR Nextcloud Community Forum (I've searched it).
• Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
• I agree to follow Nextcloud's Code of Conduct.

Bug description

On commit e231abd, the default value for $strictDynamicAllowedOnScripts in lib/public/AppFramework/Http/ContentSecurityPolicy.php was set to "true".

Given the way in which the merge policies are done in lib/private/Security/CSP/ContentSecurityPolicyManager.php, you can never set the parameter to false:

				// true wins over false
				if ($value > $currentValue) {
					$defaultPolicy->$setter($value);
				}

So, the default "true" value always remains with no option to disable it if necessary.

Steps to reproduce

1. Create a simple custom CSP in a custom module:

namespace OCA\Sample;

use OCP\AppFramework\Http\EmptyContentSecurityPolicy;

class MyContentSecurityPolicy extends EmptyContentSecurityPolicy {
	/** @var bool Whether strict-dynamic should be used on script-src-elem */
	protected $strictDynamicAllowedOnScripts = false;
}

2. Create a Controller which returns a TemplateResponse:

    public function index() {
        $fpolicy = new FeaturePolicy();
        $cspolicy = new MyContentSecurityPolicy();
        $cspolicy->addAllowedScriptDomain("https://another-domain.com/");

        $template = new TemplateResponse('sample', 'myView');
        $template->setContentSecurityPolicy($cspolicy);
        $template->setFeaturePolicy($fpolicy);

        return $template;
    }

3. Create your "myView.php" template:

<script src="https://another-domain.com/myScript.js"></script>

4. Access to the index of the module, you will see the following error on the browser console:

Refused to load the script 'https://another-domain.com/myScript.js' because it violates the following Content Security Policy directive: "script-src-elem 'strict-dynamic'

Expected behavior

On changing the property, the explicit CSP should be over the default CSP. There are 2 options: the first one is to modify the merge method; the second one is to leave default options as false.

The option of modifying the merge method can have consequences that I'm not able to figure out currently. It would be useful if someone can point into the scenarios in which the mergePolicies method must be as it is.

The second option allows you to manage the CSP, but it can be a security risk in some scenarios.

Installation method

Community Web installer on a VPS or web space

Nextcloud Server version

28

Operating system

Debian/Ubuntu

PHP engine version

PHP 8.1

Web server

Nginx

Database engine version

MariaDB

Is this bug present after an update or on a fresh install?

Upgraded to a MAJOR version (ex. 22 to 23)

Are you using the Nextcloud Server Encryption module?

Encryption is Disabled

What user-backends are you using?

• Default user-backend (database)
• LDAP/ Active Directory
• SSO - SAML
• Other

Configuration report

{
    "system": {
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "demo.vtramit.com"
        ],
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "mysql",
        "version": "28.0.5.1",
        "overwrite.cli.url": "https:\/\/demo.vtramit.com",
        "dbname": "***REMOVED SENSITIVE VALUE***",
        "dbhost": "***REMOVED SENSITIVE VALUE***",
        "dbport": "",
        "dbtableprefix": "oc_",
        "mysql.utf8mb4": true,
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "installed": true,
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "default_phone_region": "ES",
        "memcache.local": "\\OC\\Memcache\\Redis",
        "redis": {
            "host": "***REMOVED SENSITIVE VALUE***",
            "port": 6379,
            "dbindex": 0,
            "timeout": 5
        },
        "simpleSignUpLink.shown": false,
        "theme": "",
        "loglevel": 2,
        "maintenance": false
    }
}

List of activated Apps

- activity: 2.20.0
  - admin_audit: 1.18.0
  - calendar: 4.7.1
  - circles: 28.0.0
  - cloud_federation_api: 1.11.0
  - comments: 1.18.0
  - contactsinteraction: 1.9.0
  - dav: 1.29.1
  - federatedfilesharing: 1.18.0
  - federation: 1.18.0
  - files: 2.0.0
  - files_pdfviewer: 2.9.0
  - files_reminders: 1.1.0
  - files_sharing: 1.20.0
  - files_trashbin: 1.18.0
  - files_versions: 1.21.0
  - firstrunwizard: 2.17.0
  - forms: 4.2.3
  - groupfolders: 16.0.6
  - impersonate: 1.15.0
  - logreader: 2.13.0
  - lookup_server_connector: 1.16.0
  - nextcloud_announcements: 1.17.0
  - notifications: 2.16.0
  - oauth2: 1.16.3
  - onlyoffice: 9.2.0
  - password_policy: 1.18.0
  - privacy: 1.12.0
  - provisioning_api: 1.18.0
  - recommendations: 2.0.0
  - related_resources: 1.3.0
  - serverinfo: 1.18.0
  - settings: 1.10.1
  - sharebymail: 1.18.0
  - support: 1.11.1
  - survey_client: 1.16.0
  - systemtags: 1.18.0
  - text: 3.9.1
  - theming: 2.3.0
  - twofactor_backupcodes: 1.17.0
  - twofactor_totp: 10.0.0-beta.2
  - updatenotification: 1.18.0
  - user_status: 1.8.1
  - viewer: 2.2.0
  - weather_status: 1.8.0
  - workflowengine: 2.10.0
Disabled:
  - bruteforcesettings: 2.8.0
  - dashboard: 7.8.0 (installed 7.1.0)
  - encryption: 2.16.0
  - files_external: 1.20.0
  - files_rightclick: 0.15.1 (installed 1.6.0)
  - photos: 2.4.0 (installed 1.3.0)
  - suspicious_login: 6.0.0
  - user_ldap: 1.19.0
  - user_saml: 5.2.6 (installed 5.2.6)

Nextcloud Signing status

Technical information
=====================
The following list covers which files have failed the integrity check. Please read
the previous linked documentation to learn more about the errors and how to fix
them.

Results
=======
- core
	- EXTRA_FILE
		- .well-known/ai-plugin.json

Raw output
==========
Array
(
    [core] => Array
        (
            [EXTRA_FILE] => Array
                (
                    [.well-known/ai-plugin.json] => Array
                        (
                            [expected] => 
                            [current] => 828bd800d444b7aab125ec7b2453dd2c89ea2738928c52bb292daa68f60201234c1c5495884f8122633be98ed7bae324c02ad396cc2bd72772a5695f680839d1
                        )

                )

        )

)

Nextcloud Logs

No response

Additional info

No response

[mpboom]

This is a bug indeed which also breaks our plugin. It's a bit difficult to make that decision in the merge functionality. You're confronted with two values, and for some (e.g. inlineStyleAllowed) the truthy value makes the CSP less strict while for others (e.g. this one: strictDynamicAllowedOnScripts) it makes the CSP stricter. I would argue the merge function needs to respect the least strict CSP that was given.

One potential way of fixing would be to invert the behavior of the variables, so that truthy always means "less strict". In this case, I'd rename the variable to something like strictDynamicAllowedOnScripts to strictDynamicDisabledOnScripts which actually disables setting strict-dynamic on script-elem (by default, it would be added then). In the default CSP template you would then get:

protected $strictDynamicDisabledOnScripts = false;
protected $strictDynamicDisabled = true;

The merge function could then stay the same. Still not ideal though, as it would still not allow a plugin to actually enforce strict-dynamic unless enabled in the default template. However, it makes the behavior a bit more in line with the behavior on different properties.

[hschletz]

When a CSP rule has a strict default set by the framework (like strict-dynamic), the only meaningful modification would be to disable it. This may have a negative impact on site security, but apps will keep working.

On the other hand, allowing an app to enforce a strict rule when another app has already disabled it will break that app. This is also unnecessary because the strict rule is already in effect by default.

This means that a boolean parameter makes no sense if a strict default is provided, and that the only meaningful use for useStrictDynamicOnScripts(bool) currently does not work. Instead, there should be a disableStrictDynamicOnScripts() method that does not take any arguments and disables the rule once and for all. Apps that require a loosened CSP will keep working, and other apps don't have to care.

Other rules are affected as well, and it's not only on/off rules that cannot be overridden. useJsNonce(string | null) also has no effect.