BAP-21804: Set the minimum required version of PHP to 8.2 for the 5.1…

… LTS - improved trust method arguments - improved build in functions trust
oroinc · Jan 24, 2023 · aeabf0d · aeabf0d
1 parent 01441c5
commit aeabf0d
Show file tree

Hide file tree

Showing 4 changed files with 88 additions and 26 deletions.
diff --git a/README.md b/README.md
@@ -33,6 +33,7 @@ To check codebase for unsafe DQL and SQL usages perform the following actions:
  - change directory to `<application_path>/tool/` where `<application_path>` is path to application in the file system
  - install dependencies `composer install`
  - run check with `./bin/phpstan analyze -c phpstan.neon <path_to_code> --autoload-file=<path_to_autoload.php>`
+   - to run checks in debug mode add `--debug` and `--xdebug` options
 
 To speedup analysis it's recommended to run it in parallel on per package basis. This may be achieved with the help of the `parallel` command:
 ```
@@ -173,13 +174,13 @@ Available `trusted_data.neon` configuration sections are:
  - `check_methods` - contains a list of methods that are checked for safeness. If passed arguments are unsafe, a security warning about such usage is reported by the analysis tool.
    Format `class.method: true` when all passed variables should be checked or `class.method: [1]` when only certain variables require checks (their positions are listed in array).
    Use `class.__all__: true` to check all class methods.
-   For example, there is SomeClass and we want to check all its methods, except for  `method1`. For `method1`, 
+   For example, there is SomeClass, and we want to check all its methods, except for  `method1`. For `method1`, 
    we want to enable only the first and third argument checks, and for `method2` we want all arguments to be checked:
     ```yml
     check_methods:
         SomeClass:
             __all__: true
