Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

ascanrules: add threshold to SQLi and Path Traversal #5336

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mikhailevtikhov
Copy link
Contributor

@mikhailevtikhov mikhailevtikhov commented Mar 6, 2024

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
  • Update changelog
  • Run ./gradlew spotlessApply for code formatting
  • Write tests
  • Check code coverage
  • Sign-off commits
  • Squash commits
  • Use a descriptive title

Copy link

github-actions bot commented Mar 6, 2024

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

@mikhailevtikhov
Copy link
Contributor Author

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

@mikhailevtikhov

This comment has been minimized.

zapbot added a commit to zaproxy/cla that referenced this pull request Mar 6, 2024
@thc202
Copy link
Member

thc202 commented Mar 7, 2024

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

@thc202 thc202 changed the title Add threshold to ascan rules SQLi and Pathtraversal ascanrules: add threshold to SQLi and Path Traversal Mar 7, 2024
@mikhailevtikhov
Copy link
Contributor Author

@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
Copy link
Member

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

Added FP test in Path Traversal

Signed-off-by: mikhail.evtikhov <[email protected]>
@kingthorin
Copy link
Member

kingthorin commented Mar 13, 2024

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.

… functions

Enhanced regex for nix architecture

Signed-off-by: mikhail.evtikhov <[email protected]>
@thc202
Copy link
Member

thc202 commented Apr 4, 2024

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

@mikhailevtikhov
Copy link
Contributor Author

Did it in PR 5387.

@mikhailevtikhov
Copy link
Contributor Author

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

@thc202
Copy link
Member

thc202 commented Apr 19, 2024

I'm aware of the pull requests.

@thc202
Copy link
Member

thc202 commented May 16, 2024

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

@mikhailevtikhov
Copy link
Contributor Author

@kingthorin @thc202
Everything that is implemented in PR #5387 has been removed in this PR.
But I'll wait with rebase until I figure out what I did wrong in another PR... 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants