SQL Injection warning seems ... questionable?

There’s some code in a project which is taking column names from user-data and then validating them against the actual column names in the database in order to sanitize them, however, the following produces a SQL injection warning:

$myCol = $_GET['myCol'];

$STH = $DBH->prepare("SELECT COLUMN_NAME FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_SCHEMA='myDB' AND TABLE_NAME='myTbl'");
$allowedList = $STH->fetchAll(PDO::FETCH_COLUMN);

# not considered sanitized by itself, must still do sprintf
if ( ! in_array($myCol, $allowedList)) {
  throw new Exception("BAD");
}

$SQL = "SELECT $myCol FROM myTbl";
$STH = $DBH->prepare($SQL);
$STH->execute();

However, one small change allows it to pass:

$SQL = sprintf("SELECT %s FROM myTbl", $myCol);

My issue here is that the sprintf is doing nothing to increase security yet the checker requires it for a clean bill of health. Is there a better way to do this if the code MUST have a dynamic column? We prefer to not hard code a list of valid columns as that requires maintaining the database table and the list in the code base and those could get out of sync.

Hi Erik and welcome to the community!

I am not able to reproduce your problem with the code that you provided. I have prepared a repository here and the results are here. As you can see, only the vuln_ files raise an issue, as one would expect. In general we consider in_array a sanitizer for such type of issues and our analysis supports sprintf.

Is this code here your actual test code or is it just a snippet?

Thanks,
Hendrik

I ran the test on the saas version with that code above but encountered the issue on the enterprise version owned by another team where I work that we must check-in code against. The code above is a simple version of the code we have, but again we removed the critical error by using sprintf which probably shouldn’t be considered a sanitizing function. The sanitizer in the legacy code looks more like this:

if ( ! $this->columnValidator->isValid($col)) {
    throw new InvalidColumnException();
}

and by adding sprintf to the SQL, we were able to remove the “vulnerability”. This bothers me as sprintf as we’ve used it (shown in my first post) wouldn’t do anything. The isValid method does what you see in my original post – checks the value against the actual column names in the database.

Thanks for the info! So it sounds like your local version is outdated. The old version did not have support for sprintf yet, thus no issue is raised. It does not consider the sprintf a sanitizer, the analyzer simply does not understand what happens to the user input.

I would highly recommend to update to our latest version (for example LTS 8.9 was released last month). This will resolve the issue.

It is of course possible that the analyzer is not able to resolve the method isValid in a dynamic language like PHP for some reason. In that case you could manually define the method as a sanitizer. In SonarQube 8.9 this can be easily done through the web interface (in the Enterprise Edition).

Please let me know if there are any questions.