Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Vulnerability detected CWE ID 88 in version 4.2.0 #81

Closed
TatianaGarcia94 opened this issue Mar 24, 2021 · 4 comments
Closed

Vulnerability detected CWE ID 88 in version 4.2.0 #81

TatianaGarcia94 opened this issue Mar 24, 2021 · 4 comments

Comments

@TatianaGarcia94
Copy link

En la ruta: /src/Client.php 829
Nombre de la vulnerabilidad: Improper Neutralization of Argument Delimiters in a Command ('Argument Injection')
Modo de detección: Se realizó un escaneo estático, el cuál detecto la línea mencionada como vulnerabilidad según los estándares de seguridad

@gggeek
Copy link
Owner

gggeek commented Nov 24, 2022

I am not sure there is a vulnerability here: the line in question is

$curl = curl_init($method . '://' . $server . ':' . $port . $this->path);

  1. There is no command being run here, but a call being made to a php api: curl_init
  2. I do not remember ever having seen anyone using urlencode or other technique to sanitize urls before passing them to curl_init in php code - is that considered a good practice?
  3. Even if it is possible to subvert the expected "normal" functioning of the library by, fe. using a value for $method which looks like f.e. 'ftp://hackme.com/please?etc=', I am not sure if that would qualify as a vulnerability, as the the library is not designed to have end users or other untrusted 3rd parties provide parameters such as the target server name and protocol. If anyone is building a software which allows that, he/she could be expected to carry out the relevant escaping in its application code...

@gggeek
Copy link
Owner

gggeek commented Nov 25, 2022

PS: the specific error message reported by the security scanner for the curl_init call is the same message used for line 46 in logger.php, which is a print call.

The only way that I can think of for an "argument injection" to be achieved via a "print" call is by using the output of this library to create a command, which is then executed via a shell.

I wonder if the issue here is not in a known vulnerability in the curl_init call itself, but rather the tool thinking that the final results of the curl call could be echoed as output by the application, again resulting in a command injection if such output is used to build a command.

To be honest, the only way for me to make a proper, detailed, irrefutable analysis of this ticket, as well as of ticket #82, would be to talk to the developers of the security scanner in use, to better understand which code scanning rules are exactly triggering these reports. Attack scenarios, either in code or pseudo-code, would also do.

@gggeek
Copy link
Owner

gggeek commented Nov 25, 2022

After some playing around, I found something which I think could be classified as a very low impact security issue.

The "low impact" assessment is related to the fact that the developer would have to go out of his/her own way in order to allow a malicious actor to take advantage of the problem, short of having already gained Remote Code Execution.

I will release a fix in the form of updated usage documentation in the next release, which should not be very far.

@gggeek
Copy link
Owner

gggeek commented Nov 28, 2022

In the end, I decided to go for a code fix rather than pure documentation.

Detailed explanation of the specific conditions in which this issue might be abused are in https://github.com/gggeek/phpxmlrpc/releases/tag/4.9.0

Thanks for reporting this - and sorry for taking so long to fix it. I did underestimate the reported security-related tickets because there was little information provided regarding the exact problem scenario / underlying issue, and the reports seemed to come from an automated scanner tool, run without any verification of its findings, and my own experience that leads usually to a large number of false positives.

@gggeek gggeek closed this as completed Nov 28, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants