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

new: add option to skip discovery #644

Merged
merged 3 commits into from
Mar 14, 2025
Merged

Conversation

noperator
Copy link
Contributor

Fixes Skip discovery/analysis · Issue #643 · hahwul/dalfox.

Successfully tested this change on Lab: Stored XSS into HTML context with nothing encoded | Web Security Academy:

./dalfox sxss https://<LAB>.web-security-academy.net/post/comment \
    -X POST \
    -H 'Host: <LAB>.web-security-academy.net' \
    -H 'User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/133.0.0.0 Safari/537.36' \
    -H 'Accept: */*' \
    -H 'Content-Type: application/x-www-form-urlencoded' \
    -H 'Cookie: session=<SESSION>' \
    -d 'comment=example_value&csrf=<CSRF>&email=user%40example.com&name=example_value&postId=10&website=https%3A%2F%2Fwww.example.com' \
    --request-method GET \
    --trigger 'https://<LAB>.web-security-academy.net:443/post?postId=10' \
    -p comment \
    --format json \
    --proxy http://127.0.0.1:8081 \
    --skip-discovery \
    --skip-headless

Target                 https://<LAB>.web-security-academy.net/post/comment
Method                 POST
Worker                 100
BAV                    false
Mining                 false (Gf-Patterns)
Mining-DOM             false (mining from DOM)
Timeout                10
FollowRedirect         false
Started at             2025-03-04 10:51:05.202806 -0500 EST m=+0.003689042

 >>>>>>>>>>>>>>>>>>>>>>>>>
