Fix modifier default implementation and improve unit tests

Fixes #336

.gitignore

@@ -11,3 +11,4 @@ utilies/*.php
 phpunit*
 vendor/*
 composer.lock
+/composer.phar

CHANGELOG.md

@@ -6,6 +6,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+- default modifier uses empty instead of @ error suppression modifier when testing a variable https://github.com/smarty-php/smarty/issues/336
+- modifier escape now triggers a E_USER_NOTICE when an unsupported escape type is used https://github.com/smarty-php/smarty/pull/649
+
+## [3.1.39] - 2021-02-17
+
+### Security
+- Prevent access to `$smarty.template_object` in sandbox mode. This addresses CVE-2021-26119.
+- Fixed code injection vulnerability by using illegal function names in `{function name='blah'}{/function}`. This addresses CVE-2021-26120.
+
 ## [3.1.38] - 2021-01-08
 
 ### Fixed

SECURITY.md

@@ -0,0 +1,19 @@
+# Security Policy
+
+## Supported Versions
+
+Smarty currently supports the latest minor version of Smarty 3 and Smarty 4. (Smarty 4 has not been released yet.)
+
+| Version | Supported          |
+| ------- | ------------------ |
+| 4.0.x   | :white_check_mark: |
+| 3.1.x   | :white_check_mark: |
+| < 3.1   | :x:                |
+
+## Reporting a Vulnerability
+
+ If you have discovered a security issue with Smarty, please contact us at mail [at] simonwisselink.nl. Do not 
+ disclose your findings publicly and PLEASE PLEASE do not file an Issue.
+ 
+We will try to confirm the vulnerability and develop a fix if appropriate. When we release the fix, we will publish 
+a security release. Please let us know if you want to be credited.

libs/Smarty.class.php

@@ -111,7 +111,7 @@ class Smarty extends Smarty_Internal_TemplateBase
     /**
      * smarty version
      */
-    const SMARTY_VERSION = '3.1.38';
+    const SMARTY_VERSION = '3.1.39';
     /**
      * define variable scopes
      */

libs/plugins/modifier.escape.php

@@ -250,6 +250,7 @@ function smarty_modifier_escape($string, $esc_type = 'html', $char_set = null, $
             }
             return $return;
         default:
+            trigger_error("escape: unsupported type: $esc_type - returning unmodified string", E_USER_NOTICE);
             return $string;
     }
 }

libs/plugins/modifiercompiler.default.php

@@ -18,7 +18,7 @@
  *
  * @return string with compiled code
  */
-function smarty_modifiercompiler_default($params)
+function smarty_modifiercompiler_default($params, Smarty_Internal_TemplateCompilerBase $compiler)
 {
     $output = $params[ 0 ];
     if (!isset($params[ 1 ])) {
@@ -26,7 +26,13 @@ function smarty_modifiercompiler_default($params)
     }
     array_shift($params);
     foreach ($params as $param) {
-        $output = '(($tmp = @' . $output . ')===null||$tmp===\'\' ? ' . $param . ' : $tmp)';
+
+        if ($compiler->syntaxMatchesVariable($output)) {
+            $output = '(!isset(' . $output . ') || ' . $output . ' === \'\' ? ' . $param . ' : ' . $output . ')';
+        } else {
+            $output = '(($tmp = ' . $output . ')===null||$tmp===\'\' ? ' . $param . ' : $tmp)';
+        }
+
     }
     return $output;
 }

libs/sysplugins/smarty_internal_compile_function.php

@@ -58,6 +58,11 @@ class Smarty_Internal_Compile_Function extends Smarty_Internal_CompileBase
         }
         unset($_attr[ 'nocache' ]);
         $_name = trim($_attr[ 'name' ], '\'"');
+
+        if (!preg_match('/^[a-zA-Z0-9_\x80-\xff]+$/', $_name)) {
+	        $compiler->trigger_template_error("Function name contains invalid characters: {$_name}", null, true);
+        }
+
         $compiler->parent_compiler->tpl_function[ $_name ] = array();
         $save = array(
             $_attr, $compiler->parser->current_buffer, $compiler->template->compiled->has_nocache_code,

libs/sysplugins/smarty_internal_compile_private_special_variable.php

@@ -81,6 +81,10 @@ class Smarty_Internal_Compile_Private_Special_Variable extends Smarty_Internal_C
                 case 'template':
                     return 'basename($_smarty_tpl->source->filepath)';
                 case 'template_object':
+                    if (isset($compiler->smarty->security_policy)) {
+                        $compiler->trigger_template_error("(secure mode) template_object not permitted");
+                        break;
+                    }
                     return '$_smarty_tpl';
                 case 'current_dir':
                     return 'dirname($_smarty_tpl->source->filepath)';

libs/sysplugins/smarty_internal_templatecompilerbase.php

@@ -674,7 +674,7 @@ abstract class Smarty_Internal_TemplateCompilerBase
 	 * @param $string
 	 * @return bool
 	 */
-	private function syntaxMatchesVariable($string) {
+	public function syntaxMatchesVariable($string) {
     	static $regex_pattern = '/^\$[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*((->)[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*|\[.*]*\])*$/';
     	return 1 === preg_match($regex_pattern, trim($string));
     }

make-release.sh

@@ -14,6 +14,6 @@ git pull
 git merge --no-ff "release/$1"
 git branch -d "release/$1"
 git tag -a "v$1" -m "Release $1"
-git push --follow-tags
 
 printf 'Done creating release %s\n' "$1"
+printf 'Run `git push --follow-tags origin` to publish it.\n'

tests/PHPUnit_Smarty.php

@@ -177,7 +177,6 @@ class PHPUnit_Smarty extends PHPUnit_Framework_TestCase
                 $this->smartyBC->setCacheDir(dirname(__FILE__) . '/cache');
             }
         }
-        $smarty = $this->getSmartyObj();
     }
 
     /**

tests/UnitTests/A_Core/MuteExpectedErrors/MuteExpectedErrorsTest.php

@@ -64,10 +64,10 @@ class MuteExpectedErrorsTest extends PHPUnit_Smarty
         $this->smarty->clearCompiledTemplate('default.tpl');
         $this->smarty->fetch('default.tpl');
 
-        $this->assertEquals(Smarty::$_IS_WINDOWS ? 2 : 2, count($this->_errors));
+        $this->assertEquals(0, count($this->_errors));
 
         @filemtime('ckxladanwijicajscaslyxck');
-        $this->assertEquals(Smarty::$_IS_WINDOWS ? 3 : 3, count($this->_errors));
+        $this->assertEquals(1, count($this->_errors));
 
         restore_error_handler();
     }
@@ -112,11 +112,11 @@ class MuteExpectedErrorsTest extends PHPUnit_Smarty
         $this->smarty->clearCompiledTemplate('default.tpl');
         $this->smarty->fetch('default.tpl');
 
-        $this->assertEquals(Smarty::$_IS_WINDOWS ? 2 : 2, count($this->_errors));
+        $this->assertEquals(0, count($this->_errors));
 
         @filemtime('ckxladanwijicajscaslyxck');
         $error = array(__FILE__ . ' line ' . (__LINE__ - 1));
-        $this->assertEquals(Smarty::$_IS_WINDOWS ? 3 : 3, count($this->_errors));
+        $this->assertEquals(1, count($this->_errors));
 
         restore_error_handler();
     }

tests/UnitTests/ResourceTests/File/FileResourceTest.php

@@ -110,6 +110,9 @@ class FileResourceTest extends PHPUnit_Smarty
         );
     }
 
+    /**
+     * @doesNotPerformAssertions
+     */
     public function testGetCompiledTimestampPrepare()
     {
         $tpl = $this->smarty->createTemplate('helloworld.tpl');
@@ -142,6 +145,9 @@ class FileResourceTest extends PHPUnit_Smarty
         $this->assertTrue($tpl->mustCompile());
     }
 
