PHP S3649 False-Negative

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.

1 Like

Hello Hedrik
Thanks for your reply
Here is Request::getString implementation

    public static 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];
    }

Nothing special there, as you may see.
We are running 8.6 now. We can upgrade it and check the same script on 8.8 version

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.