Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix backtick analysis #10409

Merged
merged 4 commits into from
Nov 26, 2023
Merged

Fix backtick analysis #10409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
35f194e
Fix backtick analysis
danog Nov 26, 2023
e386b07
Fix
danog Nov 26, 2023
1e2e224
Fix wrong type
danog Nov 26, 2023
73a340f
Cleanup
danog Nov 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion dictionaries/PropertyMap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@
'items' => 'array<int, PhpParser\\Node\\Expr\\ArrayItem|null>',
],
'phpparser\\node\\expr\\shellexec' => [
'parts' => 'list<PhpParser\\Node>',
'parts' => 'list<PhpParser\\Node\\Expr>',
],
'phpparser\\node\\matcharm' => [
'conds' => 'null|non-empty-list<PhpParser\\Node\\Expr>',
Expand Down
97 changes: 17 additions & 80 deletions src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
use Psalm\Internal\Analyzer\Statements\Expression\BinaryOpAnalyzer;
use Psalm\Internal\Analyzer\Statements\Expression\BitwiseNotAnalyzer;
use Psalm\Internal\Analyzer\Statements\Expression\BooleanNotAnalyzer;
use Psalm\Internal\Analyzer\Statements\Expression\Call\ArgumentAnalyzer;
use Psalm\Internal\Analyzer\Statements\Expression\Call\FunctionCallAnalyzer;
use Psalm\Internal\Analyzer\Statements\Expression\Call\MethodCallAnalyzer;
use Psalm\Internal\Analyzer\Statements\Expression\Call\NewAnalyzer;
Expand Down Expand Up @@ -43,20 +42,18 @@
use Psalm\Internal\Analyzer\Statements\Expression\YieldAnalyzer;
use Psalm\Internal\Analyzer\Statements\Expression\YieldFromAnalyzer;
use Psalm\Internal\Analyzer\StatementsAnalyzer;
use Psalm\Internal\Codebase\TaintFlowGraph;
use Psalm\Internal\DataFlow\DataFlowNode;
use Psalm\Internal\DataFlow\TaintSink;
use Psalm\Internal\FileManipulation\FileManipulationBuffer;
use Psalm\Internal\Type\TemplateResult;
use Psalm\Issue\ForbiddenCode;
use Psalm\Issue\UnrecognizedExpression;
use Psalm\Issue\UnsupportedReferenceUsage;
use Psalm\IssueBuffer;
use Psalm\Node\Expr\VirtualFuncCall;
use Psalm\Node\Scalar\VirtualEncapsed;
use Psalm\Node\VirtualArg;
use Psalm\Node\VirtualName;
use Psalm\Plugin\EventHandler\Event\AfterExpressionAnalysisEvent;
use Psalm\Plugin\EventHandler\Event\BeforeExpressionAnalysisEvent;
use Psalm\Storage\FunctionLikeParameter;
use Psalm\Type;
use Psalm\Type\TaintKind;

use function get_class;
use function in_array;
Expand Down Expand Up @@ -378,80 +375,20 @@ private static function handleExpression(
}

if ($stmt instanceof PhpParser\Node\Expr\ShellExec) {
if ($statements_analyzer->data_flow_graph) {
$call_location = new CodeLocation($statements_analyzer->getSource(), $stmt);

if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) {
$sink = TaintSink::getForMethodArgument(
'shell_exec',
'shell_exec',
0,
null,
$call_location,
);

$sink->taints = [TaintKind::INPUT_SHELL];

$statements_analyzer->data_flow_graph->addSink($sink);
}

foreach ($stmt->parts as $part) {
if ($part instanceof PhpParser\Node\Expr\Variable) {
if (self::analyze($statements_analyzer, $part, $context) === false) {
break;
}

$expr_type = $statements_analyzer->node_data->getType($part);
if ($expr_type === null) {
break;
}

$shell_exec_param = new FunctionLikeParameter(
'var',
false,
);

if (ArgumentAnalyzer::verifyType(
$statements_analyzer,
$expr_type,
Type::getString(),
null,
'shell_exec',
null,
0,
$call_location,
$stmt,
$context,
$shell_exec_param,
false,
null,
true,
true,
new CodeLocation($statements_analyzer, $stmt),
) === false) {
return false;
}

foreach ($expr_type->parent_nodes as $parent_node) {
$statements_analyzer->data_flow_graph->addPath(
$parent_node,
new DataFlowNode('variable-use', 'variable use', null),
'variable-use',
);
}
}
}
}

IssueBuffer::maybeAdd(
new ForbiddenCode(
'Use of shell_exec',
new CodeLocation($statements_analyzer->getSource(), $stmt),
),
$statements_analyzer->getSuppressedIssues(),
$concat = new VirtualEncapsed($stmt->parts, $stmt->getAttributes());
$virtual_call = new VirtualFuncCall(new VirtualName(['shell_exec']), [
new VirtualArg($concat),
], $stmt->getAttributes());
return self::handleExpression(
$statements_analyzer,
$virtual_call,
$context,
$array_assignment,
$global_context,
$from_stmt,
$template_result,
$assigned_to_reference,
);

return true;
}

if ($stmt instanceof PhpParser\Node\Expr\Print_) {
Expand Down
8 changes: 8 additions & 0 deletions tests/ExpressionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,14 @@ public function providerValidCodeParse(): iterable
'$a===' => 'array{9223372036854775806: 0, 9223372036854775807: 1}',
],
];
yield 'shellExecConcatInt' => [
'code' => <<<'PHP'
<?php
$a = 123;
/** @psalm-suppress ForbiddenCode */
`ls $a`;
PHP,
];
}

/**
Expand Down
8 changes: 8 additions & 0 deletions tests/TaintTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2306,6 +2306,14 @@ function foo(array $arr) : void {
',
'error_message' => 'TaintedShell',
],
'shellExecBacktickConcat' => [
'code' => '<?php

$input = $_GET["input"];
$x = `ls $input`;
',
'error_message' => 'TaintedShell',
],
/*
// TODO: Stubs do not support this type of inference even with $this->message = $message.
// Most uses of getMessage() would be with caught exceptions, so this is not representative of real code.
Expand Down