+    /**
+     * @doesNotPerformAssertions
+     */
     public function testMustCompileTouchedSourcePrepare()
     {
         // touch to prepare next test

tests/UnitTests/ResourceTests/Php/PhpResourceTest.php

@@ -161,7 +161,7 @@ class PhpResourceTest extends PHPUnit_Smarty
      *
      * @runInSeparateProcess
      * @preserveGlobalState disabled
-     *
+     * @doesNotPerformAssertions
      */
     public function testIsCachedTouchedSourcePrepare()
     {
@@ -212,6 +212,7 @@ class PhpResourceTest extends PHPUnit_Smarty
 
     /**
      * test $smarty->is_cached
+     * @doesNotPerformAssertions
      */
     public function testSmartyIsCachedPrepare()
     {

tests/UnitTests/SecurityTests/SecurityTest.php

@@ -382,6 +382,15 @@ class SecurityTest extends PHPUnit_Smarty
         $this->smarty->security_policy->trusted_uri = array();
         $this->assertContains('<title>Preface | Smarty</title>', $this->smarty->fetch('string:{fetch file="https://www.smarty.net/docs/en/preface.tpl"}'));
     }
+
+    /**
+     * In security mode, accessing $smarty.template_object should be illegal.
+     * @expectedException SmartyCompilerException
+     */
+    public function testSmartyTemplateObject() {
+        $this->smarty->display('string:{$smarty.template_object}');
+    }
+
 }
 
 class mysecuritystaticclass

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T1.tpl

@@ -0,0 +1 @@
+{* this is a comment *}

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T10.tpl

@@ -0,0 +1,7 @@
+=
+{* comment *}
+{* comment *}
+    b
+{* comment *}
+{* comment *}
+=

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T11.tpl

@@ -0,0 +1,7 @@
+=
+a
+{* comment 1 *}
+{* comment 2 *}
+{* comment 3 *}
+b
+=

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T12.tpl

@@ -0,0 +1,7 @@
+=
+a
+{* comment 1 *}
+ {* comment 2 *}
+{* comment 3 *}
+b
+=

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T13.tpl

@@ -0,0 +1,7 @@
+=
+a
+{* comment 1 *}
+{* comment 2 *} 
+{* comment 3 *}
+b
+=

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T14.tpl

@@ -0,0 +1,7 @@
+=
+a
+{* comment 1 *}
+ {* comment 2 *} 
+{* comment 3 *}
+b
+=

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T2.tpl

@@ -0,0 +1 @@
+{* another $foo comment *}

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T3.tpl

@@ -0,0 +1 @@
+{* another  comment *}some in between{* another  comment *}

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T4.tpl

@@ -0,0 +1,2 @@
+{* multi line 
+ comment *}

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T5.tpl

@@ -0,0 +1 @@
+{* /* foo * / *}

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T6.tpl

@@ -0,0 +1,2 @@
+A{* comment *}B
+C

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T7.tpl

@@ -0,0 +1,3 @@
+D{* comment *}
+{* comment *}E
+F

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T8.tpl

@@ -0,0 +1,2 @@
+G{* multi 
+line *}H

tests/UnitTests/TemplateSource/Comments/templates_tmp/testComments_T9.tpl

@@ -0,0 +1,3 @@
+I{* multi 
+line *}
+J

tests/UnitTests/TemplateSource/TagTests/PluginFunction/PluginFunctionHtmlImageTest.php

@@ -20,6 +20,9 @@ class PluginFunctionHtmlImageTest extends PHPUnit_Smarty
         $this->setUpSmarty(dirname(__FILE__));
     }
 
+    /**
+     * @doesNotPerformAssertions
+     */
     public function testFoo()
     {
         // TODO: UnitTests for {html_image}

tests/UnitTests/TemplateSource/TagTests/PluginModifier/PluginModifierDefaultTest.php

@@ -0,0 +1,151 @@
+<?php
+/**
+ * Smarty PHPunit tests of modifier
+ *
+ * @package PHPunit
+ */
+
+/**
+ * class for modifier tests
+ */
+class PluginModifierDefaultTest extends PHPUnit_Smarty
+{
+	public function setUp()
+	{
+		$this->setUpSmarty(dirname(__FILE__));
+		$this->smarty->setErrorReporting(E_ALL ^ E_NOTICE);
+	}
+
+	/**
+	 * @dataProvider        dataTestSimpleVars
+	 */
+	public function testSimpleVars($template, $expected)
+	{
+		$tpl = $this->smarty->createTemplate($template);
+		$this->smarty->assign('s', 'v0');
+		$this->assertEquals($expected, $this->smarty->fetch($tpl));
+	}
+
+	/**
+	 * Generates test data for ::testSimpleVars
+	 */
+	public function dataTestSimpleVars() {
+		return array(
+			array('string:{$s|default:"B"}', 'v0'),       // simple set variable
+			array('string:{$u|default:"B"}', 'B'),       // simple unset variable
+		);
+	}
+
+	/**
+	 * @dataProvider        dataTestSimpleVarsNoDefaultValue
+	 */
+	public function testSimpleVarsNoDefaultValue($template, $expected)
+	{
+		$tpl = $this->smarty->createTemplate($template);
+		$this->smarty->assign('s', 'v0');
+		$this->assertEquals($expected, $this->smarty->fetch($tpl));
+	}
+
+	/**
+	 * Generates test data for ::testSimpleVarsNoDefaultValue
+	 */
+	public function dataTestSimpleVarsNoDefaultValue() {
+		return array(
+			array('string:{$s|default}', 'v0'),       // simple set variable
+			array('string:{$u|default}', ''),       // simple unset variable
+		);
+	}
+
+	public function testEmptyString()
+	{
+		$tpl = $this->smarty->createTemplate('string:{$s|default:"B"}');
+		$this->smarty->assign('s', '');
+		$this->assertEquals('B', $this->smarty->fetch($tpl));
+	}
+
+	/**
+	 * @dataProvider dataFalsyValues
+	 */
+	public function testFalsyValues($falsyvalue, $expected)
+	{
+		$tpl = $this->smarty->createTemplate('string:{$s|default:"B"}');
+		$this->smarty->assign('s', $falsyvalue);
+		$this->assertEquals($expected, $this->smarty->fetch($tpl));
+	}
+
+	/**
+	 * Data for ::testFalsyValues
+	 */
+	public function dataFalsyValues() {
+		return array(
+			array(false, false),
+			array(0, 0),
+			array('0', '0'),
+			array(array(), 'Array'),
+			array(null, 'B'),
+			array('', 'B'),
+		);
+	}
+
+
+	public function testFunctionCall()
+	{
+		$tpl = $this->smarty->createTemplate('string:{strlen("a")|default:"B"}');
+		$this->assertEquals('1', $this->smarty->fetch($tpl));
+	}
+
+	public function testFunctionCallEmptyString()
+	{
+		$tpl = $this->smarty->createTemplate('string:{trim("")|default:"B"}');
+		$this->assertEquals('B', $this->smarty->fetch($tpl));
+	}
+
+	/**
+	 * @dataProvider        dataTestArrayObjectCombinations
+	 */
+	public function testArrayObjectCombinations($template, $expected)
+	{
+		$tpl = $this->smarty->createTemplate($template);
+		$this->smarty->assign('a', array(
+			'k' => (object) array ('v' => 'v3'),
+			'v' => 'v1'
+		));
+		$this->smarty->assign('o', (object) array (
+			'p' => array('v' => 'v4'),
+			'v' => 'v2'
+		));
+		$this->smarty->assign('k', 'k');
+		$this->smarty->assign('nk', 'nk');
+		$this->assertEquals($expected, $this->smarty->fetch($tpl));
+	}
+
+	/**
+	 * Generates test data for ::testArrayObjectCombinations
+	 */
+	public function dataTestArrayObjectCombinations() {
+		return array(
+			array('string:{$a.v|default:"B"}', 'v1'),     // array key set
+			array('string:{$o->v|default:"B"}', 'v2'),    // object property set
+			array('string:{$a.k->v|default:"B"}', 'v3'),  // complex combi of array and key access
+			array('string:{$a["k"]->v|default:"B"}', 'v3'),  // complex combi of array and key access
+			array('string:{$a[$k]->v|default:"B"}', 'v3'),  // complex combi of array and key access
+			array('string:{$a[$nk]->v|default:"B"}', 'B'),  // complex combi of array and key access
+			array('string:{$a[$u]->v|default:"B"}', 'B'),  // complex combi of array and key access
+			array('string:{$o->p.v|default:"B"}', 'v4'),  // complex combi of array and key access
+
+			array('string:{$a.c|default:"B"}', 'B'),     // array key not set
+			array('string:{$u.c|default:"B"}', 'B'),     // array not set, referencing key
+			array('string:{$o->u|default:"B"}', 'B'),    // object property not set
+			array('string:{$u->u|default:"B"}', 'B'),    // object not set
+			array('string:{$a.k->u|default:"B"}', 'B'),  // complex combi of array and key access with something unset
+			array('string:{$a["k"]->u|default:"B"}', 'B'),  // complex combi of array and key access with something unset
+			array('string:{$a[$k]->u|default:"B"}', 'B'),  // complex combi of array and key access with something unset
+			array('string:{$a[$nk]->u|default:"B"}', 'B'),  // complex combi of array and key access with something unset
+			array('string:{$a[$u]->u|default:"B"}', 'B'),  // complex combi of array and key access with something unset
+			array('string:{$a.u->u|default:"B"}', 'B'),  // complex combi of array and key access with something unset
+			array('string:{$o->p.u|default:"B"}', 'B'),  // complex combi of array and key access with something unset
+			array('string:{$o->u.u|default:"B"}', 'B'),  // complex combi of array and key access with something unset
+
+		);
+	}
+}

tests/UnitTests/TemplateSource/TagTests/TemplateFunction/CompileFunctionTest.php

@@ -431,5 +431,14 @@ class CompileFunctionTest extends PHPUnit_Smarty
                      array("{function name=simple}A{\$foo}\nC{/function}{call name='simple'}", "Abar\nC", 'T14', $i++),
                      array("{function name=simple}A\n{\$foo}\nC{/function}{call name='simple'}", "A\nbar\nC", 'T15', $i++),
         );
-            }
+    }
+
+    /**
+     * Test handling of function names that are a security risk
+     * @expectedException        SmartyCompilerException
+     */
+    public function testIllegalFunctionName() {
+	    $this->smarty->fetch('string:{function name=\'rce(){};echo "hi";function \'}{/function}');
+    }
+
 }

tests/UnitTests/TemplateSource/X_Scopes/ScopeTest.php

@@ -291,6 +291,9 @@ class ScopeTest extends PHPUnit_Smarty
                            'no  smarty', $i ++,),);
     }
 
+    /**
+     * @doesNotPerformAssertions
+     */
     public function testFunctionScope()
     {
         $this->smarty->assign('scope', 'none');