From 0be92bc8a6fb83e6e0d883946f7e7c09ba4e857a Mon Sep 17 00:00:00 2001 From: Simon Wisselink Date: Tue, 28 May 2024 22:44:30 +0200 Subject: [PATCH] Merge pull request from GHSA-4rmg-292m-wg3w --- changelog/GHSA-4rmg-292m-wg3w.md | 2 + src/Compile/Tag/ExtendsTag.php | 64 +------------------ src/Compiler/Template.php | 38 +++++++---- src/Smarty.php | 9 +++ .../BockExtend/CompileBlockExtendsTest.php | 36 ++++++++++- .../BockExtend/templates/escaping.tpl | 1 + .../BockExtend/templates/escaping2.tpl | 1 + .../BockExtend/templates/escaping3.tpl | 1 + .../TagTests/Include/CompileIncludeTest.php | 12 ++++ .../templates/test_include_security.tpl | 1 + .../_Issues/419/ExtendsIssue419Test.php | 7 ++ 11 files changed, 96 insertions(+), 76 deletions(-) create mode 100644 changelog/GHSA-4rmg-292m-wg3w.md create mode 100644 tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping.tpl create mode 100644 tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping2.tpl create mode 100644 tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping3.tpl create mode 100644 tests/UnitTests/TemplateSource/TagTests/Include/templates/test_include_security.tpl diff --git a/changelog/GHSA-4rmg-292m-wg3w.md b/changelog/GHSA-4rmg-292m-wg3w.md new file mode 100644 index 000000000..c0e28bc16 --- /dev/null +++ b/changelog/GHSA-4rmg-292m-wg3w.md @@ -0,0 +1,2 @@ +- Fixed a code injection vulnerability in extends-tag. This addresses CVE-2024-35226. +- Added `$smarty->setCacheModifiedCheck()` setter for cache_modified_check \ No newline at end of file diff --git a/src/Compile/Tag/ExtendsTag.php b/src/Compile/Tag/ExtendsTag.php index dcdbbc976..0ec2cf2ba 100644 --- a/src/Compile/Tag/ExtendsTag.php +++ b/src/Compile/Tag/ExtendsTag.php @@ -32,7 +32,7 @@ class ExtendsTag extends Inheritance { * * @var array */ - protected $optional_attributes = ['extends_resource']; + protected $optional_attributes = []; /** * Attribute definition: Overwrites base class. @@ -64,29 +64,7 @@ public function compile($args, \Smarty\Compiler\Template $compiler, $parameter = } // add code to initialize inheritance $this->registerInit($compiler, true); - $file = trim($_attr['file'], '\'"'); - if (strlen($file) > 8 && substr($file, 0, 8) === 'extends:') { - // generate code for each template - $files = array_reverse(explode('|', substr($file, 8))); - $i = 0; - foreach ($files as $file) { - if ($file[0] === '"') { - $file = trim($file, '".'); - } else { - $file = "'{$file}'"; - } - $i++; - if ($i === count($files) && isset($_attr['extends_resource'])) { - $this->compileEndChild($compiler); - } - $this->compileInclude($compiler, $file); - } - if (!isset($_attr['extends_resource'])) { - $this->compileEndChild($compiler); - } - } else { - $this->compileEndChild($compiler, $_attr['file']); - } + $this->compileEndChild($compiler, $_attr['file']); return ''; } @@ -106,42 +84,4 @@ private function compileEndChild(\Smarty\Compiler\Template $compiler, $template (isset($template) ? ", {$template}, \$_smarty_current_dir" : '') . ");\n?>" ); } - - /** - * Add code for including subtemplate to end of template - * - * @param \Smarty\Compiler\Template $compiler - * @param string $template subtemplate name - * - * @throws \Smarty\CompilerException - * @throws \Smarty\Exception - */ - private function compileInclude(\Smarty\Compiler\Template $compiler, $template) { - $compiler->getParser()->template_postfix[] = new \Smarty\ParseTree\Tag( - $compiler->getParser(), - $compiler->compileTag( - 'include', - [ - $template, - ['scope' => 'parent'], - ] - ) - ); - } - - /** - * Create source code for {extends} from source components array - * - * @param \Smarty\Template $template - * - * @return string - */ - public static function extendsSourceArrayCode(\Smarty\Template $template) { - $resources = []; - foreach ($template->getSource()->components as $source) { - $resources[] = $source->resource; - } - return $template->getLeftDelimiter() . 'extends file=\'extends:' . join('|', $resources) . - '\' extends_resource=true' . $template->getRightDelimiter(); - } } diff --git a/src/Compiler/Template.php b/src/Compiler/Template.php index b775abba0..1d4aa2447 100644 --- a/src/Compiler/Template.php +++ b/src/Compiler/Template.php @@ -403,21 +403,37 @@ public function compileTemplateSource(\Smarty\Template $template, \Smarty\Compil } // get template source if (!empty($this->template->getSource()->components)) { - // we have array of inheritance templates by extends: resource - // generate corresponding source code sequence - $_content = - ExtendsTag::extendsSourceArrayCode($this->template); + + $_compiled_code = 'getInheritance()->init($_smarty_tpl, true); ?>'; + + $i = 0; + $reversed_components = array_reverse($this->template->getSource()->components); + foreach ($reversed_components as $source) { + $i++; + if ($i === count($reversed_components)) { + $_compiled_code .= 'getInheritance()->endChild($_smarty_tpl); ?>'; + } + $_compiled_code .= $this->compileTag( + 'include', + [ + var_export($source->resource, true), + ['scope' => 'parent'], + ] + ); + } + $_compiled_code = $this->smarty->runPostFilters($_compiled_code, $this->template); } else { // get template source $_content = $this->template->getSource()->getContent(); + $_compiled_code = $this->smarty->runPostFilters( + $this->doCompile( + $this->smarty->runPreFilters($_content, $this->template), + true + ), + $this->template + ); } - $_compiled_code = $this->smarty->runPostFilters( - $this->doCompile( - $this->smarty->runPreFilters($_content, $this->template), - true - ), - $this->template - ); + } catch (\Exception $e) { if ($this->smarty->debugging) { $this->smarty->getDebug()->end_compile($this->template); diff --git a/src/Smarty.php b/src/Smarty.php index dbb917262..9c27d7a83 100644 --- a/src/Smarty.php +++ b/src/Smarty.php @@ -2211,5 +2211,14 @@ private function returnOrCreateTemplate($template, $cache_id = null, $compile_id return $template; } + /** + * Sets if Smarty should check If-Modified-Since headers to determine cache validity. + * @param bool $cache_modified_check + * @return void + */ + public function setCacheModifiedCheck($cache_modified_check): void { + $this->cache_modified_check = (bool) $cache_modified_check; + } + } diff --git a/tests/UnitTests/TemplateSource/TagTests/BockExtend/CompileBlockExtendsTest.php b/tests/UnitTests/TemplateSource/TagTests/BockExtend/CompileBlockExtendsTest.php index 1b0ee50f9..6f0a798f5 100644 --- a/tests/UnitTests/TemplateSource/TagTests/BockExtend/CompileBlockExtendsTest.php +++ b/tests/UnitTests/TemplateSource/TagTests/BockExtend/CompileBlockExtendsTest.php @@ -1193,8 +1193,38 @@ public function dataTestBlockNocache() ); } - public function testBlockWithAssign() { - $this->assertEquals('Captured content is: Content with lots of html here', $this->smarty->fetch('038_child.tpl')); - } + public function testBlockWithAssign() { + $this->assertEquals('Captured content is: Content with lots of html here', $this->smarty->fetch('038_child.tpl')); + } + + /** + * Test escaping of file parameter + */ + public function testEscaping() + { + $this->expectException(\Smarty\Exception::class); + $this->expectExceptionMessageMatches('/Unable to load.*/'); + $this->assertEquals('hello world', $this->smarty->fetch('escaping.tpl')); + } + + /** + * Test escaping of file parameter 2 + */ + public function testEscaping2() + { + $this->expectException(\Smarty\Exception::class); + $this->expectExceptionMessageMatches('/Unable to load.*/'); + $this->assertEquals('hello world', $this->smarty->fetch('escaping2.tpl')); + } + + /** + * Test escaping of file parameter 3 + */ + public function testEscaping3() + { + $this->expectException(\Smarty\Exception::class); + $this->expectExceptionMessageMatches('/Unable to load.*/'); + $this->assertEquals('hello world', $this->smarty->fetch('escaping3.tpl')); + } } diff --git a/tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping.tpl b/tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping.tpl new file mode 100644 index 000000000..79c52e0aa --- /dev/null +++ b/tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping.tpl @@ -0,0 +1 @@ +{extends "extends:helloworld.tpl', var_dump(shell_exec('ls')), 1, 2, 3);}}?>"} \ No newline at end of file diff --git a/tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping2.tpl b/tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping2.tpl new file mode 100644 index 000000000..d72a57188 --- /dev/null +++ b/tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping2.tpl @@ -0,0 +1 @@ +{extends 'extends:"helloworld.tpl\', var_dump(shell_exec(\'ls\')), 1, 2, 3);}}?>'} \ No newline at end of file diff --git a/tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping3.tpl b/tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping3.tpl new file mode 100644 index 000000000..96372c82a --- /dev/null +++ b/tests/UnitTests/TemplateSource/TagTests/BockExtend/templates/escaping3.tpl @@ -0,0 +1 @@ +{extends file='extends:"helloworld.tpl'|cat:"', var_dump(shell_exec('ls')), 1, 2, 3);}}?>"} \ No newline at end of file diff --git a/tests/UnitTests/TemplateSource/TagTests/Include/CompileIncludeTest.php b/tests/UnitTests/TemplateSource/TagTests/Include/CompileIncludeTest.php index 8a7cb78e5..bd6eeaf40 100644 --- a/tests/UnitTests/TemplateSource/TagTests/Include/CompileIncludeTest.php +++ b/tests/UnitTests/TemplateSource/TagTests/Include/CompileIncludeTest.php @@ -82,6 +82,18 @@ public function testSpacing_001V3($merge, $caching, $text) $this->assertEquals('I1I2I3', $content, $text); } + /** + * test template name escaping + */ + public function testIncludeFilenameEscaping() + { + $this->expectException(\Smarty\Exception::class); + $this->expectExceptionMessageMatches('/Unable to load.*/'); + $tpl = $this->smarty->createTemplate('test_include_security.tpl'); + $content = $this->smarty->fetch($tpl); + $this->assertEquals("hello world", $content); + } + /** * test standard output * diff --git a/tests/UnitTests/TemplateSource/TagTests/Include/templates/test_include_security.tpl b/tests/UnitTests/TemplateSource/TagTests/Include/templates/test_include_security.tpl new file mode 100644 index 000000000..47d83bc14 --- /dev/null +++ b/tests/UnitTests/TemplateSource/TagTests/Include/templates/test_include_security.tpl @@ -0,0 +1 @@ +{include file="helloworld.tpl', var_dump(shell_exec('ls')), 1, 2, 3);}}?>"} diff --git a/tests/UnitTests/TemplateSource/_Issues/419/ExtendsIssue419Test.php b/tests/UnitTests/TemplateSource/_Issues/419/ExtendsIssue419Test.php index 2079fc0a6..9f6d81643 100644 --- a/tests/UnitTests/TemplateSource/_Issues/419/ExtendsIssue419Test.php +++ b/tests/UnitTests/TemplateSource/_Issues/419/ExtendsIssue419Test.php @@ -32,4 +32,11 @@ public function testextends419() $this->assertEquals('child', $this->smarty->fetch('extends:001_parent.tpl|001_child.tpl')); } + public function testextendsSecurity() + { + $this->expectException(\Smarty\Exception::class); + $this->expectExceptionMessageMatches('/Unable to load.*/'); + $this->assertEquals('child', $this->smarty->fetch('string:{include "001_parent.tpl\', var_dump(shell_exec(\'ls\')), 1, 2, 3);}}?>"}')); + } + }