-            method1: [0, 2]
+            method1: [0, 2, "3.array_keys_only", "4.array_values_only"]
             mrthod2: true
     ```
 

diff --git a/extension.neon b/extension.neon
@@ -83,7 +83,7 @@ parametersSchema:
             safe_methods: arrayOf(arrayOf(anyOf(bool(), arrayOf(int()))))
             check_static_methods_safety: arrayOf(arrayOf(anyOf(bool(), arrayOf(int()))))
             check_methods_safety: arrayOf(arrayOf(anyOf(bool(), arrayOf(int()))))
-            check_methods: arrayOf(arrayOf(anyOf(bool(), arrayOf(int()))))
+            check_methods: arrayOf(arrayOf(anyOf(bool(), arrayOf(anyOf(int(), string())))))
             clear_static_methods: arrayOf(arrayOf(bool()))
             clear_methods: arrayOf(arrayOf(bool()))
         ])

diff --git a/rules.neon b/rules.neon
@@ -175,9 +175,9 @@ parameters:
                     fetchArray: [0]
                     fetchColumn: [0]
                     fetchAll: [0]
-                    delete: [0, 1]
-                    update: [0, 1, 2]
-                    insert: [0, 1]
+                    delete: [0, "1.array_keys_only"]
+                    update: [0, "1.array_keys_only", "2.array_keys_only"]
+                    insert: [0, "1.array_keys_only"]
                     prepare: true
                     executeQuery: [0]
                     executeCacheQuery: [0]

diff --git a/src/Oro/Rules/Methods/QueryBuilderInjectionRule.php b/src/Oro/Rules/Methods/QueryBuilderInjectionRule.php
@@ -19,7 +19,7 @@
  */
 class QueryBuilderInjectionRule implements \PHPStan\Rules\Rule
 {
-    const SAFE_FUNCTIONS = [
+    const CHECK_FUNCTION_ARGS = [
         'sprintf' => true,
         'implode' => true,
         'join' => true,
@@ -31,6 +31,14 @@ class QueryBuilderInjectionRule implements \PHPStan\Rules\Rule
         'strtoupper' => true
     ];
 
+    const TRUST_FUNCTIONS = [
+        'count' => true,
+        'intval' => true,
+        'round' => true,
+        'mt_rand' => true,
+        'rand' => true
+    ];
+
     const VAR = 'variables';
     const PROPERTIES = 'properties';
 
@@ -44,6 +52,8 @@ class QueryBuilderInjectionRule implements \PHPStan\Rules\Rule
 
     const CHECK_METHODS = 'check_methods';
     const ALL_METHODS = '__all__';
+    const ARRAY_VALUES_ONLY = 'array_values_only';
+    const ARRAY_KEYS_ONLY = 'array_keys_only';
 
     /**
      * @var \PHPStan\Rules\RuleLevelHelper
@@ -105,7 +115,7 @@ public function __construct(
      */
     public function getNodeType(): string
     {
-        return Node\Expr::class;
+        return \PhpParser\NodeAbstract::class;
     }
 
     /**
@@ -121,11 +131,30 @@ public function processNode(Node $node, Scope $scope): array
             return $this->processMethodCalls($node, $scope);
         } elseif ($node instanceof Node\Expr\StaticCall) {
             $this->processStaticMethodCall($node, $scope);
+        } elseif ($node instanceof Node\Stmt\ClassMethod) {
+            $this->trustMethodArguments($node, $scope);
         }
 
         return [];
     }
 
+    private function trustMethodArguments(Node\Stmt\ClassMethod $node, Scope $scope): void
+    {
+        foreach ($node->getParams() as $param) {
+            $type = $param->type;
+            if ($type instanceof Node\Name) {
+                $className = (string)$type;
+                if ($className === 'DateTime' || $className === 'DateTimeZone') {
+                    $this->trustParam($param->var, $scope, (string)$node->name);
+                }
+            } elseif ($type instanceof Node\Identifier) {
+                if (in_array($type->name, ['int', 'float', 'bool'])) {
+                    $this->trustParam($param->var, $scope, (string)$node->name);
+                }
+            }
+        }
+    }
+
     /**
      * Check that all arguments of function are safe.
      *
@@ -136,15 +165,19 @@ public function processNode(Node $node, Scope $scope): array
     private function isUnsafeFunctionCall(Node\Expr $value, Scope $scope): bool
     {
         if ($value instanceof Node\Expr\FuncCall) {
-            if ($value->name instanceof Node\Name
-                && !empty(self::SAFE_FUNCTIONS[\strtolower($value->name->toString())])) {
-                foreach ($value->args as $arg) {
-                    if ($this->isUnsafe($arg->value, $scope)) {
-                        return true;
+            if ($value->name instanceof Node\Name) {
+                if (!empty(self::TRUST_FUNCTIONS[\strtolower($value->name->toString())])) {
+                    return false;
+                }
+                if (!empty(self::CHECK_FUNCTION_ARGS[\strtolower($value->name->toString())])) {
+                    foreach ($value->args as $arg) {
+                        if ($this->isUnsafe($arg->value, $scope)) {
+                            return true;
+                        }
                     }
+                } else {
+                    return true;
                 }
-            } else {
-                return true;
             }
         }
 
@@ -314,8 +347,13 @@ private function checkMethodArguments(
     ) {
         if (isset($config[$className])) {
             $methodName = $isConstructor ? '__construct': (string)$value->name;
-            $checkArg = function ($pos, array &$errors = []) use ($className, $value, $scope, $methodName) {
-                if ($this->isUnsafe($value->args[$pos]->value, $scope)) {
+            $checkArg = function ($pos, array &$errors = [], ?string $checkType = null) use (
+                $className,
+                $value,
+                $scope,
+                $methodName
+            ) {
+                if ($this->isUnsafe($value->args[$pos]->value, $scope, $checkType)) {
                     $errors[] = \sprintf(
                         'Unsafe calling method %s::%s. ' . PHP_EOL .
                         'Argument %d contains unsafe values %s. ' . PHP_EOL .
@@ -336,8 +374,12 @@ private function checkMethodArguments(
             // If method is listed in check methods and only certain arguments should be checked - check them
             if (isset($config[$className][$lowerMethodName]) && \is_array($config[$className][$lowerMethodName])) {
                 foreach ($config[$className][$lowerMethodName] as $argNum) {
+                    $checkType = null;
+                    if (str_contains((string)$argNum, '.')) {
+                        [$argNum, $checkType] = explode('.', $argNum);
+                    }
                     if (isset($value->args[$argNum])) {
-                        $checkArg($argNum, $errors);
+                        $checkArg($argNum, $errors, $checkType);
                     }
                 }
             } elseif ((isset($config[$className][$lowerMethodName]) && $config[$className][$lowerMethodName] === true)
@@ -389,11 +431,15 @@ private function isUnsafeNew(Node\Expr $value, Scope $scope)
             return false;
         }
 
+        if (!$value->class instanceof Node\Name) {
+            return false;
+        }
+
         $errors = [];
         return $this->checkMethodArguments(
             $value,
             $scope,
-            $value->class->toString(),
+            (string)$value->class,
             $this->trustedData[self::CHECK_METHODS_SAFETY],
             $errors,
             true
@@ -524,25 +570,30 @@ private function isUnsafeArrayDimFetch(Node\Expr $value, Scope $scope): bool
      *
      * @param Node\Expr $value
      * @param Scope $scope
+     * @param ?string $checkType
      * @return bool
      */
-    private function isUnsafeArray(Node\Expr $value, Scope $scope): bool
+    private function isUnsafeArray(Node\Expr $value, Scope $scope, ?string $checkType = null): bool
     {
         if ($value instanceof Node\Expr\Array_) {
             foreach ($value->items as $arrayItem) {
-                if (($arrayItem->key && $this->isUnsafe($arrayItem->key, $scope))
-                    || $this->isUnsafe($arrayItem->value, $scope)
+                if ($checkType !== self::ARRAY_VALUES_ONLY && $arrayItem->key
+                    && $this->isUnsafe($arrayItem->key, $scope)
                 ) {
                     return true;
                 }
+
+                if ($checkType !== self::ARRAY_KEYS_ONLY && $this->isUnsafe($arrayItem->value, $scope)) {
+                    return true;
+                }
             }
         }
 
         return false;
     }
 
     /**
-     * Consider variables casted to boolean or numeric as safe.
+     * Consider variables cast to boolean or numeric as safe.
      *
      * @param Node\Expr $value
      * @param Scope $scope
@@ -587,7 +638,7 @@ private function isUnsafeTernary(Node\Expr $value, Scope $scope): bool
      * @param Scope $scope
      * @return bool
      */
-    private function isUnsafe(Node\Expr $value, Scope $scope): bool
+    private function isUnsafe(Node\Expr $value, Scope $scope, ?string $checkType = null): bool
     {
         return $this->isUncheckedType($value)
             || $this->isUnsafeNew($value, $scope)
@@ -600,7 +651,7 @@ private function isUnsafe(Node\Expr $value, Scope $scope): bool
             || $this->isUnsafeConcat($value, $scope)
             || $this->isUnsafeEncapsedString($value, $scope)
             || $this->isUnsafeCast($value, $scope)
-            || $this->isUnsafeArray($value, $scope)
+            || $this->isUnsafeArray($value, $scope, $checkType)
             || $this->isUnsafeTernary($value, $scope);
     }
 
@@ -758,7 +809,17 @@ protected function loadTrustedData(array $loadedData)
      */
     private function trustVariable(Node\Expr\Variable $var, Scope $scope)
     {
-        $functionName = \strtolower((string)$scope->getFunctionName());
+        $this->trustParam($var, $scope, (string)$scope->getFunctionName());
+    }
+
+    /**
+     * @param Node\Expr\Variable $var
+     * @param Scope $scope
+     * @param string $functionName
+     */
+    private function trustParam(Node\Expr\Variable $var, Scope $scope, string $functionName)
+    {
+        $functionName = \strtolower($functionName);
         $varName = \strtolower((string)$var->name);
         $this->localTrustedVars[$scope->getFile()][$functionName][$varName] = true;
     }