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: Fix FP in Path Traversal directory browsing checks #5387

Merged
merged 7 commits into from
Jun 21, 2024

Conversation

mikhailevtikhov
Copy link
Contributor

@mikhailevtikhov mikhailevtikhov commented Apr 4, 2024

Overview

This was extracted from #5336
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.

In this PR checks for windows and nix directories are implemented with different functions and enhanced regex for nix architecture.

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

@mikhailevtikhov
Copy link
Contributor Author

mikhailevtikhov commented Apr 12, 2024

@kingthorin
I figured it out in more detail. There are two scenarios in this situation.

  1. It can be directory browsing at the web server level. In this case, you can focus on the examples of the response from the server.
   1.1 "<td><a href=\"/etc/\">etc</a></td>" ...
   1.2 "<li><a href="bin/">bin/</a></li>" ...
  1. It can be directory browsing at the web application level and be given in plain text.
curl http://localhost:4444/ls\?directory\=../../../
sbin
usr
mnt
lib
...
root
srv
opt
home
etc
bin

For this reason, I decided to replace regex. Slightly increase the number of checks to get rid of FP accurately, as well as remove the hardcoded "href" in order to match plain text.

@mikhailevtikhov
Copy link
Contributor Author

@kingthorin
What are my next steps?

@kingthorin
Copy link
Member

Someone else in the team needs to review and approve (hopefully).

@mikhailevtikhov
Copy link
Contributor Author

ping

@mikhailevtikhov
Copy link
Contributor Author

@kingthorin
What should I do to get this PR merged?) How do I invite other Reviewers?

@thc202
Copy link
Member

thc202 commented Jun 7, 2024

As mentioned in #5336 (comment) you should rebase both PRs.

@kingthorin
Copy link
Member

If you need help with the rebases lemme know I can tackle it

Copy link

github-actions bot commented Jun 7, 2024

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

@mikhailevtikhov
Copy link
Contributor Author

@kingthorin looks like something went wrong...

@kingthorin
Copy link
Member

Okay no problem, I'll have a look 👀 in a few hours

… functions

Enhanced regex for nix architecture

Signed-off-by: mikhail.evtikhov <[email protected]>
Added test for plain text dir_based pathTraversal

Signed-off-by: mikhail.evtikhov <[email protected]>
Fixed a typo in the tests.

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

Okay so we have two options:

  1. You turn on "Allow edits by maintainers" (you should see it in the menu on the right). Then I can push a copy in the proper state.
  2. You check this comparison: https://github.com/zaproxy/zap-extensions/compare/main...kingthorin:zap-extensions:pathTraversalDirBased?expand=1
    • Assuming it's correct you add my fork as a git remote git remote add kingthorin https://github.com/kingthorin/zap-extensions.git
    • Fetch my remote git fetch kingthorin
    • Delete your local branch git branch -d pathTraversalDirBased (If it complains that it isn't fully merged you might need to use -D)
    • Checkout the branch on my fork (that's in the proper state) git checkout kingthorin/pathTraversalDirBased
    • Push that back to your fork, replacing the messed up version git push -u origin pathTraversalDirBased

@thc202
Copy link
Member

thc202 commented Jun 8, 2024

The option 1 will not be present, this PR is from an org's fork.

@mikhailevtikhov
Copy link
Contributor Author

@kingthorin
Please tell me if I can try the following:

  1. Update the fork of the organization;
  2. Create a new branch;
  3. Cheery-pick the necessary commits;
  4. Push a branch into a fork;
  5. Checkout on pathTraversalDirBased from this PR
  6. Rebase pathTraversalDirBased from this PR on new_branch
  7. Push new state of pathTraversalDirBased

@kingthorin
Copy link
Member

Discussed this with the team. We think that "should" work, though it actually seems like more effort than my option 2 😉

@mikhailevtikhov
Copy link
Contributor Author

mikhailevtikhov commented Jun 13, 2024

@kingthorin
I am trying your option 2. But I encounter the following error at the last stage.

git push -u origin pathTraversalDirBased     

To https://github.com/vulnspace/zap-extensions.git
 ! [rejected]            pathTraversalDirBased -> pathTraversalDirBased (non-fast-forward)
error: failed to push some refs to 'https://github.com/vulnspace/zap-extensions.git'

@kingthorin
Copy link
Member

You probably need to use --force since you're rewriting history

@mikhailevtikhov
Copy link
Contributor Author

Looks like it worked :D

@kingthorin
Copy link
Member

That seems to be back on track. Thanks

@thc202
Copy link
Member

thc202 commented Jun 21, 2024

Can you address the conflict?

@mikhailevtikhov
Copy link
Contributor Author

Can you address the conflict?

Done.

@thc202 thc202 enabled auto-merge (squash) June 21, 2024 14:02
@thc202 thc202 merged commit 24fd523 into zaproxy:main Jun 21, 2024
10 checks passed
@thc202
Copy link
Member

thc202 commented Jun 21, 2024

Thank you!

@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2024
@kingthorin
Copy link
Member

@mikhailevtikhov thanks for sticking with this and seeing it through!

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

Successfully merging this pull request may close these issues.

3 participants