designsecurity / progpilot

A static analysis tool for security
MIT License
326 stars 59 forks source link

ProgPilot is unable to deal with ternary operator #52

Open Mister-Stein opened 1 year ago

Mister-Stein commented 1 year ago

Progpilot built from source, includes all recent commits and in default configuration. PHP 8.2.1

I see that ProgPilot don't know how to deal with ternary operator. It's syntax: (Conditional statement) ? (Statement_1) : (Statement_2); Simplified example code, no result after analysis:

<?php
global $wpdb;
$tainted = isset($_POST['order_by']) ? $_POST['order_by'] : 'post_date';
$wpdb->get_row($tainted);

And there's exists it's shorthand variant, so called "Elvis" operator. It's syntax: expression1 ?: expression2 Another example, with no results too:

<?php
global $wpdb;
$tainted = isset($_POST['order_by']) ?: 'post_date';
$wpdb->get_row($tainted);

The ternary operator is pretty common these days, I often see it in WordPress plugins. Although, it's shorthand variant I haven't seen in real code yet. Description about it in PHP Doc How do we go about it? Is it possible to make ProgPilot be able to deal with ternary operator?

eric-therond commented 1 year ago

yes it's a big limitation, progpilot should support phi operator generated by php-cfg. Unfortunately, it's complicated and I do not intend to solve this problem in the medium term.

Mister-Stein commented 1 year ago

Hello Eric, sorry for the delay, I was feeding my brain with that SSA, CFG, compilation stuff to inflate my knowledge in that field, perhaps, a little bit. As far as I get, that so-called phi function appears in places of SSA, where it is not clear to what "version" of a variable assign the value. Such situations happen in many places, like for and while loops, and in conditional blocks, such as if, else, and ternary operator is just a simple if, else. phi function appears after such blocks in SSA. And now we're facing the main problem here: how to resolve phi function? I've started to google... I've found an interesting project: Recki-CT. But it was archived, and initially started by @ircmaxell. "Recki-CT is a set of tools that implement a compiler for PHP", as said in the description. But what captured my attention was the documentation, especially the file, written by @ircmaxell about the Phi Resolving. Read this for sure. Of course, there's problems too, especially with mixed return type of the phi function. There's possible solutions for that case, but they impact performance. But hey, we don't care about performance here, it is makes sense for compiler, not for ProgPilot. We can afford some performance impact, if we can gain in return extended functionality, and resolve the problem.

I hope that I didn't confused. I just want to help as much as I can, and as much as my knowledge allow. Let's remain this issue and #53 issue open, I see that they have identical root cause, until we resolve it.

Jeremie-Kiwik commented 1 week ago

hello ! any news on it? For now, this is the main problem that prevents using this great tool on real life (as it silently ignore sinks after a ternary operation). For example:

$id = $_GET['id'];
$id = !empty($id) ? $id : 'NULL';
// will never be triggered -- $id seems now safe
$sql = 'SELECT * FROM table WHERE id = ' . $id;