From 9ea0159d04fcc36ab9ca00cfd8d11a000be7482c Mon Sep 17 00:00:00 2001
From: Ferdinand Thiessen <opensource@fthiessen.de>
Date: Fri, 17 Nov 2023 10:56:02 +0100
Subject: [PATCH] feat(ContentSecurityPolicy): Allow to set `strict-dynamic` on
 `script-src-elem` only

This adds the possibility to set `strict-dynamic` on `script-src-elem` only while keep the default rules for `script-src`.
The idea is to allow loading module js which imports other files and thus does not allow nonces on import but on the initial script tag.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
---
 .../Security/CSP/ContentSecurityPolicy.php    |  8 ++++
 .../Http/ContentSecurityPolicy.php            |  2 +
 .../Http/EmptyContentSecurityPolicy.php       | 34 ++++++++++++++---
 .../Http/ContentSecurityPolicyTest.php        | 37 +++++++++++++++++++
 .../Http/EmptyContentSecurityPolicyTest.php   | 36 ++++++++++++++++++
 5 files changed, 111 insertions(+), 6 deletions(-)

diff --git a/lib/private/Security/CSP/ContentSecurityPolicy.php b/lib/private/Security/CSP/ContentSecurityPolicy.php
index eca3e2b6b2940..ee525af4c2a51 100644
--- a/lib/private/Security/CSP/ContentSecurityPolicy.php
+++ b/lib/private/Security/CSP/ContentSecurityPolicy.php
@@ -191,4 +191,12 @@ public function isStrictDynamicAllowed(): bool {
 	public function setStrictDynamicAllowed(bool $strictDynamicAllowed): void {
 		$this->strictDynamicAllowed = $strictDynamicAllowed;
 	}
+
+	public function isStrictDynamicAllowedOnScripts(): bool {
+		return $this->strictDynamicAllowedOnScripts;
+	}
+
+	public function setStrictDynamicAllowedOnScripts(bool $strictDynamicAllowedOnScripts): void {
+		$this->strictDynamicAllowedOnScripts = $strictDynamicAllowedOnScripts;
+	}
 }
diff --git a/lib/public/AppFramework/Http/ContentSecurityPolicy.php b/lib/public/AppFramework/Http/ContentSecurityPolicy.php
index f17dd9bd27048..386d908ffb66d 100644
--- a/lib/public/AppFramework/Http/ContentSecurityPolicy.php
+++ b/lib/public/AppFramework/Http/ContentSecurityPolicy.php
@@ -48,6 +48,8 @@ class ContentSecurityPolicy extends EmptyContentSecurityPolicy {
 	protected ?bool $evalWasmAllowed = false;
 	/** @var bool Whether strict-dynamic should be set */
 	protected $strictDynamicAllowed = false;
+	/** @var bool Whether strict-dynamic should be set for 'script-src-elem' */
+	protected $strictDynamicAllowedOnScripts = false;
 	/** @var array Domains from which scripts can get loaded */
 	protected $allowedScriptDomains = [
 		'\'self\'',
diff --git a/lib/public/AppFramework/Http/EmptyContentSecurityPolicy.php b/lib/public/AppFramework/Http/EmptyContentSecurityPolicy.php
index 7e1de2ef2eb93..960efa75d2c65 100644
--- a/lib/public/AppFramework/Http/EmptyContentSecurityPolicy.php
+++ b/lib/public/AppFramework/Http/EmptyContentSecurityPolicy.php
@@ -41,6 +41,8 @@ class EmptyContentSecurityPolicy {
 	protected $useJsNonce = null;
 	/** @var bool Whether strict-dynamic should be used */
 	protected $strictDynamicAllowed = null;
+	/** @var bool Whether strict-dynamic should be used on script-src-elem */
+	protected $strictDynamicAllowedOnScripts = null;
 	/**
 	 * @var bool Whether eval in JS scripts is allowed
 	 * TODO: Disallow per default
@@ -93,6 +95,18 @@ public function useStrictDynamic(bool $state = false): self {
 		return $this;
 	}
 
+	/**
+	 * In contrast to `useStrictDynamic` this only sets strict-dynamic on script-src-elem
+	 * Meaning only grants trust to all imports of scripts that were loaded in `<script>` tags, and thus weakens less the CSP.
+	 * @param bool $state
+	 * @return EmptyContentSecurityPolicy
+	 * @since 28.0.0
+	 */
+	public function useStrictDynamicOnScripts(bool $state = false): self {
+		$this->strictDynamicAllowedOnScripts = $state;
+		return $this;
+	}
+
 	/**
 	 * Use the according JS nonce
 	 * This method is only for CSPMiddleware, custom values are ignored in mergePolicies of ContentSecurityPolicyManager
@@ -448,27 +462,35 @@ public function buildPolicy() {
 
 		if (!empty($this->allowedScriptDomains) || $this->evalScriptAllowed || $this->evalWasmAllowed) {
 			$policy .= 'script-src ';
+			$scriptSrc = '';
 			if (is_string($this->useJsNonce)) {
 				if ($this->strictDynamicAllowed) {
-					$policy .= '\'strict-dynamic\' ';
+					$scriptSrc .= '\'strict-dynamic\' ';
 				}
-				$policy .= '\'nonce-'.base64_encode($this->useJsNonce).'\'';
+				$scriptSrc .= '\'nonce-'.base64_encode($this->useJsNonce).'\'';
 				$allowedScriptDomains = array_flip($this->allowedScriptDomains);
 				unset($allowedScriptDomains['\'self\'']);
 				$this->allowedScriptDomains = array_flip($allowedScriptDomains);
 				if (count($allowedScriptDomains) !== 0) {
-					$policy .= ' ';
+					$scriptSrc .= ' ';
 				}
 			}
 			if (is_array($this->allowedScriptDomains)) {
-				$policy .= implode(' ', $this->allowedScriptDomains);
+				$scriptSrc .= implode(' ', $this->allowedScriptDomains);
 			}
 			if ($this->evalScriptAllowed) {
-				$policy .= ' \'unsafe-eval\'';
+				$scriptSrc .= ' \'unsafe-eval\'';
 			}
 			if ($this->evalWasmAllowed) {
-				$policy .= ' \'wasm-unsafe-eval\'';
+				$scriptSrc .= ' \'wasm-unsafe-eval\'';
 			}
+			$policy .= $scriptSrc . ';';
+		}
+
+		// We only need to set this if 'strictDynamicAllowed' is not set because otherwise we can simply fall back to script-src
+		if ($this->strictDynamicAllowedOnScripts && !(is_string($this->useJsNonce) && $this->strictDynamicAllowed)) {
+			$policy .= 'script-src-elem \'strict-dynamic\' ';
+			$policy .= $scriptSrc ?? '';
 			$policy .= ';';
 		}
 
diff --git a/tests/lib/AppFramework/Http/ContentSecurityPolicyTest.php b/tests/lib/AppFramework/Http/ContentSecurityPolicyTest.php
index 8e6ac32b416cd..4fd21859bf7f3 100644
--- a/tests/lib/AppFramework/Http/ContentSecurityPolicyTest.php
+++ b/tests/lib/AppFramework/Http/ContentSecurityPolicyTest.php
@@ -479,4 +479,41 @@ public function testGetPolicyNonceStrictDynamic() {
 		$this->contentSecurityPolicy->useStrictDynamic(true);
 		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
 	}
+
+	public function testGetPolicyNonceStrictDynamicOnScripts() {
+		$nonce = 'my-nonce';
+		$expectedPolicy = "default-src 'none';base-uri 'none';manifest-src 'self';script-src 'nonce-".base64_encode($nonce) . "';script-src-elem 'strict-dynamic' 'nonce-".base64_encode($nonce) . "';style-src 'self' 'unsafe-inline';img-src 'self' data: blob:;font-src 'self' data:;connect-src 'self';media-src 'self';frame-ancestors 'self';form-action 'self'";
+
+		$this->contentSecurityPolicy->useJsNonce($nonce);
+		$this->contentSecurityPolicy->useStrictDynamicOnScripts(true);
+		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
+	}
+
+	public function testGetPolicyStrictDynamicOnScripts() {
+		$expectedPolicy = "default-src 'none';base-uri 'none';manifest-src 'self';script-src 'self';script-src-elem 'strict-dynamic' 'self';style-src 'self' 'unsafe-inline';img-src 'self' data: blob:;font-src 'self' data:;connect-src 'self';media-src 'self';frame-ancestors 'self';form-action 'self'";
+
+		$this->contentSecurityPolicy->useStrictDynamicOnScripts(true);
+		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
+	}
+
+	public function testGetPolicyStrictDynamicAndStrictDynamicOnScripts() {
+		$expectedPolicy = "default-src 'none';base-uri 'none';manifest-src 'self';script-src 'self';script-src-elem 'strict-dynamic' 'self';style-src 'self' 'unsafe-inline';img-src 'self' data: blob:;font-src 'self' data:;connect-src 'self';media-src 'self';frame-ancestors 'self';form-action 'self'";
+
+		$this->contentSecurityPolicy->useStrictDynamic(true);
+		$this->contentSecurityPolicy->useStrictDynamicOnScripts(true);
+		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
+	}
+
+	/**
+	 * No duplication as we can fallback
+	 */
+	public function testGetPolicyNonceStrictDynamicAndStrictDynamicOnScripts() {
+		$nonce = 'my-nonce';
+		$expectedPolicy = "default-src 'none';base-uri 'none';manifest-src 'self';script-src 'strict-dynamic' 'nonce-".base64_encode($nonce) . "';style-src 'self' 'unsafe-inline';img-src 'self' data: blob:;font-src 'self' data:;connect-src 'self';media-src 'self';frame-ancestors 'self';form-action 'self'";
+
+		$this->contentSecurityPolicy->useJsNonce($nonce);
+		$this->contentSecurityPolicy->useStrictDynamic(true);
+		$this->contentSecurityPolicy->useStrictDynamicOnScripts(true);
+		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
+	}
 }
diff --git a/tests/lib/AppFramework/Http/EmptyContentSecurityPolicyTest.php b/tests/lib/AppFramework/Http/EmptyContentSecurityPolicyTest.php
index 328e464f981cf..31fc2ffc785ea 100644
--- a/tests/lib/AppFramework/Http/EmptyContentSecurityPolicyTest.php
+++ b/tests/lib/AppFramework/Http/EmptyContentSecurityPolicyTest.php
@@ -425,6 +425,42 @@ public function testGetPolicyWithJsNonceAndScriptDomains() {
 		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
 	}
 
+	public function testGetPolicyWithJsNonceAndStrictDynamic() {
+		$expectedPolicy = "default-src 'none';base-uri 'none';manifest-src 'self';script-src 'strict-dynamic' 'nonce-TXlKc05vbmNl' www.nextcloud.com;frame-ancestors 'none'";
+
+		$this->contentSecurityPolicy->addAllowedScriptDomain('www.nextcloud.com');
+		$this->contentSecurityPolicy->useStrictDynamic(true);
+		$this->contentSecurityPolicy->useJsNonce('MyJsNonce');
+		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
+	}
+
+	public function testGetPolicyWithJsNonceAndStrictDynamicAndStrictDynamicOnScripts() {
+		$expectedPolicy = "default-src 'none';base-uri 'none';manifest-src 'self';script-src 'strict-dynamic' 'nonce-TXlKc05vbmNl' www.nextcloud.com;frame-ancestors 'none'";
+
+		$this->contentSecurityPolicy->addAllowedScriptDomain('www.nextcloud.com');
+		$this->contentSecurityPolicy->useStrictDynamic(true);
+		$this->contentSecurityPolicy->useStrictDynamicOnScripts(true);
+		$this->contentSecurityPolicy->useJsNonce('MyJsNonce');
+		// Should be same as `testGetPolicyWithJsNonceAndStrictDynamic` because of fallback
+		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
+	}
+
+	public function testGetPolicyWithJsNonceAndStrictDynamicOnScripts() {
+		$expectedPolicy = "default-src 'none';base-uri 'none';manifest-src 'self';script-src 'nonce-TXlKc05vbmNl' www.nextcloud.com;script-src-elem 'strict-dynamic' 'nonce-TXlKc05vbmNl' www.nextcloud.com;frame-ancestors 'none'";
+
+		$this->contentSecurityPolicy->addAllowedScriptDomain('www.nextcloud.com');
+		$this->contentSecurityPolicy->useStrictDynamicOnScripts(true);
+		$this->contentSecurityPolicy->useJsNonce('MyJsNonce');
+		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
+	}
+
+	public function testGetPolicyWithStrictDynamicOnScripts() {
+		$expectedPolicy = "default-src 'none';base-uri 'none';manifest-src 'self';script-src-elem 'strict-dynamic' ;frame-ancestors 'none'";
+
+		$this->contentSecurityPolicy->useStrictDynamicOnScripts(true);
+		$this->assertSame($expectedPolicy, $this->contentSecurityPolicy->buildPolicy());
+	}
+
 	public function testGetPolicyWithJsNonceAndSelfScriptDomain() {
 		$expectedPolicy = "default-src 'none';base-uri 'none';manifest-src 'self';script-src 'nonce-TXlKc05vbmNl';frame-ancestors 'none'";