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 2 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
98 changes: 18 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,21 @@ 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(),
/** @psalm-suppress ArgumentTypeCoercion $stmt->parts is untyped, but it's a list of expressions */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird, shellExec::$parts should be typed here: https://github.com/vimeo/psalm/blob/5.x/dictionaries/PropertyMap.php#L387

What's the error exactly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type was wrong, fixed it now, it must be identical to the parts of an Encapsed

$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
16 changes: 16 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,22 @@ function foo(array $arr) : void {
',
'error_message' => 'TaintedShell',
],
'shellExecBacktickConcat' => [
'code' => '<?php

$input = $_GET["input"];
$x = `ls $input`;
',
'error_message' => 'TaintedShell',
],
'shellExecBacktickConcatInt' => [
'code' => '<?php

$input = (int) $_GET["input"];
$x = `ls /path/$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