From 0397c581db26928768bd54194a312b820af6b582 Mon Sep 17 00:00:00 2001 From: Adrien LUCAS Date: Tue, 10 Nov 2020 11:23:21 +0100 Subject: [PATCH] [tainting] Allow Twig\Environment::render to be tainted even with a variable as template name (#97) Allow Twig\Environment::render to be tainted even with a variable as template parameters Allow using a variable as template name for CachedTemplatesTainter too Add TwigUtils::extractTemplateNameFromExpression tests --- src/Twig/AnalyzedTemplatesTainter.php | 65 +++++++++++--- src/Twig/CachedTemplatesTainter.php | 9 +- src/Twig/TemplateFileAnalyzer.php | 5 ++ src/Twig/TwigUtils.php | 31 +++++++ .../TwigTaintingWithAnalyzer.feature | 20 +++++ .../TwigTaintingWithCachedTemplates.feature | 21 +++++ tests/unit/Symfony/TwigUtilsTest.php | 90 +++++++++++++++++++ 7 files changed, 221 insertions(+), 20 deletions(-) create mode 100644 src/Twig/TwigUtils.php create mode 100644 tests/unit/Symfony/TwigUtilsTest.php diff --git a/src/Twig/AnalyzedTemplatesTainter.php b/src/Twig/AnalyzedTemplatesTainter.php index a41eeaac..c3968a40 100644 --- a/src/Twig/AnalyzedTemplatesTainter.php +++ b/src/Twig/AnalyzedTemplatesTainter.php @@ -7,12 +7,16 @@ use PhpParser\Node\Expr; use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\MethodCall; -use PhpParser\Node\Scalar\String_; +use PhpParser\Node\Expr\Variable; use Psalm\Codebase; use Psalm\Context; +use Psalm\Internal\DataFlow\DataFlowNode; use Psalm\Plugin\Hook\AfterMethodCallAnalysisInterface; use Psalm\StatementsSource; +use Psalm\Type\Atomic\TKeyedArray; use Psalm\Type\Union; +use RuntimeException; +use SplObjectStorage; use Twig\Environment; /** @@ -26,25 +30,60 @@ public static function afterMethodCallAnalysis(Expr $expr, string $method_id, st if ( null === $codebase->taint_flow_graph || !$expr instanceof MethodCall || $method_id !== Environment::class.'::render' || empty($expr->args) - || !isset($expr->args[0]->value) || !$expr->args[0]->value instanceof String_ - || !isset($expr->args[1]->value) || !$expr->args[1]->value instanceof Array_ + || !isset($expr->args[0]->value) + || !isset($expr->args[1]->value) ) { return; } - $template_name = $expr->args[0]->value->value; - $twig_arguments_type = $statements_source->getNodeTypeProvider()->getType($expr->args[1]->value); + $templateName = TwigUtils::extractTemplateNameFromExpression($expr->args[0]->value, $statements_source); + $templateParameters = self::generateTemplateParameters($expr->args[1]->value, $statements_source); + foreach ($templateParameters as $sourceNode) { + $parameterName = $templateParameters[$sourceNode]; + $label = $argumentId = strtolower($templateName).'#'.strtolower($parameterName); + $destinationNode = new DataFlowNode($argumentId, $label, null, null); - if (null === $twig_arguments_type) { - return; + $codebase->taint_flow_graph->addPath($sourceNode, $destinationNode, 'arg'); + } + } + + /** + * @return SplObjectStorage + */ + private static function generateTemplateParameters(Expr $templateParameters, StatementsSource $source): SplObjectStorage + { + $type = $source->getNodeTypeProvider()->getType($templateParameters); + if (null === $type) { + throw new RuntimeException(sprintf('Can not retrieve type for the given expression (%s)', get_class($templateParameters))); } - foreach ($twig_arguments_type->parent_nodes as $source_taint) { - preg_match('/array\[\'([a-zA-Z]+)\'\]/', $source_taint->label, $matches); - $sink_taint = TemplateFileAnalyzer::getTaintNodeForTwigNamedVariable( - $template_name, $matches[1] - ); - $codebase->taint_flow_graph->addPath($source_taint, $sink_taint, 'arg'); + if ($templateParameters instanceof Array_) { + /** @var SplObjectStorage $parameters */ + $parameters = new SplObjectStorage(); + foreach ($type->parent_nodes as $node) { + if (preg_match('/array\[\'([a-zA-Z]+)\'\]/', $node->label, $matches)) { + $parameters[$node] = $matches[1]; + } + } + + return $parameters; } + + if ($templateParameters instanceof Variable && array_key_exists('array', $type->getAtomicTypes())) { + /** @var TKeyedArray $arrayValues */ + $arrayValues = $type->getAtomicTypes()['array']; + + /** @var SplObjectStorage $parameters */ + $parameters = new SplObjectStorage(); + foreach ($arrayValues->properties as $parameterName => $parameterType) { + foreach ($parameterType->parent_nodes as $node) { + $parameters[$node] = (string) $parameterName; + } + } + + return $parameters; + } + + throw new RuntimeException(sprintf('Can not retrieve template parameters from given expression (%s)', get_class($templateParameters))); } } diff --git a/src/Twig/CachedTemplatesTainter.php b/src/Twig/CachedTemplatesTainter.php index 120445ec..2e2d59a4 100644 --- a/src/Twig/CachedTemplatesTainter.php +++ b/src/Twig/CachedTemplatesTainter.php @@ -7,7 +7,6 @@ use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Identifier; -use PhpParser\Node\Scalar\String_; use Psalm\CodeLocation; use Psalm\Context; use Psalm\Internal\Analyzer\Statements\Expression\Call\MethodCallAnalyzer; @@ -59,12 +58,8 @@ public static function getMethodReturnType( isset($call_args[1]) ? [$call_args[1]] : [] ); - $firstArgument = $call_args[0]->value; - if (!$firstArgument instanceof String_) { - return null; - } - - $cacheClassName = CachedTemplatesMapping::getCacheClassName($firstArgument->value); + $templateName = TwigUtils::extractTemplateNameFromExpression($call_args[0]->value, $source); + $cacheClassName = CachedTemplatesMapping::getCacheClassName($templateName); $context->vars_in_scope['$__fake_twig_env_var__'] = new Union([ new TNamedObject($cacheClassName), diff --git a/src/Twig/TemplateFileAnalyzer.php b/src/Twig/TemplateFileAnalyzer.php index 96703093..b6265c7a 100644 --- a/src/Twig/TemplateFileAnalyzer.php +++ b/src/Twig/TemplateFileAnalyzer.php @@ -11,6 +11,11 @@ use Twig\Loader\FilesystemLoader; use Twig\NodeTraverser; +/** + * This class is to be used as a "checker" for the `.twig` files in the psalm configuration. + * + * @psalm-suppress UnusedClass + */ class TemplateFileAnalyzer extends FileAnalyzer { public function analyze( diff --git a/src/Twig/TwigUtils.php b/src/Twig/TwigUtils.php new file mode 100644 index 00000000..775dca0b --- /dev/null +++ b/src/Twig/TwigUtils.php @@ -0,0 +1,31 @@ +getNodeTypeProvider()->getType($templateName) ?? new Union([new TNull()]); + $templateName = array_values($type->getAtomicTypes())[0]; + } + + if (!$templateName instanceof String_ && !$templateName instanceof TLiteralString) { + throw new RuntimeException(sprintf('Can not retrieve template name from given expression (%s)', get_class($templateName))); + } + + return $templateName->value; + } +} diff --git a/tests/acceptance/acceptance/TwigTaintingWithAnalyzer.feature b/tests/acceptance/acceptance/TwigTaintingWithAnalyzer.feature index fac4a93a..9c84eae6 100644 --- a/tests/acceptance/acceptance/TwigTaintingWithAnalyzer.feature +++ b/tests/acceptance/acceptance/TwigTaintingWithAnalyzer.feature @@ -83,6 +83,26 @@ Feature: Twig tainting with analyzer | TaintedInput | Detected tainted html | And I see no other errors + Scenario: One tainted parameter (in a variable) of the twig template (named in a variable) is displayed with only the raw filter + Given I have the following code + """ + $untrustedParameters = ['untrusted' => $_GET['untrusted']]; + $template = 'index.html.twig'; + + twig()->render($template, $untrustedParameters); + """ + And I have the following "index.html.twig" template + """ +

+ {{ untrusted|raw }} +

+ """ + When I run Psalm with taint analysis + Then I see these errors + | Type | Message | + | TaintedInput | Detected tainted html | + And I see no other errors + Scenario: One tainted parameter of the twig rendering is displayed with some filter followed by the raw filter Given I have the following code """ diff --git a/tests/acceptance/acceptance/TwigTaintingWithCachedTemplates.feature b/tests/acceptance/acceptance/TwigTaintingWithCachedTemplates.feature index 3ad9d88a..03c80f67 100644 --- a/tests/acceptance/acceptance/TwigTaintingWithCachedTemplates.feature +++ b/tests/acceptance/acceptance/TwigTaintingWithCachedTemplates.feature @@ -86,6 +86,27 @@ Feature: Twig tainting with cached templates | TaintedInput | Detected tainted html | And I see no other errors + Scenario: One tainted parameter (in a variable) of the twig template (named in a variable) is displayed with only the raw filter + Given I have the following code + """ + $untrustedParameters = ['untrusted' => $_GET['untrusted']]; + $template = 'index.html.twig'; + + twig()->render($template, $untrustedParameters); + """ + And I have the following "index.html.twig" template + """ +

+ {{ untrusted|raw }} +

+ """ + And the "index.html.twig" template is compiled in the "cache/twig/" directory + When I run Psalm with taint analysis + Then I see these errors + | Type | Message | + | TaintedInput | Detected tainted html | + And I see no other errors + Scenario: The template has a taint sink and is aliased Given I have the following code """ diff --git a/tests/unit/Symfony/TwigUtilsTest.php b/tests/unit/Symfony/TwigUtilsTest.php new file mode 100644 index 00000000..42a65812 --- /dev/null +++ b/tests/unit/Symfony/TwigUtilsTest.php @@ -0,0 +1,90 @@ +args[0]->value, $statements_source)); + } + }; + + $statementsAnalyzer = self::createStatementsAnalyzer($assertionHook); + $statementsAnalyzer->analyze($statements, new Context()); + } + + public function provideExpressions(): array + { + return [ + ['dummy("expected.twig");'], + ['dummy(\'expected.twig\');'], + ['$a = "expected.twig"; dummy($a);'], + ]; + } + + public function testItThrowsAnExceptionWhenTryingToExtractTemplateNameFromAnUnsupportedExpression() + { + $code = 'args[0]->value, $statements_source); + } + }; + + $statementsAnalyzer = self::createStatementsAnalyzer($assertionHook); + + $this->expectException(RuntimeException::class); + $statementsAnalyzer->analyze($statements, new Context()); + } + + private static function createStatementsAnalyzer(AfterEveryFunctionCallAnalysisInterface $hook): StatementsAnalyzer + { + $config = (function () { return new self(); })->bindTo(null, Config::class)(); + $config->after_every_function_checks[] = $hook; + + $nullFileAnalyzer = new FileAnalyzer(new ProjectAnalyzer($config, new Providers(new FileProvider())), '', ''); + $nullFileAnalyzer->codebase->functions->addGlobalFunction('dummy', new FunctionStorage()); + $nullFileAnalyzer->codebase->file_storage_provider->create(''); + + $nodeData = new NodeDataProvider(); + (function () use ($nodeData) { + $this->node_data = $nodeData; + })->bindTo($nullFileAnalyzer, $nullFileAnalyzer)(); + + return new StatementsAnalyzer($nullFileAnalyzer, $nodeData); + } +}