SonarCloud PHP scanner did not catch SQL injection vulnerability

SonarCloud
,
1
  • versions used: SonarCloud (latest and no ability to include add ons)

Error observed:

Sample code included below, but the PHP scan missed a SQL injection vulnerability in our code base. It is expected that the SQL Injection rules should have caught this

Steps to reproduce:

  1. With the following code:
    $joins = array('LEFT JOIN tableName as t' . $integerVal . ' ON(t' . $integerVal . '.key=keyValue AND t' . $integerVal . '.locale=\'' . $injectableVar . '\' AND anotherDBValue=FALSE)');
    Run the static analysis.
  2. Inspect the results.

Expected:

The vulnerability should have been flagged as a blocker issue with a suggested remedy to escape the injectable variable.
$joins = array('LEFT JOIN tableName as t' . $integerVal . ' ON(t' . $integerVal . '.key=keyValue AND t' . $integerVal . '.locale=' . AppKernel::db()->escape($injectableVar) . '\' AND anotherDBValue=FALSE)');

Actual:

The code is not flagged up as an issue of any kind.

2

Hello @dono-greeff,

Thanks for reporting this false-negative.

Do you confirm this (https://github.com/mautic/mautic/blob/staging/app/AppKernel.php) class is coming from the Mautic project?

Which API do you use to run your SQL query involving the $joins variable?

Thanks
Alex

3

Hello Alex,

Thank you for your reply.

No it is not from that project. We define it internally within our app. There is sensitive code in there so sharing that will be difficult.

But the db method returns a postgres object.

Many thanks

6

Hello,

This is going to be almost impossible to investigate this one without being able to reproduce first.
You need to find a way to share with us a reproducer, so we can scan it locally and understand why the expected issue is not raise.

At minimum, we would need:

  • the web framework involved: Symfony, Laravel, others? This will help us to understand if we “see” the user input (aka the source)
  • details about this “postgres object”
  • which API is used to query the DB: a link to the documentation would help to clarify (aka: the sink)
  • logs of the code scan

All of this can be shared privately because you have sensitive data on these files.

Thanks
Alex

1 Like
7

Update: we exchanged a couple of private messages with @dono-greeff. I have a better understanding of the problem which is linked to how the SAST engine is handling the call to AppKernel::db() that is an in-house class and not coming from [Mautic]( https://github.com/mautic/mautic/blob/staging/app/AppKernel.php ).
I will update this thread once we have some ideas about how to fix the limitation.

9

The problem has been understood and fixed. It is referenced internally by the ticket SONARSEC-919: “Type inference should track static fields”. It is not yet deployed on SonarCloud. That should happen in January (we have other pending improvements under validation).

1 Like