[
[*] 🦊 Start scan [SID:Single] / URL: https://<LAB>.web-security-academy.net/post/comment
[I] Discovery phase and content-type checks skipped. Testing with 1 parameters from -p flag
[V] Triggered XSS Payload (found DOM Object): comment="><Svg/onload=alert(1) class=dlafox>
{"type":"V","inject_type":"inHTML","poc_type":"plain","method":"POST","data":"https://<LAB>.web-security-academy.net/post/comment?comment=%2522%253E%253C%2553%2576%2567%252F%256F%256E%256C%256F%2561%2564%253D%2561%256C%2565%2572%2574%2528%2531%2529%2520%2563%256C%2561%2573%2573%253D%2564%256C%2561%2566%256F%2578%253E -d comment=example_value\u0026csrf=<CSRF>\u0026email=user%40example.com\u0026name=example_value\u0026postId=10\u0026website=https%3A%2F%2Fwww.example.com","param":"comment","payload":"\"\u003e\u003cSvg/onload=alert(1) class=dlafox\u003e","evidence":"","cwe":"CWE-79","severity":"High","message_id":288,"message_str":"Triggered XSS Payload (found DOM Object): comment=\"\u003e\u003cSvg/onload=alert(1) class=dlafox\u003e"},
 ⠹  [9/272 Queries][3.31%] Passing "comment" param queries^C

@hahwul, a few questions I had while making this change:

  • are the results of discovery used anywhere downstream that would break if discovery isn't actually performed?
  • should the -p option by mandatory if --skip-discovery is used (I assume yes)? since we won't have discovered any params
  • should we bypass Content-Type checks when skipping discovery (I assume yes)?
  • do we need to fill out param fields that are normally populated during discovery?
    • param type: e.g., URL, form, path, header, cookie, etc. will dalfox locate the param automatically, or should we specify the type explicitly (perhaps by -p username:form or something)?
    • special chars: not sure how this is normally determined
    • reflected: will it cause problems if we initially assume true?

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @noperator, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request introduces a new --skip-discovery option to dalfox, addressing issue #643. When this flag is used, dalfox will skip the initial discovery phase and proceed directly to XSS scanning, relying on parameters provided via the -p flag. The changes involve modifying cmd/args.go, cmd/root.go, pkg/model/options.go, and pkg/scanning/scan.go to add the new flag, configure it, and adjust the scanning logic accordingly. The PR author has also raised several questions regarding the implications of skipping discovery, such as handling downstream dependencies on discovery results, the necessity of the -p option, bypassing content-type checks, and populating parameter fields normally determined during discovery.

Highlights

  • New Feature: --skip-discovery flag: Adds a new command-line option --skip-discovery to bypass the discovery phase and directly initiate XSS scanning using user-provided parameters.
  • Parameter Handling with --skip-discovery: When --skip-discovery is enabled, the -p flag becomes essential for specifying parameters to be scanned. The code now checks for the presence of parameters when --skip-discovery is used.
  • Content-Type Check Bypass: The PR modifies the content-type check logic to allow bypassing it when --skip-discovery is enabled, allowing custom payloads to be used even if the content type is not typically allowed.
  • Parameter Type Handling: When skipping discovery, the parameter type is defaulted to URL, reflection is assumed to be true, and special characters are not determined.

Changelog

Click here to see the changelog
  • cmd/args.go
    • Added SkipDiscovery field to the Args struct to store the value of the --skip-discovery flag.
  • cmd/root.go
    • Added the --skip-discovery flag to the command-line options, binding it to the SkipDiscovery field in the Args struct.
    • Configured the SkipDiscovery option to be passed to the Options struct.
  • pkg/model/options.go
    • Added SkipDiscovery field to the Options struct to store the skip discovery setting.
  • pkg/scanning/scan.go
    • Implemented the logic to skip the discovery phase when options.SkipDiscovery is true.
    • Added a check to ensure that parameters are provided via the -p flag when skipping discovery.
    • Populated the params map with user-provided parameters when skipping discovery, setting default values for parameter attributes.
    • Bypassed content-type checks when skipping discovery.
    • Modified the conditional logic for custom payload and common payload setting to account for the skip discovery option.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


The scanner sleeps, no need to roam,
Discovery's path, it leaves alone.
With parameters set, and targets in sight,
It leaps to the scan, with all of its might.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The pull request introduces a new feature to skip the discovery phase and proceed directly to XSS scanning. This is a useful addition for users who know the parameters they want to test. The implementation is generally well-structured, but there are a few areas that could be improved for clarity and robustness.

Summary of Findings

  • Parameter Type Handling: When skipping discovery, the parameter type is hardcoded to "URL". It might be beneficial to allow users to specify the parameter type (e.g., URL, FORM, COOKIE) for more accurate testing.
  • Content-Type Bypass: The code sets a dummy content type of "text/html" to bypass content type checks when skipping discovery. While this works, it might be better to either skip the content type check entirely or allow the user to specify a content type.
  • Error Handling: The error message for missing parameters when using --skip-discovery could be more user-friendly. Suggesting how to use the -p flag would be helpful.

Merge Readiness

The code changes introduce a useful feature, but there are a few suggestions for improvement. I would recommend addressing these before merging to enhance the user experience and robustness of the tool. I am unable to approve this pull request, and recommend that others review and approve this code before merging.

@hahwul
Copy link
Owner

hahwul commented Mar 5, 2025

@noperator
First of all, thank you so much for submitting the PR! I’ll address the question you raised below.

  • are the results of discovery used anywhere downstream that would break if discovery isn't actually performed?
    • The core of discovery lies in creating the parameters to analyze. As long as the parameters to be analyzed are clearly provided, there’s no problem.
  • should the -p option by mandatory if --skip-discovery is used (I assume yes)? since we won't have discovered any params
    • Yes, I agree.
  • should we bypass Content-Type checks when skipping discovery (I assume yes)?
    • Yes, I agree that skipping Content-Type checks should be fine. This feature was originally introduced to reduce unnecessary tests during bulk scans, so bypassing it in this scenario should not cause issues.
  • do we need to fill out param fields that are normally populated during discovery?
    • Param Type: Currently, all parameters with the same name as those provided via -p are tested. While explicitly specifying types (e.g., -p username:form) could be beneficial in the long run, skipping discovery should not pose significant issues, as parameters with matching names will still be tested.
    • Special chars: During the XSS scan phase, payloads containing special characters that do not get reflected are excluded. If discovery is skipped, the list of allowed special characters will not be generated, which could affect scan accuracy. A possible solution would be to include all special character patterns by default when discovery is skipped.
    • Reflected: No, I think there won’t be any problems.

Also, please note that we’re currently in the process of refactoring the entire codebase, so even after merging this feature, improvements and changes may continue to happen. Thanks for keeping this in mind!

Copy link

codecov bot commented Mar 14, 2025

Codecov Report

Attention: Patch coverage is 0% with 70 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/scanning/scan.go 0.00% 70 Missing ⚠️

📢 Thoughts on this report? Let us know!

@hahwul hahwul merged commit b8aa52a into hahwul:main Mar 14, 2025
7 checks passed
@hahwul
Copy link
Owner

hahwul commented Mar 14, 2025

@noperator
I’ve made a few more fixes and merged them! If you spot anything else that could use some improvement, please feel free to raise an issue or submit a PR.

Thanks again, and see you in v2.10.0!

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

Successfully merging this pull request may close these issues.

Skip discovery/analysis
2 participants