CVE-2021-41170: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: neoan3-apps/template is a neoan3 minimal template engine. Versions prior to 1.1.1 have allowed for passing in closures directly into the template engine. As a result values that are callable are executed by the template engine. The issue arises if a value has the same name as a method or function in scope and can therefore be executed either by mistake or maliciously. In theory all users of the package are affected as long as they either deal with direct user input or database values. A multi-step attack on is therefore plausible. Version 1.1.1 has addressed this vulnerability. Unfortunately only working with hardcoded values is safe in prior versions. As this likely defeats the purpose of a template engine, please upgrade.
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 9.8
Published: 2021-11-08T19:15Z
Repository: https://github.com/sroehrl/neoan3-template
Language: PHP
Files changed: Template.php, TemplateFunctions.php, composer.json, tests/TemplateTest.php
Commit date: 2021-10-21 19:25:24 -0400
Commit message: SECURITY: allowing for direct injection (Issue #8)

Diff: diff --git a/Template.php b/Template.php
index f9a8613..746cb1d 100644
--- a/Template.php
+++ b/Template.php
@@ -22,16 +22,14 @@ class Template
         }
         $saveOpening = preg_quote(TemplateFunctions::getDelimiters()[0]);
         $saveClosing = preg_quote(TemplateFunctions::getDelimiters()[1]);
         foreach ($flatArray as $flatKey => $value){
             $flatKey = preg_replace('/[\/\.\\\+\*\?\[\^\]\$\(\)\{\}\=\!\<\>\|\:\-]/', "\\\\$0",$flatKey);
-            if(is_callable($value)){
-                TemplateFunctions::registerClosure($flatKey,$value);
-            } else {
-                $content = preg_replace("/$saveOpening\s*$flatKey\s*$saveClosing/", $value, $content);
-                $content = TemplateFunctions::tryClosures($flatArray, $content, false);
-            }
+            // PATCHED: direct function injection is not allowed anymore
+            $content = preg_replace("/$saveOpening\s*$flatKey\s*$saveClosing/", $value, $content);
+            $content = TemplateFunctions::tryClosures($flatArray, $content, false);
+
         }
 
         return $content;
     }
 
diff --git a/TemplateFunctions.php b/TemplateFunctions.php
index 783dc49..48941eb 100644
--- a/TemplateFunctions.php
+++ b/TemplateFunctions.php
@@ -101,11 +101,11 @@ class TemplateFunctions
     {
         $pattern = '/';
         if (!$pure) {
             $pattern .= preg_quote(self::$registeredDelimiters[0]) . "\s*";
         }
-        $pattern .= "$closureName\(([a-z0-9,\.\s]+)\)";
+        $pattern .= "$closureName\(([a-z0-9,\.\s_]+)\)";
         if (!$pure) {
             $pattern .= "\s*" . preg_quote(self::$registeredDelimiters[1]);
         }
         return $pattern . "/i";
     }
@@ -199,11 +199,10 @@ class TemplateFunctions
     {
         $bool = false;
         foreach ($flatArray as $key => $value) {
             $pattern = '/' . $key . '([^.]|$)/';
             if (preg_match($pattern, $expression, $matches)) {
-
                 switch (gettype($flatArray[$key])) {
                     case 'boolean':
                         $expression = str_replace($key, $flatArray[$key] ? 'true' : 'false', $expression);
                         break;
                     case 'NULL':
@@ -239,15 +238,16 @@ class TemplateFunctions
         $hits = $xPath->query("//*[@n-if]");
         if ($hits->length < 1) {
             return $content;
         }
 
+        $array = Template::flattenArray($array);
+        // important: first try closures
+        $array = array_merge(self::$registeredClosures, $array);
         foreach ($hits as $hit) {
             $expression = $hit->getAttribute('n-if');
-            $array = Template::flattenArray($array);
             $bool = self::evaluateTypedCondition($array, $expression);
-
             if (!$bool) {
                 $hit->parentNode->removeChild($hit);
             } else {
                 $hit->removeAttribute('n-if');
             }
diff --git a/composer.json b/composer.json
index 97ea57b..dffe8ac 100644
--- a/composer.json
+++ b/composer.json
@@ -1,15 +1,14 @@
 {
     "name": "neoan3-apps/template",
     "description": "neoan3 minimal template engine",
-    "version": "1.1.0",
+    "version": "1.1.1",
     "license": "MIT",
     "autoload": {
         "psr-4": {
             "Neoan3\\Apps\\": "./"
         }
-
     },
     "require": {
         "ext-openssl": "*",
         "ext-mbstring": "*",
         "ext-json": "*",
diff --git a/tests/TemplateTest.php b/tests/TemplateTest.php
index 20e4e2e..dda1d75 100644
--- a/tests/TemplateTest.php
+++ b/tests/TemplateTest.php
@@ -111,15 +111,15 @@ class TemplateTest extends TestCase
     }
 
     public function testCallback()
     {
         $array = [
-            'myFunc' => function ($x) {
-                return strtoupper($x);
-            },
             'some' => 'value'
         ];
+        TemplateFunctions::registerClosure('myFunc',function($x){
+            return strtoupper($x);
+        });
         $t = Template::embraceFromFile('callback.html', $array);
 
         $this->assertStringContainsString('<p>VALUE</p>', $t);
         $this->assertStringContainsString('<li>show me</li>', $t);
     }

Vulnerable code:             if(is_callable($value)){
                TemplateFunctions::registerClosure($flatKey,$value);
            } else {
                $content = preg_replace("/$saveOpening\s*$flatKey\s*$saveClosing/", $value, $content);
                $content = TemplateFunctions::tryClosures($flatArray, $content, false);
            }
        $pattern .= "$closureName\(([a-z0-9,\.\s]+)\)";

            $array = Template::flattenArray($array);

    "version": "1.1.0",

            'myFunc' => function ($x) {
                return strtoupper($x);
            },
Fixed code:             // PATCHED: direct function injection is not allowed anymore
            $content = preg_replace("/$saveOpening\s*$flatKey\s*$saveClosing/", $value, $content);
            $content = TemplateFunctions::tryClosures($flatArray, $content, false);

        $pattern .= "$closureName\(([a-z0-9,\.\s_]+)\)";
        $array = Template::flattenArray($array);
        // important: first try closures
        $array = array_merge(self::$registeredClosures, $array);
    "version": "1.1.1",
        TemplateFunctions::registerClosure('myFunc',function($x){
            return strtoupper($x);
        });