Skip to content

Remove dependencies on isSink in the taint tracking configurations of the default queries #180

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

Merged
merged 6 commits into from
Apr 3, 2025

Conversation

jeongsoolee09
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 commented Mar 28, 2025

Remove dependencies on isSink of the standard library configurations in the custom taint tracking configurations. This will hopefully remove the duplicate XSS alerts raised by the default queries. These are done by:

  1. Not making our custom configurations extend the default ones; don't mix up the two, and make the custom configuration be on its own one.
  2. Removing super.isSink(...) in the custom configurations to make the kind of vulnerabilities sink dependent.
    • For example, if a UI5 function call happens to be the sink of an XSS, it's a UI5 XSS and should only be raised by the UI5Xss query.
  3. Keeping the sources from the default configurations in the isSource definitions.

@jeongsoolee09 jeongsoolee09 changed the title Remove Remove dependencies on taint tracking configurations of the default queries Mar 28, 2025
@jeongsoolee09 jeongsoolee09 changed the title Remove dependencies on taint tracking configurations of the default queries Remove dependencies on isSink in the taint tracking configurations of the default queries Mar 28, 2025
Copy link
Contributor

@lcartey lcartey left a comment

Choose a reason for hiding this comment

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

Thanks @jeongsoolee09!

As noted in the review comments, I think it's both fine and desirable for us to extend the default configurations for the queries we've implemented, so long as we ensure isSink specifies only SAP specific sinks not reported by the standard library. Then we're guaranteed to avoid duplication.

@jeongsoolee09
Copy link
Contributor Author

Thanks @lcartey! I agree with the CAP log injection and XSJS Reflected XSS queries needing to extend the default configurations, but will deleting the UrlRedirect::LocationSink in CAPLoginjectionConfiguration.isSink remove the URL Redirect alert raised from a default query?

The only thing that line does is to make queries with CAPLogInjectionConfiguration raise an alert on URL redirection sinks, and that doesn't contribute anything to the default URL redirection query.

@jeongsoolee09
Copy link
Contributor Author

Yep, totally agree - I made the configurations extend the default ones again, but retained the deletion of UrlRedirect::Sink in UI5Xss.

@jeongsoolee09 jeongsoolee09 requested a review from lcartey April 3, 2025 19:13
Copy link
Contributor

@lcartey lcartey left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@jeongsoolee09 jeongsoolee09 merged commit 39b8171 into main Apr 3, 2025
5 checks passed
@jeongsoolee09 jeongsoolee09 deleted the jeongsoolee09/remove-duplicate-alerts branch April 3, 2025 20:35
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.

2 participants