Improvements to Active Scanning's Time-based Blind OS Command Injection

[Aaron Eason]

Hello, I haven't been using ZAP for very long so bare with me if I've missed something.

On an engagement, ZAP missed a blind OS command injection that I eventually found, but took much longer. I wanted to understand why it didn't raise an alert, so I dove into the source code (albeit after tweaking config and attempting to research the behavior) for the Command Injection plugin in the Active Scanning add-on.

I noticed that the time-based blind detection uses a heuristic that only throws an alert if the delay is longer than the requested delay (ie sleep 5 must take longer then 5 seconds) AND if the delay is in the 99.9999999997440 percentile of delays for that endpoint. The first condition makes sense to me, but the second one doesn't.

I did check the git blame and past issues surrounding this behavior, but this part of the addon hasn't really changed since 2014 and I'm not sure what the justification is for the delay needing to be 7 standard deviations away in order to be significant. I imagine this is to help prevent false positives, but I think it breaks the detection significantly in the following fairly reasonable situations:

1. The endpoint in question already takes a long time (ie the mean is already high). In my case, the payload was being passed to a media conversion executable, so 5 sec+ delays were already the norm. 
2. The endpoint in question takes a varied amount of time (ie the standard deviation is high). In my case, some of the other payloads would break the conversion, so the delays were scattered between 5 sec+ to basically instant when the script call fails.

I was hoping to learn why it was written this way and if there are any plans to improve the heuristic to address the above situation.

My suggestion would be to instead send multiple very small delays, ie `sleep 1, 2, ..., 5`, and perform a simple linear regression to see if the increment in payload delay correlates to an increment in actual delay. Then you could tie that numerical confidence threshold to the actual threshold configured for the add-on.

Let me know what you think, and thanks in advance!

[psiinon]

I cant remember the justification for this check either :D

But I would agree that its probably to reduce false positives.

This is the first time I can remember anyone reporting this as a problem, so up until now there have been no plans to change it.

I'm all for improving any rule, but you do need to be aware that we have to do our best to limit the number of requests any one rule makes.

However I'm guessing that in this case we only need to keep trying longer delays if the endpoint keeps taking longer than the delays specified?

Cheers,

Simon

[Aaron Eason]

Hey Simon,

Yeah, the idea was to keep the check "actual delay > requested delay".  Then the plugin can open with the 5s sleep, which is what it currently does. On any endpoint that takes less than 5 seconds, it'll just move on. If it actually takes longer than 5 seconds, we can send a few smaller follow up delays to do some statistics against. As soon as we reach the confidence threshold, we can throw an alert. If we still don't meet the confidence threshold after sending "n" requests, we'll move on.

For the record, I am willing to take a crack at a PR if you think this is a good idea. Whatever would create less work, especially if I'm the only one to have a problem with it in 8 years. :)

Thanks,
Aaron

[kingthorin+owaspzap]

You definitely aren't the only person to encounter this in 8 years.

There are definitely a number of SQLi rules that could probably benefit from this. I wonder if there's a way it could be done somewhat generically and leveraged in all time based checks/rules?

PRs are welcome, feel free to ask questions here. It would also be good to ensure no drop in coverage vs wavsep.

[Aaron Eason]

Excellent suggestion. I'll start by messing around with a PR to add the described behavior to Blind OS Command Injection and if that works out and the coverage is good, I'll look into making sure the logic is generalized and can be easily picked up by other plugins.

Thanks for the speedy responses, but the PR might take me a few days to get around to. If I have any questions I'll fire them here.

[Diogo Silva]

Hi, one way to do it in a generic way is to have a function in common lib “testTimeDelays(sendPayloadForDelay sendDelay)” with the times comparison  logic. Each scanning rule for each different payload would create an implementation of “sendPayloadForDelay(int sec)” that would send a request with payload that was expected to cause a delay of “sec” seconds. Need to have 2 of these function one to handle absolute delays and one to handle relative, like the ones in SQLite time based detection. I can help on this if you want.

[Aaron Eason]

Quick update. I've submitted a PR with the changes I discussed here and I've also generalized it in commonlib as suggested.
I'll be watching the PR more closely so if you want to follow up, I suggest doing so on Github.

First time contributing to the project so I'm sure I missed something, thanks in advance.