semgrep / semgrep-rules

Semgrep rules registry
https://semgrep.dev/registry
Other
817 stars 398 forks source link

False positive on php.lang.security.injection.tainted-sql-string.tainted-sql-string #3252

Closed Sjord closed 11 months ago

Sjord commented 11 months ago

foo.php:

<?php
$this->delete("id:".$_GET['id']);
$ semgrep -c 'r/php.lang.security.injection.tainted-sql-string.tainted-sql-string' foo.php
...
    foo.php
       php.lang.security.injection.tainted-sql-string.tainted-sql-string
          User data flows into this manually-constructed SQL string. User data can be safely inserted
          into SQL strings using prepared statements or an object-relational mapper (ORM). Manually-
          constructed SQL strings is a possible indicator of SQL injection, which could let an
          attacker steal or manipulate data from the database. Instead, use prepared statements
          (`$mysqli->prepare("INSERT INTO test(id, label) VALUES (?, ?)");`) or a safe library.
          Details: https://sg.run/lZYG

            2┆ $this->delete("id:".$_GET['id']);

It matches because the function name is delete, even if it is not part of the string.

inkz commented 11 months ago

Hi @Sjord ! thank you for reporting, what is the version of semgrep are you using? I can't reproduce on 1.54.3

Sjord commented 11 months ago

I can reproduce this on semgrep 1.54.1, and also in the playground which has semgrep 1.52.0.

inkz commented 11 months ago

Can you please add playground link? @Sjord

Sjord commented 11 months ago

https://semgrep.dev/playground/r/r6Uy4Yv/sjord.tainted-sql-string?editorMode=advanced

inkz commented 11 months ago

@Sjord ok! I was able to reproduce, taking it in work! Thank you! :pray: