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.

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

Allow to implement custom taint type classes #10736

Open
wants to merge 1 commit into
base: 5.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions docs/security_analysis/custom_taint_kinds.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
# Custom Taint Kinds

[Psalm\Type\TaintKindRegistry](https://github.com/vimeo/psalm/blob/master/src/Psalm/Type/TaintKindRegistry.php)
allows plugins to define their own taint kinds and groups. The registry can be accessed via the `Psalm\Config`
singleton.


## Define Kinds

```php
$registry = \Psalm\Config::getInstance()->taint_kind_registry;
$registry->defineKinds([
'custom-first' => \Example\Package\TaintedCustomA::class,
'custom-second' => \Example\Package\TaintedCustomB::class,
], 'custom');
```

The example above defines the custom taints `custom-first` and `custom-second`,
maps them to corresponding individual issue classes and organizes them
in a new custom group `custom`.

```php
/**
* @psalm-taint-source custom-first
*/
function fetchFirst(): string {}
/**
* Stub for the `custom` group - which includes both defined kinds.
*
* @psalm-taint-source custom
*/
function fetchData(): array {}
```

## Define Groups

```php
$registry = \Psalm\Config::getInstance()->taint_kind_registry;
$registry->defineGroup('specific-input', 'html', 'sql');
```

The exmaple above defines a new group `specific-input`, which only holds
a reduced subset (`html` and `sql`) of the default built-in group `input`.

```php
/**
* @psalm-taint-source specific-input
*/
function fetchData(): array {}
```

## Extend Groups

```php
$registry = \Psalm\Config::getInstance()->taint_kind_registry;
$registry->extendGroup('input', 'custom-first');
```

The example above adds the custom kind `custom-first` to an existing group `input`.

## Define Group Proxies

```php
$registry = \Psalm\Config::getInstance()->taint_kind_registry;
$registry->defineGroupProxy('input-sql', 'input', [
'sql' => \Example\Package\TaintedSqlSecondOrder::class,
]);
```

The example above is special as it defines `input-sql` to be a proxy for
the existing group `input-sql` and keeps track of that usage. In addition,
the build-in kind `sql` is overloaded with a custom issue class.

This example is known as "Second Order SQL Injection", where data that was
retrieved from a database is reused for new SQL queries. The assumption is,
that the persisted data was not sanitized and might contain user submitted
malicious snippets.

```php
/**
* @psalm-taint-source input-sql
*/
function fetchAliasFromDatabase(): string {}

/**
* @psalm-taint-sink sql $query
* @psalm-taint-specialize
*/
function executeDatabaseQuery($query): array {}

// this would still issue the default `TaintedSql`
$alias = $_GET['alias'];
$query = sprintf('SELECT * FROM comments WHERE alias="%s";', $alias);
$rows = executeDatabaseQuery($query);

// this would issue the custom `TaintedSqlSecondOrder`
$alias = fetchAliasFromDatabase();
$query = sprintf('SELECT * FROM comments WHERE alias="%s";', $alias);
$rows = executeDatabaseQuery($query);
```
3 changes: 2 additions & 1 deletion docs/security_analysis/custom_taint_sources.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class BadSqlTainter implements AfterExpressionAnalysisInterface
$expr = $event->getExpr();
$statements_source = $event->getStatementsSource();
$codebase = $event->getCodebase();
$taint_type_registry = $codebase->config->taint_kind_registry;
if ($expr instanceof PhpParser\Node\Expr\Variable
&& $expr->name === 'bad_data'
) {
Expand All @@ -63,7 +64,7 @@ class BadSqlTainter implements AfterExpressionAnalysisInterface
$codebase->addTaintSource(
$expr_type,
$expr_identifier,
TaintKindGroup::ALL_INPUT,
[TaintKindGroup::GROUP_INPUT],
new CodeLocation($statements_source, $expr)
);
}
Expand Down
7 changes: 7 additions & 0 deletions docs/security_analysis/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,14 @@ Psalm recognises a number of taint types by default, defined in the [Psalm\Type\
- `user_secret` - used for strings that could contain user-supplied secrets
- `system_secret` - used for strings that could contain system secrets

Each of these default kind types will be mapped to a corresponding class which inherits from abstract class
`TaintedInput` - for instance `TaintedHtml`, `TaintedShell`, `TaintedHeader`, etc.

You're also free to define your own taint types when defining custom taint sources – they're just strings.
Per default those custom types are mapped to the generic class `TaintedCustom`.

However, plugins can utilize [custom taint kinds](custom_taint_kinds.md) to define more specific classes
for improved semantics and enhanced issue handling in general.

## Taint Sources

Expand Down
4 changes: 2 additions & 2 deletions src/Psalm/Codebase.php
Original file line number Diff line number Diff line change
Expand Up @@ -2420,10 +2420,10 @@
* @param array<string> $taints
* @psalm-suppress PossiblyUnusedMethod
*/
public function addTaintSource(

Check failure on line 2423 in src/Psalm/Codebase.php

View workflow job for this annotation

GitHub Actions / Check backward compatibility

Default parameter value for parameter $taints of Psalm\Codebase#addTaintSource() changed from array ( 0 => 'html', 1 => 'has_quotes', 2 => 'shell', 3 => 'sql', 4 => 'callable', 5 => 'eval', 6 => 'unserialize', 7 => 'include', 8 => 'ssrf', 9 => 'ldap', 10 => 'file', 11 => 'header', 12 => 'cookie', ) to array ( 0 => 'input', )
Union $expr_type,
string $taint_id,
array $taints = TaintKindGroup::ALL_INPUT,
array $taints = [TaintKindGroup::GROUP_INPUT],
?CodeLocation $code_location = null
): Union {
if (!$this->taint_flow_graph) {
Expand All @@ -2447,9 +2447,9 @@
* @param array<string> $taints
* @psalm-suppress PossiblyUnusedMethod
*/
public function addTaintSink(

Check failure on line 2450 in src/Psalm/Codebase.php

View workflow job for this annotation

GitHub Actions / Check backward compatibility

Default parameter value for parameter $taints of Psalm\Codebase#addTaintSink() changed from array ( 0 => 'html', 1 => 'has_quotes', 2 => 'shell', 3 => 'sql', 4 => 'callable', 5 => 'eval', 6 => 'unserialize', 7 => 'include', 8 => 'ssrf', 9 => 'ldap', 10 => 'file', 11 => 'header', 12 => 'cookie', ) to array ( 0 => 'input', )
string $taint_id,
array $taints = TaintKindGroup::ALL_INPUT,
array $taints = [TaintKindGroup::GROUP_INPUT],
?CodeLocation $code_location = null
): void {
if (!$this->taint_flow_graph) {
Expand Down
7 changes: 7 additions & 0 deletions src/Psalm/Config.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
use Psalm\Plugin\PluginInterface;
use Psalm\Progress\Progress;
use Psalm\Progress\VoidProgress;
use Psalm\Type\TaintKindRegistry;
use RuntimeException;
use SimpleXMLElement;
use Symfony\Component\Filesystem\Path;
Expand Down Expand Up @@ -727,6 +728,11 @@ class Config
/** @var list<string> */
public array $config_warnings = [];

/**
* @readonly
*/
public TaintKindRegistry $taint_kind_registry;

/** @internal */
protected function __construct()
{
Expand All @@ -735,6 +741,7 @@ protected function __construct()
$this->universal_object_crates = [
strtolower(stdClass::class),
];
$this->taint_kind_registry = new TaintKindRegistry();
}

/**
Expand Down
3 changes: 2 additions & 1 deletion src/Psalm/Config/IssueHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ public static function getAllIssueTypes(): array
&& $issue_name !== 'ParseError'
&& $issue_name !== 'PluginIssue'
&& $issue_name !== 'MixedIssue'
&& $issue_name !== 'MixedIssueTrait',
&& $issue_name !== 'MixedIssueTrait'
&& $issue_name !== 'TaintKindFactory',
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ private static function taintVariable(
$var_name,
null,
null,
TaintKindGroup::ALL_INPUT,
[TaintKindGroup::GROUP_INPUT],
);

$statements_analyzer->data_flow_graph->addSource($server_taint_source);
Expand Down
Loading
Loading