I have what I deem unsafe code: $results = $object->method( $_GET['type'] );
In $object->method() this happens (where $type is the argument passed to method())
$sql = 'SELECT * FROM ' . $type;
This is then passed to PDO PHP class.
Of course the code is more elaborate than above example, but what happens is, $_GET is a user variable and should not be used to populate SQL Queries. SC should (and did in past) flag that. It does not in my latest test.
I tested this on a single-file project, and had the same results.
Code:
$results = get_items( $_GET['type'] );
function get_items($type){
$sql = 'SELECT * FROM ' . $type;
// do PDO SQL Query.
return $sql;
}
The only error returned by SC is a code smell about immediately returning $sql instead of assigning it to var first.
IMO, this is a classic security issue and should be flagged (and it did, about a week ago).
Interestingly thou, if I change to echo the $type, then it flags it.
But that shouldn’t matter whether a code returns or echoes. It is still unsafe to pass a user input to a function, specially if later used on a SQL Query (which is the case in my example)
it will only be reported as a SQL injection if it is actually passed to a function that executes the query. So for example, if I add PDO::query($results); at the end of your sample, an issue is reported for me.
There are multiple things that could be the cause why no issue is detected in your actual code:
The source of the user input is not defined as a source in the analyzer ($_GET is definitely a source though).
The sensitive function is not defined as a sink in the analyzer.
The analyzer can not follow the execution flow for some reason, for example, if reflection is involved it is not always clear what the execution flow will look like.
All three things are very specific to your code, so to be able to tell why no issue is detected I will need to see the code or a simplified version of the code that consists of the source and the sink.
I can also establish a private channel to share any samples if you would prefer that.
So that means $_GET actually is set by a jQuery AJAX call.
But that really shouldn’t matter.
As soon $_GET is passed to a function, which then passes its arg to a SQL Query (which in my code is using PDO, but again from another class, not directly in the same function) … it should flag it.
Since the repo is private (for now, it will be open at some point) I cannot share the code here unfortunately.
If you can establish a private channel that would be good, I can also invite you to the project, the code is no rocket science, it involves only perhaps 10 lines of code, but it is part of a much larger code file(s).
I believe this should be flagged, but for some reason is not (I only discovered it because I did run a PHPCS on the code, which was less sophisticated than SC and it just flags any unescaped $_GET without much “thinking”. Sometimes that is annoying, but in this case I think I am happy it did).
Let me know what you would need for me to share the code privately.
The issue was not detected due to reflection. The analyzer was not able to follow the source to the sink. Beda changed the code to not use reflection and now everything works as expected
