diff --git a/src/ExpressionParser/Infix/DotExpressionParser.php b/src/ExpressionParser/Infix/DotExpressionParser.php index 31418b585..e1afa5f59 100644 --- a/src/ExpressionParser/Infix/DotExpressionParser.php +++ b/src/ExpressionParser/Infix/DotExpressionParser.php @@ -67,12 +67,15 @@ final class DotExpressionParser extends AbstractExpressionParser implements Infi if ( $expr instanceof NameExpression + && $attribute instanceof ConstantExpression + && \is_string($name = $attribute->getAttribute('value')) + && preg_match('#^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$#D', $name) && ( null !== $parser->getImportedSymbol('template', $expr->getAttribute('name')) - || '_self' === $expr->getAttribute('name') && $attribute instanceof ConstantExpression + || '_self' === $expr->getAttribute('name') ) ) { - return new MacroReferenceExpression(new TemplateVariable($expr->getAttribute('name'), $expr->getTemplateLine()), 'macro_'.$attribute->getAttribute('value'), $arguments, $expr->getTemplateLine()); + return new MacroReferenceExpression(new TemplateVariable($expr->getAttribute('name'), $expr->getTemplateLine()), 'macro_'.$name, $arguments, $expr->getTemplateLine()); } return new GetAttrExpression($expr, $attribute, $arguments, $type, $lineno, $nullSafe); diff --git a/src/Node/Expression/MacroReferenceExpression.php b/src/Node/Expression/MacroReferenceExpression.php index ba1d556b4..59cb2092a 100644 --- a/src/Node/Expression/MacroReferenceExpression.php +++ b/src/Node/Expression/MacroReferenceExpression.php @@ -26,6 +26,14 @@ class MacroReferenceExpression extends AbstractExpression implements SupportDefi public function __construct(TemplateVariable $template, string $name, AbstractExpression $arguments, int $lineno) { + // The name is emitted as raw PHP in compile() via "->{$name}(...)", + // so it must be a valid PHP method identifier. Reject anything else + // as a defense-in-depth against accidental PHP code injection from + // a caller that forgot to validate user-controlled input. + if (!preg_match('#^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$#D', $name)) { + throw new \LogicException(\sprintf('Macro name "%s" is not a valid PHP identifier.', $name)); + } + parent::__construct(['template' => $template, 'arguments' => $arguments], ['name' => $name], $lineno); } diff --git a/tests/Extension/SandboxTest.php b/tests/Extension/SandboxTest.php index 2f8f5f471..6d8db8044 100644 --- a/tests/Extension/SandboxTest.php +++ b/tests/Extension/SandboxTest.php @@ -519,6 +519,73 @@ EOF $this->assertEquals('

username

', $twig->load('index')->render([])); } + public function testSelfMacroReferenceWithStringLiteralDoesNotInjectPhp() + { + $twig = $this->getEnvironment(true, [], ['index' => '{{ _self.(\'foo + 1; trigger_error("BAD-MACRO-REF") //\') }}']); + + $compiled = $twig->compileSource($twig->getLoader()->getSourceContext('index')); + $this->assertStringNotContainsString('trigger_error("BAD-MACRO-REF")', $compiled, 'Attacker-controlled string must not appear raw in compiled PHP source.'); + $this->assertStringNotContainsString('->macro_foo + 1;', $compiled, 'No raw injection should reach the generated method-call site.'); + + $triggered = false; + set_error_handler(static function ($severity, $message) use (&$triggered) { + if (str_contains($message, 'BAD-MACRO-REF')) { + $triggered = true; + } + return true; + }, \E_USER_NOTICE | \E_USER_WARNING); + try { + try { + $twig->load('index')->render([]); + } catch (\Throwable) { + } + } finally { + restore_error_handler(); + } + + $this->assertFalse($triggered, 'No PHP from the template literal must execute.'); + } + + public function testImportedTemplateMacroReferenceWithBadIdentifierDoesNotInjectPhp() + { + $payload = '{% import "m" as m %}{{ m.(\'foo + 1; trigger_error("BAD-IMPORT-REF") //\') }}'; + $twig = $this->getEnvironment(true, [], [ + 'index' => $payload, + 'm' => '{% macro greet() %}hi{% endmacro %}', + ], ['import']); + + $compiled = $twig->compileSource($twig->getLoader()->getSourceContext('index')); + $this->assertStringNotContainsString('trigger_error("BAD-IMPORT-REF")', $compiled, 'Attacker-controlled string must not appear raw in compiled PHP source.'); + + $triggered = false; + set_error_handler(static function ($severity, $message) use (&$triggered) { + if (str_contains($message, 'BAD-IMPORT-REF')) { + $triggered = true; + } + return true; + }, \E_USER_NOTICE | \E_USER_WARNING); + try { + try { + $twig->load('index')->render([]); + } catch (\Throwable) { + } + } finally { + restore_error_handler(); + } + $this->assertFalse($triggered, 'No PHP from the template literal must execute.'); + } + + public function testSelfMacroReferenceWithValidIdentifierStillWorks() + { + $twig = $this->getEnvironment(true, ['autoescape' => 'html'], ['index' => <<assertSame('Hi World', $twig->load('index')->render([])); + } + public function testSandboxDisabledAfterIncludeFunctionError() { $twig = $this->getEnvironment(false, [], self::$templates); diff --git a/tests/Node/Expression/MacroReferenceTest.php b/tests/Node/Expression/MacroReferenceTest.php new file mode 100644 index 000000000..817542c5b --- /dev/null +++ b/tests/Node/Expression/MacroReferenceTest.php @@ -0,0 +1,41 @@ +expectException(\LogicException::class); + $this->expectExceptionMessage(\sprintf('Macro name "%s" is not a valid PHP identifier.', $name)); + + new MacroReferenceExpression(new TemplateVariable('foo', 1), $name, new ArrayExpression([], 1), 1); + } + + public static function provideInvalidMacroNames(): iterable + { + yield 'empty' => ['']; + yield 'starts with digit' => ['1foo']; + yield 'contains space' => ['foo bar']; + yield 'contains semicolon' => ['foo;bar']; + yield 'PHP injection payload' => ['macro_foo + 1; trigger_error("BAD") //']; + yield 'contains NUL byte' => ["foo\x00bar"]; + } +}