ascanrules: add threshold to SQLi and Path Traversal

[mikhailevtikhov]

Overview

PathTraversal directory browsing detects (Check 3): This check finds a large number of FP in various web applications, because the regular expression that parses the response from the server for the nix architecture will not accurately determine that this is the root directory of the OS. For example, to generate FP on payload "c:" or any other from the LOCAL_DIR_TARGETS array, the words in the response from the server will be enough (etc. , bootstrap.min.js , tabindex) to generate Aletr. The reason for this is a weak pattern check for nix systems implemented in DirNamesContentsMatcher. You can play this FP on Apache Superset.

SQLi expression based (Check 4): Similarly, this check can find FP in certain web applications. For example, when a parameter with an int value is present in the URL, which for some reason is ignored by the server and regardless of what is passed to this parameter, the same page will return. In this case, Expression Based SQLi ascan rule can successfully perform mathematical operations to convert the original int value and generate an Alert.

SQLi expression based and PathTraversal directory browsing detects can be useful and bring real detectors, but in the current implementation they bring a large amount of FP. At the moment, these types of sub-checks do not have any adjustment. I suggest adding a Threshold for them so that the user can disable/enable verification data depending on his needs.

Checklist

• [ ] Update help
• [x] Update changelog
• [x] Run ./gradlew spotlessApply for code formatting
• [x] Write tests
• [ ] Check code coverage
• [x] Sign-off commits
• [ ] Squash commits
• [x] Use a descriptive title

[github-actions[bot]]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[mikhailevtikhov]

I have read the CLA Document and I hereby sign the CLA

[mikhailevtikhov]

recheck

[thc202]

I'd suggest provide evidence of the FPs, better as tests. (In any case this needs tests for the new behaviour.)

[mikhailevtikhov]

@thc202 Do I need to do unit tests aimed at FP, which will successfully pass the gradlew test or which will fail? Thank you.

[kingthorin]

Test that test the current behavior, so that if changes in the future break it the tests will fail ci and we'll know.

[kingthorin]

I'm not convinced this is the right option/solution for either of these.

For path traversal, I'm confused with your example because it flips between nix and windows, but either way, if the issue is weak patterns, then let's improve the patterns. (Maybe the existing response should be prechecked, or maybe they should include a word boundary requirement? 🤷‍♂️ )

There are other SQLi PRs in flight. I'm not sure if they touch this code, but again, improvements to detection logic vs. logical skipping seems better.

Lastly, parameterizing unit tests to pass a single value seems unnecessary.

[thc202]

Might be better to split what's ready to other PR.

[mikhailevtikhov]

Did it in PR 5387.

[mikhailevtikhov]

@thc202 Can you do a review of this PR(5387) please?

[thc202]

I'm aware of the pull requests.

[thc202]

@mikhailevtikhov could you remove the changes done in the other PR? (And rebase both PRs too.)

[mikhailevtikhov]

@kingthorin @thc202 Everything that is implemented in PR https://github.com/zaproxy/zap-extensions/pull/5387 has been removed in this PR. But I'll wait with rebase until I figure out what I did wrong in another PR... :sweat: