Skip to content

Commit 99a1038

Browse files
fabpotfabpot
committed
security #558 Fix sandbox filter/tag/function allow-list bypass when sandbox state changes between renders (fabpot)
This PR was merged into the twig-3.x branch. Discussion ---------- Fix sandbox filter/tag/function allow-list bypass when sandbox state changes between renders Fixes #555 Commits ------- 23eb6eb Fix sandbox filter/tag/function allow-list bypass when sandbox state changes between renders
2 parents 7d55aa8 + 23eb6eb commit 99a1038

8 files changed

Lines changed: 463 additions & 29 deletions

File tree

‎CHANGELOG‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
* Add a strict mode to `Twig\Sandbox\SecurityPolicy` to opt-in to the 4.0 behavior for the `extends`/`use` tags and the `parent`/`block`/`attribute` functions, which are otherwise still implicitly allowed in a sandbox
44
* Deprecate the fact that the `parent`, `block`, and `attribute` functions are always allowed in a sandboxed template
5+
* Fix sandbox filter/tag/function allow-list bypass when the sandbox state changed between renders of a cached `Template` instance
56
* Fix PHP 8.1+ implicit float-to-int deprecation triggered by sandboxed `ArrayAccess` attribute access with a float key
67
* Restrict allowed classes in `Twig\Profiler\Profile::unserialize()` to prevent arbitrary class instantiation
78
* Escape root profile name in `HtmlDumper`

‎phpstan-baseline.neon‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,5 @@
11
parameters:
22
ignoreErrors:
3-
- # The method is dynamically generated by the CheckSecurityNode
4-
message: '#^Call to an undefined method Twig\\Template\:\:checkSecurity\(\)\.$#'
5-
identifier: method.notFound
6-
count: 1
7-
path: src/Extension/CoreExtension.php
8-
93
- # 2 parameters will be required
104
message: '#^Method Twig\\Node\\IncludeNode\:\:addGetTemplate\(\) invoked with 2 parameters, 1 required\.$#'
115
identifier: arguments.count

‎src/Extension/CoreExtension.php‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1519,10 +1519,6 @@ public static function include(Environment $env, $context, $template, $variables
15191519
return '';
15201520
}
15211521

1522-
if ($isSandboxed) {
1523-
$loaded->unwrap()->checkSecurity();
1524-
}
1525-
15261522
return $loaded->render($variables);
15271523
} finally {
15281524
if ($isSandboxed && !$alreadySandboxed) {

‎src/Node/CheckSecurityCallNode.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ public function compile(Compiler $compiler)
2727
{
2828
$compiler
2929
->write("\$this->sandbox = \$this->extensions[SandboxExtension::class];\n")
30-
->write("\$this->checkSecurity();\n")
3130
;
3231
}
3332
}

‎src/Node/CheckSecurityNode.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,17 @@ public function __construct(array $usedFilters, array $usedTags, array $usedFunc
4141
public function compile(Compiler $compiler): void
4242
{
4343
$compiler
44+
->write("\n")
45+
->write("public function ensureSecurityChecked(): void\n")
46+
->write("{\n")
47+
->indent()
48+
->write("if (\$this->sandbox->isSandboxed(\$this->source)) {\n")
49+
->indent()
50+
->write("\$this->checkSecurity();\n")
51+
->outdent()
52+
->write("}\n")
53+
->outdent()
54+
->write("}\n")
4455
->write("\n")
4556
->write("public function checkSecurity()\n")
4657
->write("{\n")

‎src/Node/IncludeNode.php‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,6 @@ public function compile(Compiler $compiler): void
3838
{
3939
$compiler->addDebugInfo($this);
4040

41-
$sandboxed = $this->hasAttribute('sandboxed') && $this->getAttribute('sandboxed');
42-
4341
if ($this->getAttribute('ignore_missing')) {
4442
$template = $compiler->getVarName();
4543

@@ -64,10 +62,6 @@ public function compile(Compiler $compiler): void
6462
->indent()
6563
;
6664

67-
if ($sandboxed) {
68-
$compiler->write(\sprintf("\$%s->unwrap()->checkSecurity();\n", $template));
69-
}
70-
7165
$compiler->write(\sprintf('yield from $%s->unwrap()->yield(', $template));
7266

7367
$this->addTemplateArguments($compiler);
@@ -76,18 +70,6 @@ public function compile(Compiler $compiler): void
7670
->outdent()
7771
->write("}\n")
7872
;
79-
} elseif ($sandboxed) {
80-
$template = $compiler->getVarName();
81-
82-
$compiler->write(\sprintf('$%s = ', $template));
83-
$this->addGetTemplate($compiler);
84-
$compiler
85-
->raw(";\n")
86-
->write(\sprintf("\$%s->unwrap()->checkSecurity();\n", $template))
87-
->write(\sprintf('yield from $%s->unwrap()->yield(', $template))
88-
;
89-
$this->addTemplateArguments($compiler);
90-
$compiler->raw(");\n");
9173
} else {
9274
$compiler->write('yield from ');
9375
$this->addGetTemplate($compiler);

‎src/Template.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,14 @@ public function getParent(array $context): self|TemplateWrapper|false
8080
return $this->parent;
8181
}
8282

83+
// The compiled doGetParent() may evaluate user expressions (filters,
84+
// functions, method calls) when the parent name is dynamic. Make sure
85+
// the sandbox security check runs first so those expressions cannot
86+
// bypass the allow-list when getParent() is reached before the first
87+
// ensureSecurityChecked() call on this template (e.g. via
88+
// getTemplateForMacro() or yieldBlock() into a pre-warmed instance).
89+
$this->ensureSecurityChecked();
90+
8391
if (!$parent = $this->doGetParent($context)) {
8492
return false;
8593
}
@@ -399,6 +407,7 @@ public function yield(array $context, array $blocks = []): iterable
399407
$blocks = array_merge($this->blocks, $blocks);
400408

401409
try {
410+
$this->ensureSecurityChecked();
402411
yield from $this->doDisplay($context, $blocks);
403412
} catch (Error $e) {
404413
if (!$e->getSourceContext()) {
@@ -443,6 +452,7 @@ public function yieldBlock($name, array $context, array $blocks = [], $useBlocks
443452

444453
if (null !== $template) {
445454
try {
455+
$template->ensureSecurityChecked();
446456
yield from $template->$block($context, $blocks);
447457
} catch (Error $e) {
448458
if (!$e->getSourceContext()) {
@@ -510,19 +520,32 @@ protected function hasMacro(string $name, array $context): bool
510520
protected function getTemplateForMacro(string $name, array $context, int $line, Source $source): self
511521
{
512522
if (method_exists($this, $name)) {
523+
$this->ensureSecurityChecked();
524+
513525
return $this;
514526
}
515527

516528
$parent = $this;
517529
while ($parent = $parent->getParent($context)) {
518530
if (method_exists($parent, $name)) {
531+
$parent->ensureSecurityChecked();
532+
519533
return $parent;
520534
}
521535
}
522536

523537
throw new RuntimeError(\sprintf('Macro "%s" is not defined in template "%s".', substr($name, \strlen('macro_')), $this->getTemplateName()), $line, $source);
524538
}
525539

540+
/**
541+
* Runs the sandbox security check against the current sandbox state.
542+
*
543+
* @internal
544+
*/
545+
public function ensureSecurityChecked(): void
546+
{
547+
}
548+
526549
/**
527550
* Auto-generated method to display the template with the given context.
528551
*

0 commit comments

Comments
 (0)