This package contains a set of additional rules for PHPStan - PHP Static Analysis Tool.
We use these rules at Oro, Inc. and ask anyone contributing code to Oro Products to follow them as well.
Using DQL does not protect against injection vulnerabilities. The following APIs are designed to be SAFE from SQL injections:
Consider ALL other APIs to be not safe for user-input:
Expressions constructed with the help of various Expression Builders
See full article at Doctrine Security
As checking the whole codebase requires a lot of time, sql-injection search tool was added to simplify the process. The tool is based on PHPStan - PHP Static Analysis Tool and is implemented as additional Rule.
To check codebase for unsafe DQL and SQL usages perform the following actions:
<application_path>/tool/
where <application_path>
is path to application in the file systemcomposer install
./bin/phpstan analyze -c phpstan.neon <path_to_code> --autoload-file=<path_to_autoload.php>
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:
cd my_application/tool/
COMPOSER=composer-sql.json composer install
rm -rf logs;
mkdir logs;
ls ../package/ \
| grep -v "\-demo" | grep -v "demo-" | grep -v "test-" | grep -v "german-" | grep -v "view-switcher" | grep -v "twig-inspector" \
| parallel -j 4 "./bin/phpstan analyze -c phpstan.neon `pwd`/../package/{} --autoload-file=`pwd`/../application/commerce-crm-ee/vendor/autoload.php > logs/{}.log"
Note that commerce-crm-ee application should have autoload.php
generated.
The results of the analysis should be available within a minute. Each result should be checked carefully. Unsafe variables should be sanitized or escaped as a precaution.
Unsafe variables are any methods that may depend on external data. Even cached data may be unsafe if an attacker manages to access the cache storage. Any variable that comes from the outside, or contains a value returned by an unsafe function, is also unsafe and should be passed into queries with caution.
You can make a variable safe in a number of ways:
ORM based queries may contain vulnerable inputs. To keep them clean, follow the next rules:
\DateTime
and booleans.If there is a need to pass a variable directly into the query, use QueryBuilderUtil
safe methods
select($alias . '.' . $field)
, select('alias.', $field)
, select(sprintf('%s.%s', $alias, $field)
, etc.select('IDENTITY(%s.%s) as %s', $alias, $fieldName, $fieldAlias)
getSortOrder - return ASC or DESC if one of these values is passed. Otherwise, an exception is thrown. Used to clear sort directions passed as parameters.
NOTE!!! ->select(sprintf(%s as something', $fullName)) may be not quick fixed as $fullName may contain CONCAT(firstName, lastName) or any other statement. Such calls should be checked and marked safe
Use bind parameters or quote them with the connection quote method. Identifiers should be either checked for safety with QueryBuilderUtil or quoted with the quoteIdentifier method of connection.
Unsafe field is used as a part of query
$queryBuilder->andWhere($queryBuilder->expr()->eq($field, ':parameter'));
Fix - use QueryBuilderUtil::checkField
to check field for safeness
QueryBuilderUtil::checkField($field);
$queryBuilder->andWhere($queryBuilder->expr()->eq($field, ':parameter'));
Using composite identifier
$queryBuilder->andWhere($queryBuilder->expr()->eq($alias . '.' . $field, ':parameter'));
Or
$queryBuilder->andWhere($queryBuilder->expr()->eq(sprintf('%s.%s', $alias, $field), ':parameter'));
Possible ways to fix.
Fix 1 - use QueryBuilderUtil::getField
$queryBuilder->andWhere($queryBuilder->expr()->eq(QueryBuilderUtil::getField($alias, $field), ':parameter'));
Fix 2 - check each identifier separately with QueryBuilderUtil::checkIdentifier
QueryBuilderUtil::checkIdentifier($alias);
QueryBuilderUtil::checkIdentifier($field);
$queryBuilder->andWhere($queryBuilder->expr()->eq($alias . '.' . $field, ':parameter'));
Fix 3 - use safe QueryBuilderUtil::sprintf
, also applicable when replacing sprintf
$queryBuilder->andWhere($queryBuilder->expr()->eq(QueryBuilderUtil::sprintf('%s.%s', $alias, $field), ':parameter'));
Using composite parameter name
$queryBuilder->andWhere($queryBuilder->expr()->eq('table.id', $paramer));
Fix - use QueryBuilderUtil::checkParameter
QueryBuilderUtil::checkParameter($paramer);
$queryBuilder->andWhere($queryBuilder->expr()->eq('table.id', $paramer));
Using sort order passed from outside
$queryBuilder->orderBy('table.field', $sortOrder);
Fix - use QueryBuilderUtil::getSortOrder
$queryBuilder->orderBy('table.field', QueryBuilderUtil::getSortOrder($sortOrder));
Literal is passed to query
$queryBuilder->select(sprintf("'%s' as className", $className));
Fix use literal
expression
$queryBuilder->select(
sprintf((string)$queryBuilder->expr()->literal($className) . ' as className')
);
If a variable, a property or a method are considered safe after a detailed manual analysis, they may be added to trusted_data.neon
.
Such items will be marked as safe during further checks and skipped.
Available trusted_data.neon
configuration sections are:
variables
- whitelist of safe variables. Format class.method.variable: true
properties
- whitelist of safe properties. Format class.method.property: true
safe_methods
- whitelist of safe class methods. Format class.method: true
safe_static_methods
- whitelist of safe class static methods. Format class.method: true
check_methods_safety
- consider method safe if passed variables are safe. Format class.method: true
when all passed variables should be checked. Includes __construct
method for new instance creation checksclass.method: [1]
when only certain variables require checks (their positions are listed in array)check_static_methods_safety
- consider static method safe if passed variables are safe. 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)clear_methods
- variable is considered as safe if it is passed as argument into listed method. Format class.method: true
clear_static_methods
- variable is considered as safe if it is passed as argument into listed static method. Format class.method: true
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
,
we want to enable only the first and third argument checks, and for method2
we want all arguments to be checked:
check_methods:
SomeClass:
__all__: true
method1: [0, 2]
mrthod2: true
It is recommended to mark methods as safe. If a variable consists of several parts, it is better to add a minimal unsafe part to the whitelist, rather than the whole expression.
protected function addWhereToQueryBuilder(QueryBuilder $qb, string $suffix, int $index)
{
$rootAlias = $qb->getRootAlias();
$fieldName = $rootAlias . '.field' . $idx . $suffix;
$qb->andWhere($qb->expr()->gt($fieldName, 10);
}
Such code will lead to a security warning, as $fieldName
variable was constructed using several parts, some of which are not safe.
The best solution to make this expression safe is to check $suffix
with QueryBuilderUtil::checkIdentifier($suffix)
Another option is to add $suffix
into the trusted_data.neon
whitelist if its values are always passed as safe or checked in the caller.
The worst solution would be to mark $fieldName
as safe because its parts may be changed and, after adding a new or an unsafe part, it will be skipped, although it may contain an unchecked vulnerability.
Please referer to Oro Community Guide for information on how to contribute to this package and other Oro products.