Progpilot v1.0.2 don't recognize functions that are instances of an object

[Mister-Stein]

I think that Progpilot don't recognize functions that are instances of an object, in sinks and maybe in sanitizers, validators etc. I use phar Progpilot v1.0.2 in default configuration, PHP 8.2.1 In the following screenshot, I'm scanning dummy file, showing that nothing was found: I changed this file, so it will contain just an usual function as a sink, and everything works:

I checked various sinks in Progpilot's default configuration, but the picture is the same: just functions are working, but functions that are instances of an object are not recognized, but should. I remember that in previous versions of Progpilot I haven't seen such an issue, but after I installed v1.0.2, I'm immediately encountered it. Also I'm tested that Progpilot v1.0.0, recognizes everything just nice. In this screenshot, I used Progpilot v1.0.0:

[eric-therond]

Hello, in your code PHP itself doesn't know what is $maxdb variable (the type of the object) so it's normal progpilot cannot guess it too.

This vulnerability is detected for instance:

<?php
$maxdb = new maxdb("localhost", "MONA", "RED", "DEMODB");
$maxdb->query($_POST['form_id']);

[Mister-Stein]

Firstly, older versions of Progpilot were detecting this vulnerability even without the presence of instantiation of an object in analyzed source code. This is maybe due to the changed way of how Progpilot perform analysis in the latest version.

Secondly, that way of analysis is limiting the number of detected vulnerabilities, because instantiation of an object is not always appears in analyzed source code. That situation may happen, for example, in the source code of some plugin for some framework; Object may be initialized and saved in global variable in the core of the framework, and only then be used in the source code of the plugin for this framework, just by declaring this global variable like that for example: global $maxdb;

Here's the real example: In the code of the most of plugins for WordPress, very common the use of $wpdb object. The access to this object in the code of the plugin is obtained by declaring the global $wpdb; variable, because this object initialized in the core and made global. In such situation Progpilot won't recognize the object, and such situations may appear in other frameworks.

Maybe, I must somehow include the core code of some framework when analyzing the code of some plugin for that framework now, the addition of corresponding sources, sanitizers, sinks etc. is already not enough?

[eric-therond]

Yes you are correct, progpilot version 0.8.0 was able to find this vulnerability, if I remember correctly, it used the name of the variable as an indicator of the object type:

global $maxdb;
$maxdbf->query($_POST['form_id']); // detected

global $maxdb2;
$maxdb2->query($_POST['form_id']); // not detected

This can generate a lot of false positives so it preferable to not be based on such kind of approximation IMHO.

About real code examples, progpilot is designed to resolve, global / include / autoloading / uses, just as an example with include:

<?php
include("def.php");
$maxdb->query($_POST['form_id']); // detected

def.php:

<?php
$maxdb = new maxdb("localhost", "MONA", "RED", "DEMODB");

[Mister-Stein]

I get what you mean, and you have a point. I was already guessing that old way of analysis could generate false positives, so I'm support the new way of analysis.

But, now we have a problem. If to recall the example of Wordpress, and if I want to audit the codebase of the plugin, I have to include in the analysis the whole WordPress core to resolve functions, classes, interfaces, global variables etc., because the codebase of analyzed plugin doesn't contain definitions/declarations of all of this. So, does it means that now Progpilot needs not only configured sources, sanitizers, sinks etc., but also "stubs", included in the analysis, as in other static code analysis tools to properly perform analysis? I will try to check that.

[eric-therond]

I never developed a WordPress plugin but as far I know, I see two possibilities:

1. As you guess, you have to audit the plugin and WordPress core, personally it doesn't shock me, because when you develop a WordPress plugin you extend the WordPress core. In this case, Wordpress core is not an immutable dependency like the ones managed with composer, I suppose when you develop a plugin you can even be tempted to modify the Wordpress core codebase at least to debug/test your plugin. So it makes sense to analyze both.
2. If you just want to audit your plugin, by analyzing only the folder of your plugin, you need to extend progpilot analysis, with sources/sinks/validators/sanitizers specific to WordPress. It's something that can be easy if you know well WordPress (eg: sinks on $wpdb object ect).

[Mister-Stein]

Sorry for the delay. Why Progpilot doesn't detect anything in this code:

<?php
$maxdb = new maxdb("localhost", "MONA", "RED", "DEMODB");
function test() {
    global $maxdb;
    $maxdb->query($_POST['form_id']);
}

With include it is working, as in your previous example, but why global is not resolved? Or I'm doing something wrong?

[eric-therond]

good catch! I fixed that on master but you will find a lot of "true negatives" like that. Progpilot has a low priority on my side, I am not sure I will continue to fix every TN, but I am still interested by feedback: don't hesitate to open other issues and maybe other maintainers will be interested to continue this project.

In your code you still need to call the test() function to have an execution path that leads to a vulnerability and then progpilot will detect it:

<?php
$maxdb = new maxdb("localhost", "MONA", "RED", "DEMODB");
function test() {
    global $maxdb;
    $maxdb->query($_POST['form_id']);
}
test();

[Mister-Stein]

Nice! That's working now. But here's another one code snippet:

<?php
function func1() {
    global $maxdb;
    $maxdb = new maxdb("localhost", "MONA", "RED", "DEMODB");
}

function func2() {
    global $maxdb;
    $form = $maxdb->query($_POST['form_id']);
}
func1();
func2();

I think that Progpilot should detect it, am I wrong? Sorry, I'm starting to annoy you :stuck_out_tongue_winking_eye: Yeah, I feel you. You're the only one who developing Progpilot. That will be good if other enthusiasts get attracted to it. On my side, I will dig deeper in the source code to fully understand how Progpilot works, because I see in-depth potential in it. Progpilot deserves more attention. You here for years, still maintaining it, not throwing away. It makes me admire you. :wink:

[eric-therond]

This code sample is "not readable": when func1() is called, maxdb is referenced as global but there is no maxdb variable in the global scope. It works in PHP because "of the magic" of PHP which implicitly declare maxdb in the global scope.

It's not a realistic PHP code and the use of global variables in any languages is discouraged:

• https://www.baeldung.com/cs/global-variables
• https://www.geeksforgeeks.org/can-global-variables-be-dangerous/

Progpilot, like any code analysis tool is able to find vulnerabilities in code that "is easy" to read / understand, which should be the priority for any developers.

If someone develops like that: using global mutable variables, not declared in the global scope, and playing with implicit declarations made by PHP then security / progpilot is definitely not a thing, because at the end, nobody, humans and tools included, will be able to understand this kind of code.

Progpilot has limitations as I said, there is a lot to do, so the priorities to improve progpilot should be based on "real code", not code designed to "defeat progpilot" or code that is not understandable even by an human.

[Mister-Stein]

I get what you telling me, but sorry man, there's tenets that I don't agree. Let me clear: The code I provided above is valid, and I just simplified situations that appears in real code to that simple form. The solid proof of that is WordPress. In a lot of PHP code for WordPress, you’ll be inside of a function. This is often because you’re working with a WordPress hook. To make a variable available everywhere (globally) from inside a function means that you must first declare that variable as global. Then you can assign that variable to anything you like. Read this article, please, it explains what I try to say: https://wpshout.com/php-globals-variable-scope-wordpress/

Of course there's debate about PHP global variables, and their use is generally discouraged, and there's ways to avoid them. BUT, the most of the developers write PHP code using frameworks or CMS, not their own. And in some of them, global variables may be widely used, such as WordPress, that powers 42% of the web. And most of the PHP code in frameworks and CMS is object-oriented, it means that most of declarations, calls, etc. happens inside the functions.

And you know, I think that it is not right to throw away at least 42% of all PHP web applications, only because you don't like global variables. I see Progpilot as a tool, and the tool should be designed to serve it's purpose, in this case: detect vulnerabilities in the source code of an application using static code analysis methods, even if analyzed code contain "bad practices". If such practices are widely used, we should consider them, because we developing the tool, not coding standards. Progpilot should support frameworks, not only pure-PHP code.

I am struggling to make Progpilot to detect vulnerabilities in WordPress plugins, because WordPress plugins are such a big security hole in the Internet that should be patched. Older versions of Progpilot were able to detect some known vulnerabilities after configuration of sinks, sanitizers, etc., but was generating many false-positives. Now I can't even detect the most obvious ones. I think it is still related with the new way of analysis, specifically, as I guess, how Progpilot now resolves things like functions, globals, classes, etc.

So, as I can see now, I'm starting to think that the reason of the issue I've opened is a lot deeper than I thought...

[eric-therond]

I am struggling to make Progpilot to detect vulnerabilities in WordPress plugins

Can you share vulnerabilities you are trying to detect? the reproducer / vulnerable wordpress plugins?

You can either create a new issue with the link to a real wordpress plugin vulnerability, we'll see exactly what is missing the most in progpilot, or you can even propose a PR to add some test cases based on real world example.

[Mister-Stein]

Ok, I'll make an issue after some checks then, so I could properly and thoroughly describe and explain what I've tried. See you soon and thanks for the interest. :handshake: