search

zaproxy / zaproxy

The ZAP by Checkmarx Core project
https://www.zaproxy.org
Apache License 2.0
12.55k stars 2.24k forks source link

False positive w.r.t OS Command Injection #2588

Open abhijeth opened 8 years ago

abhijeth commented 8 years ago

According to the current ZAP scanning rule set, ZAP reports OS command injection based on "sleep/timeout" conditions. Quite often the application in general takes more than 5s time to return a respond to the any malformed request. This increases the possibility of this being a false positive and creating unwanted alerts. Hence this rule set might not be appropriate to report OS Command injection and might make more sense if other test cases are used.

thc202 commented 8 years ago

Could you provide more information about the "malformed request", what was wrong in the request? Was ZAP sending wrong value in Content-Length header field? Is that kind of issues? Or, because of what's being injected?

abhijeth commented 8 years ago

Oops sorry for the delay here. I mean when the original request is modified/tampered and if the application cannot understand it, the response might still be delayed.

For example: id=10 is valid. id=123adf213 might be invalid so the web server might cause a delay in the response. So, when a payload like sleep/timeout is given, it is not necessary that the delay in the response is because of the command passed. So it might not be appropriate to judge if a page is vulnerable to OS Command injection or not based on sleep/time out etc.

I found a bunch of false positives previously but I will make sure to post the screenshot here as soon as I find the next one.

kingthorin commented 8 years ago

Could leverage https://github.com/zaproxy/zap-extensions/pull/444/files ?

https://github.com/zaproxy/zap-extensions/blob/master/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/CommandInjectionPlugin.java

thc202 commented 7 years ago

@kingthorin yeah, I ended up doing that for #3037 (which is the same issue as this one), although that's just a workaround.

rnehra01 commented 7 years ago

@thc202 , @kingthorin The issue can be resolved by making the same query again but with an increased sleep time. So next elapsed sleep time should be longer than previous. This will confirm CODEINJECTION. I'll resolve it if the solution sounds correct.

kingthorin commented 7 years ago

As long as the second timeout doesn't exceed the default connection timeout.

thc202 commented 7 years ago

Although that check is something that could be added it might not fix this issue. This FP happens because the RTT is not being correctly taken into account. If the RTT takes more time than the sleep time it will end up alerting (same could happen with the "confirmation sleep").

psiinon commented 2 years ago

The default timeout in now 15 seconds, and that is configurable by the user: https://www.zaproxy.org/docs/desktop/ui/dialogs/options/ruleconfig/ Can we close this issue on that basis?

kingthorin commented 2 years ago

The user did indicate that changing the timeout worked, however as @thc202 pointed out it's probably "more" correct to account for round-trip-time on a few 'normalizing' requests for comparison.

This might also benefit from the idea of a follow-up check being done single threaded (I believe that's discussed in another issue WRT SQLi FPs).

Edit: Maybe this specific ticket should be closed and open another WRT RTT checks?