Skip to content

Commit 2ba2584

Browse files
cedric-annecedric-anne
committed
Add a rule to forbid dynamic object-instantiation
1 parent df0445e commit 2ba2584

16 files changed

+496
-1
lines changed

‎.github/workflows/continuous-integration.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ jobs:
5959
- name: "PHP Parallel Lint"
6060
if: ${{ !cancelled() }}
6161
run: |
62-
vendor/bin/parallel-lint --exclude ./.git/ --exclude ./vendor/ --checkstyle . | cs2pr
62+
vendor/bin/parallel-lint --exclude ./.git/ --exclude ./tests/data/ --exclude ./vendor/ --checkstyle . | cs2pr
6363
- name: "PHP CS Fixer"
6464
if: ${{ !cancelled() }}
6565
run: |

‎README.md

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,41 @@ parameters:
1515
1616
## Rules
1717
18+
### `ForbidDynamicInstantiationRule`
19+
20+
> Since GLPI 11.0.
21+
22+
Instantiating an object from an unrestricted dynamic string is unsecure.
23+
Indeed, it can lead to unexpected code execution and has already been a source of security issues in GLPI.
24+
25+
Before instantiating an object, a check must be done to validate that the variable contains an expected class string.
26+
```php
27+
$class = $_GET['itemtype'];
28+
29+
$object = new $class(); // unsafe
30+
31+
if (is_a($class, CommonDBTM::class, true)) {
32+
$object = new $class(); // safe
33+
}
34+
```
35+
36+
If the `treatPhpDocTypesAsCertain` PHPStan parameter is not set to `false`, a variable with a specific `class-string`
37+
type will be considered safe.
38+
```php
39+
class MyClass
40+
{
41+
/**
42+
* @var class-string<\CommonDBTM> $class
43+
*/
44+
public function doSomething(string $class): void
45+
{
46+
$object = new $class(); // safe
47+
48+
// ...
49+
}
50+
}
51+
```
52+
1853
### `ForbidExitRule`
1954

2055
> Since GLPI 11.0.

‎rules.neon

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,13 @@ services:
1212
class: PHPStanGlpi\Services\GlpiVersionResolver
1313
arguments:
1414
version: %glpi.glpiVersion%
15+
-
16+
class: PHPStanGlpi\Rules\ForbidDynamicInstantiationRule
17+
arguments:
18+
treatPhpDocTypesAsCertain: %treatPhpDocTypesAsCertain%
1519

1620
rules:
21+
- PHPStanGlpi\Rules\ForbidDynamicInstantiationRule
1722
- PHPStanGlpi\Rules\ForbidExitRule
1823
- PHPStanGlpi\Rules\ForbidHttpResponseCodeRule
1924
- PHPStanGlpi\Rules\MissingGlobalVarTypeRule

‎src/Rules/ForbidDynamicInstantiationRule.php

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PHPStanGlpi\Rules;
6+
7+
use PhpParser\Node;
8+
use PhpParser\Node\Expr\New_;
9+
use PhpParser\Node\Name;
10+
use PHPStan\Analyser\Scope;
11+
use PHPStan\Rules\Rule;
12+
use PHPStan\Rules\RuleErrorBuilder;
13+
use PHPStan\Type\Type;
14+
use PHPStan\Type\UnionType;
15+
use PHPStanGlpi\Services\GlpiVersionResolver;
16+
17+
/**
18+
* @implements Rule<New_>
19+
*/
20+
final class ForbidDynamicInstantiationRule implements Rule
21+
{
22+
private GlpiVersionResolver $glpiVersionResolver;
23+
24+
private bool $treatPhpDocTypesAsCertain;
25+
26+
public function __construct(
27+
GlpiVersionResolver $glpiVersionResolver,
28+
bool $treatPhpDocTypesAsCertain
29+
) {
30+
$this->glpiVersionResolver = $glpiVersionResolver;
31+
$this->treatPhpDocTypesAsCertain = $treatPhpDocTypesAsCertain;
32+
}
33+
34+
public function getNodeType(): string
35+
{
36+
return New_::class;
37+
}
38+
39+
public function processNode(Node $node, Scope $scope): array
40+
{
41+
if (\version_compare($this->glpiVersionResolver->getGlpiVersion(), '11.0.0-dev', '<')) {
42+
// Only applies for GLPI >= 11.0.0
43+
return [];
44+
}
45+
46+
if ($this->isSafe($node, $scope)) {
47+
return [];
48+
}
49+
50+
return [
51+
RuleErrorBuilder::message(
52+
'Instantiating an object from an unrestricted dynamic string is forbidden (see https://github.com/glpi-project/phpstan-glpi?tab=readme-ov-file#forbiddynamicinstantiationrule).'
53+
)
54+
->identifier('glpi.forbidDynamicInstantiation')
55+
->build(),
56+
];
57+
}
58+
59+
private function isSafe(New_ $node, Scope $scope): bool
60+
{
61+
if ($node->class instanceof Name) {
62+
// Either a class identifier (e.g. `new User()`),
63+
// or a PHP keyword (e.g. `new self()` or `new static()`).
64+
return true;
65+
}
66+
67+
if ($node->class instanceof Node\Stmt\Class_) {
68+
// Anonymous class instantiation (e.g. `$var = new class () extends CommonDBTM {}`).
69+
return true;
70+
}
71+
72+
$type = $this->treatPhpDocTypesAsCertain ? $scope->getType($node->class) : $scope->getNativeType($node->class);
73+
74+
if ($this->isTypeSafe($type)) {
75+
return true;
76+
}
77+
78+
return false;
79+
}
80+
81+
private function isTypeSafe(Type $type): bool
82+
{
83+
if ($type instanceof UnionType) {
84+
// A union type variable is safe only if all of the possible types are safe.
85+
foreach ($type->getTypes() as $sub_type) {
86+
if (!$this->isTypeSafe($sub_type)) {
87+
return false;
88+
}
89+
}
90+
return true;
91+
}
92+
93+
if ($type->isObject()->yes()) {
94+
// Either a instanciation from another object instance (e.g. `$a = new Computer(); $b = new $a();`),
95+
// or from a variable with an object type assigned by the PHPDoc (e.g. `/* @var $class Computer */ $c = new $class();`).
96+
// Creating an instance from an already instantiated object is considered safe.
97+
return true;
98+
}
99+
100+
if ($type->isClassString()->yes()) {
101+
// A variable with a `class-string` type assigned by the PHPDoc.
102+
//
103+
// Unless the generic type is unspecified, we consider that the related code produces all the necessary
104+
// checks to ensure that the variable is safe before assigning this type.
105+
return count($type->getClassStringObjectType()->getObjectClassNames()) > 0;
106+
}
107+
108+
$constant_strings = $type->getConstantStrings();
109+
if (count($constant_strings) > 0) {
110+
// Instantiation from a string variable with constant(s) value(s).
111+
// If every values matches a known class (e.g. `$class = 'Computer'; $c = new $class();`),
112+
// this is considered safe as the class name has been intentionally hardcoded.
113+
foreach ($constant_strings as $constant_string) {
114+
if ($constant_string->isClassString()->yes() === false) {
115+
return false;
116+
}
117+
}
118+
}
119+
120+
if ($type->isNull()->yes()) {
121+
// Instantiation will a `null` hardcoded class name (e.g. `$a = $condition ? Computer::class : null; $b = new $a();`),
122+
// or from a variable with a nullable type assigned by the PHPDoc (e.g. `/* @var $class class-string<CommonDBTM>|null */ $c = new $class();`).
123+
// This is safe from this rule point of view as it will not permit to instantiate an unexpected object.
124+
//
125+
// An error will be triggered by base PHPStan rules with a most relevant message.
126+
return true;
127+
}
128+
129+
return false;
130+
}
131+
}

‎tests/IgnoredGlpiVersion/Rules/ForbidDynamicInstantiationRuleTest.php

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PHPStanGlpi\Tests\IgnoredGlpiVersion\Rules;
6+
7+
use PHPStan\Rules\Rule;
8+
use PHPStan\Testing\RuleTestCase;
9+
use PHPStanGlpi\Rules\ForbidDynamicInstantiationRule;
10+
use PHPStanGlpi\Services\GlpiVersionResolver;
11+
use PHPStanGlpi\Tests\IgnoredGlpiVersion\TestIgnoredRuleTrait;
12+
13+
/**
14+
* @extends RuleTestCase<ForbidDynamicInstantiationRule>
15+
*/
16+
class ForbidDynamicInstantiationRuleTest extends RuleTestCase
17+
{
18+
use TestIgnoredRuleTrait;
19+
20+
protected function getRule(): Rule
21+
{
22+
return new ForbidDynamicInstantiationRule(
23+
new GlpiVersionResolver('10.0.18'), // should be ignored in GLPI < 11.0.0
24+
false
25+
);
26+
}
27+
}

‎tests/Rules/ForbidDynamicInstantiationRulePhpDocAsCertainTest.php

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PHPStanGlpi\Tests\Rules;
6+
7+
use PHPStan\Rules\Rule;
8+
use PHPStan\Testing\RuleTestCase;
9+
use PHPStanGlpi\Services\GlpiVersionResolver;
10+
use PHPStanGlpi\Rules\ForbidDynamicInstantiationRule;
11+
12+
/**
13+
* @extends RuleTestCase<ForbidDynamicInstantiationRule>
14+
*/
15+
class ForbidDynamicInstantiationRulePhpDocAsCertainTest extends RuleTestCase
16+
{
17+
protected function getRule(): Rule
18+
{
19+
return new ForbidDynamicInstantiationRule(
20+
new GlpiVersionResolver('11.0.0'),
21+
true
22+
);
23+
}
24+
25+
public function testAnonymousClass(): void
26+
{
27+
$this->analyse([__DIR__ . '/../data/ForbidDynamicInstantiationRule/anonymous-class.php'], [
28+
]);
29+
}
30+
31+
public function testContantValues(): void
32+
{
33+
$this->analyse([__DIR__ . '/../data/ForbidDynamicInstantiationRule/constant-values.php'], [
34+
]);
35+
}
36+
37+
public function testInstanceof(): void
38+
{
39+
$this->analyse([__DIR__ . '/../data/ForbidDynamicInstantiationRule/instanceof.php'], [
40+
]);
41+
}
42+
43+
public function testIsA(): void
44+
{
45+
$this->analyse([__DIR__ . '/../data/ForbidDynamicInstantiationRule/is-a.php'], [
46+
]);
47+
}
48+
49+
public function testIsSubclassOf(): void
50+
{
51+
$this->analyse([__DIR__ . '/../data/ForbidDynamicInstantiationRule/is-subclass-of.php'], [
52+
]);
53+
}
54+
55+
public function testMixedType(): void
56+
{
57+
$this->analyse([__DIR__ . '/../data/ForbidDynamicInstantiationRule/mixed-type.php'], [
58+
[
59+
'Instantiating an object from an unrestricted dynamic string is forbidden (see https://github.com/glpi-project/phpstan-glpi?tab=readme-ov-file#forbiddynamicinstantiationrule).',
60+
11,
61+
],
62+
]);
63+
}
64+
65+
66+
public function testPhpDocClassString(): void
67+
{
68+
$this->analyse([__DIR__ . '/../data/ForbidDynamicInstantiationRule/phpdoc-class-string.php'], [
69+
[
70+
'Instantiating an object from an unrestricted dynamic string is forbidden (see https://github.com/glpi-project/phpstan-glpi?tab=readme-ov-file#forbiddynamicinstantiationrule).',
71+
7,
72+
],
73+
]);
74+
}
75+
76+
public function testPhpDocSpecificClass(): void
77+
{
78+
$this->analyse([__DIR__ . '/../data/ForbidDynamicInstantiationRule/phpdoc-specific-class.php'], [
79+
]);
80+
}
81+
82+
/**
83+
* @requires PHP >= 8.0
84+
*/
85+
public function testUnionType(): void
86+
{
87+
$this->analyse([__DIR__ . '/../data/ForbidDynamicInstantiationRule/union-type.php'], [
88+
[
89+
'Instantiating an object from an unrestricted dynamic string is forbidden (see https://github.com/glpi-project/phpstan-glpi?tab=readme-ov-file#forbiddynamicinstantiationrule).',
90+
19,
91+
],
92+
]);
93+
}
94+
}

0 commit comments

Comments
 (0)