SonarQube 8.6.0.39681, PHP
I’ve created simle script for SQL-injection and writing raw user data into file and SonarQube didn’t find any problem about it.
The script looks like
<?php
use Framework\Helper\Request;
$search_text = Request::getString('search_text', $_REQUEST);
$test_obj = new \Common\Model\Dev6265();
$res = $test_obj->checkSql($search_text);
$test_obj->checkFileWrite($search_text);
And the class it uses
<?php
namespace Common\Model;
use Framework\Helper\Php;
class Dev6265
{
public function checkSql($user_text) {
$q = "SELECT * FROM b2bcenter.test_table WHERE text LIKE '$user_text%'";
$db = \App::i()->getDbConnection();
$res = $db->query($q);
return $res->fetchAll();
}
public function checkFileWrite($user_text) {
$file = 'out.txt';
$file_pointer = Php::fopen($file, 'a');
Php::fwrite($file_pointer, $user_text);
Php::fclose();
}
}
This code uses some Framework classes that are also included in scan scope.
We were using RIPS before the project was closed and it could find such issues.
Hello Andrey and welcome to the community! May I ask what edition of SonarQube you are using? Also, could you share the source code of Request::getString? Thanks!
Please note that we also released SonarQube 8.8. recently which - among other things - contains a lot of improvements for the taint-analysis engine.
Thanks! The edition is quite important because the taint-analysis engine (called security engine) is not available in the Community Edition. It is available starting from the Developer Edition. Are you using the Developer Edition (or above)?
I have constructed this test code (I had to guess about some stuff) and tested it with 8.6:
<?php
class App {
private static $instance = null;
public static function i()
{
if (self::$instance == null) {
self::$instance = new App();
}
return self::$instance;
}
public static function getDbConnection()
{
return new PDO();
}
};
function getString($variable_name, array $source_array, $default_value = false)
{
if (
!isset($source_array[$variable_name])
|| !is_scalar($source_array[$variable_name])
) {
return $default_value;
}
return (string)$source_array[$variable_name];
}
function checkSql($user_text) {
$q = "SELECT * FROM b2bcenter.test_table WHERE text LIKE '$user_text%'";
$db = App::i()->getDbConnection();
$res = $db->query($q);
return $res->fetchAll();
}
$search_text = getString('search_text', $_REQUEST);
checkSql($search_text);
Here, the SQL injection is found for me. In regards to the checkFileWrite, fwrite is currently not considered a sink and thus it will not raise an issue.
8.6 Developer Edition.
I was doing the test that is mostly relevant to the code we are actually writing. Specially I wanted to find out if the scripts that use external classes (in seperate files) are well analysed.
I can try to make the test with a bit more obvious code, but some parts like getDbConnection won’t be relevant to our actual code
It is no problem if the other classes are in separate files. Is there any chance that you could also share the other involved classes with me privately, e.g. App? Then I can create a test case that matches your code more accurately.
Hello, Hendrik_Buchwald
I’ve researched for a while, why does your scan work well and my not and the problem is in inheritance of the used classes.
I’ve created sample project where one can see the problem:
There are three versions of the same code there. One uses local class, second vendor class, and third (dir ‘ChildUsed’) uses local class that inherits vendor one. If you scan this project, you will see that SonarScan finds issues in the for the first two cases, but doesn’t for the third one.
It’s actually a big deal for our project, beacause we use many classes that have ancestors in our project
by default the vendor folder is ignored, thus nothing is found. You can change this with the following setting:
sonar.php.exclusions=
The SCM Sensor also ignores files that are specified in .gitignore. Since the vendor folder is one of those you also have to set the following setting:
Hi Hendrik,
I use sonar.scm.exclusions.disabled=true and SonarQube finds issues when using vendor classes. The problem appeares when I use inheritance in the classes I use.
If method getString is implemeted in the used class the problem is found, if this method is implemented in the anchestor of the used class, the problem is not found.
Thanks for the very thorough investigation and the nice reproducer!
With deactivated exclusions of the vendor folder, I could also reproduce it.
The problem is a limitation with static calls and inheritance in our PHP taint analysis. We are aware of it and already have a (non-public) ticket for it. A minimal reproducer is the following:
class A {
public static function foo($x) {
echo $x;
}
}
class B extends A {}
A::foo($_GET[1]); // raises an XSS issue
B::foo($_GET[1]); // it should raise an issue, but it doesn't
The problem only exists with static calls. You could verify it by, experimentally, changing the call in your reproducers ChildUsed\Model::checkSql() method to not use a static call. It will raise the expected issue in that case.
class Model
{
public function checkSql() {
$request = new Request();
$search_text = $request->getString('search_text', $_REQUEST);
// .....
Unfortunately, I cannot give you an estimate yet on when the limitation will be fixed. The ticket is in our backlog, but we did not plan for it yet.
As soon that it gets fixed, we will inform you in this thread.
Thanks for your answer, I’ve reproduced your case. Dynamic inheritance really works well. I’ve also tried to scan using Singleton construction
public static function i()
{
if (self::$instance === null) {
self::$instance = new static();
}
return self::$instance;
}
and works well when defining DbConnection (maybe because of the method name), but doesn’t work for user data handler classes. Maybe you could also consider this case.
