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

Java: Remove local query variants. #16362

Merged
merged 14 commits into from May 16, 2024

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Apr 30, 2024

In this PR we remove the local query variants.

The results from the queries can still be found by configuring threat models to also include local sources and using the non local query variants.

Some notes to consider in terms of review:

I have not changed the java/tainted-permissions-check query. It is hardcoded to use UserInput as sources (both remote and local sources). Should it be considered to make this an threat model opt-in instead?

There are still some experimental queries where the exist a local and non local query variant.

Comments to the DCA experiment:
DCA was run on the nightly source suite and the security extended query suite and for variant v0 the threat model configuration was the default (remote sources) and for variant v1 the threat model configuration was set to remote and local.

  • Performance drop of 7%. This looks acceptable. In variant v0 only remote sources are included and in in v1 both remote and local sources are included for queries that have opt'ed in to threat models.
  • Approximately 7k extra alerts found.
  • The only queries that showed a decrease in the number of alerts are: java/concatenated-command-line and java/concatenated-sql-query. This is not surprising as java/concatenated-command-line excludes all findings by java/command-line-injection (which shows an increase in findings) and java/concatenated-sql-query excludes all findings by java/sql-injection (which also shows an increase in findings).
  • A similar experiment was run on Java: Enable threat models for most Java queries. #14370 where the quality of the results were checked (and that the combined sources found the combined results).

Copy link
Contributor

QHelp previews:

@michaelnebel michaelnebel force-pushed the java/removelocalqueries branch 2 times, most recently from 9ba9a11 to cf6b755 Compare May 1, 2024 06:56
@michaelnebel michaelnebel marked this pull request as ready for review May 2, 2024 11:01
@michaelnebel michaelnebel requested a review from a team as a code owner May 2, 2024 11:01
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

This is great, thank you! 🎉

Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

LGTM!

I have not changed the java/tainted-permissions-check query. It is hardcoded to use UserInput as sources (both remote and local sources). Should it be considered to make this an threat model opt-in instead?

The query is so old that it probably predated remote flow sources and the old UserInput class was used instead. So it probably would make sense to adapt it to threat models as well, but we can defer this decision and change it in another PR if you want.

Since this change is pretty impactful, maybe we should coordinate communication with @coadaflorin before merging/releasing.

@michaelnebel
Copy link
Contributor Author

@atorralba : Thank you for the review (and I agree with your comment)

@coadaflorin : Last year we discussed the removal of local versions of the security queries. Now that threat models has been properly released and public documentation is available for enabling (the local) threat models, it makes (in my opinion) that we delete the local variants. Is that acceptable from a product perspective?

@coadaflorin
Copy link
Contributor

We already announced when we released C# threat models that for some queries we removed the local sources, so I think it's fair that we follow-up with this work.

If someone manually included this query in their suite, what would they see?

@michaelnebel michaelnebel merged commit b1329fd into github:main May 16, 2024
18 checks passed
@michaelnebel michaelnebel deleted the java/removelocalqueries branch May 16, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants