Skip to content

Commit

Permalink
Merge pull request #41571 from nextcloud/fix/allow-strict-dynamic-elem
Browse files Browse the repository at this point in the history
Allow setting `strict-dynamic` on `strict-src-elem` and set it by default
  • Loading branch information
susnux authored Nov 17, 2023
2 parents 8e3a08d + e231abd commit 4fa2749
Show file tree
Hide file tree
Showing 6 changed files with 167 additions and 65 deletions.
8 changes: 8 additions & 0 deletions lib/private/Security/CSP/ContentSecurityPolicy.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
2 changes: 2 additions & 0 deletions lib/public/AppFramework/Http/ContentSecurityPolicy.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 = true;
/** @var array Domains from which scripts can get loaded */
protected $allowedScriptDomains = [
'\'self\'',
Expand Down
34 changes: 28 additions & 6 deletions lib/public/AppFramework/Http/EmptyContentSecurityPolicy.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 .= ';';
}

Expand Down
Loading

0 comments on commit 4fa2749

Please sign in to comment.