Skip to content

Conversation

d10c
Copy link
Contributor

@d10c d10c commented Jul 17, 2025

This PR enables diff-informed mode on queries that select a location other than dataflow source or sink. This entails adding a non-trivial location override that returns the locations that are actually selected.

Prior work includes PRs like #19663, #19759, and #19817. This PR uses the same patch script as those PRs to find candidate queries to convert to diff-enabled. This is the final step in mass-enabling diff-informed queries on all the languages.

Commit-by-commit reviewing is recommended.

  • I have split the commits that add/modify tests from the ones that enable/disable diff-informed queries.
  • If the commit modifies a .qll file, in the commit message I've included links to the queries that depend on that .qll for easier reviewing.
  • Feel free to delegate parts of the review to others who may be more specialized in certain languages.

Potentially tricky cases:

@@ -0,0 +1 @@
experimental/Security/CWE-208/TimingAttackAgainstHash/TimingAttackAgainstHash.ql

Check warning

Code scanning / CodeQL

Query test without inline test expectations Warning test

Query test does not use inline test expectations.
@d10c d10c added the no-change-note-required This PR does not need a change note label Jul 17, 2025
@d10c d10c marked this pull request as ready for review July 17, 2025 14:27
@d10c d10c requested a review from a team as a code owner July 17, 2025 14:27
@d10c d10c requested review from michaelnebel and igfoo July 17, 2025 14:27
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

  • Thank you for fixing the select statements in the queries. They looked wrong before as .getResultType() is a string and not something that has a meaningful location.
  • The DCA run doesn't run the changed queries (nightly.qls doesn't contain the experimental queries). Consider running security-experimental instead. This suite contains py/possible-timing-attack-against-hash but it doesn't contain py/timing-attack-against-hash (as this query doesn't have the experimental tag - which looks like a mistake though).
  • IMO your don't need to add a new test case for the query as these are purely experimental queries and since the query in question (py/timing-attack-against-hash) is not even in the experimental suite. If you are interested in playing around with making a proper test, then as you write in the description, one way of emulating remote user input could be by using MaD. I am not familiar with MaD for Python, but maybe inspiration can be drawn from here (where kind needs to be remote instead) - making an ext.yml file next to the test can make some remote sources that only has the test as scope.

@d10c
Copy link
Contributor Author

d10c commented Aug 15, 2025

Started a security-experimental DCA run (see above); rerunning failed workflow.

@michaelnebel
Copy link
Contributor

but it doesn't contain py/timing-attack-against-hash (as this query doesn't have the experimental tag - which looks like a mistake though).

Might not be a mistake actually, as the produced alerts are a subset of the alerts produced by py/possible-timing-attack-against-hash. I would recommend not trying to spend time on making a test for it.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

LGTM!
Can be merged unless DCA indicates otherwise.

@d10c
Copy link
Contributor Author

d10c commented Aug 15, 2025

10s slowdown on py/possible-timing-attack-against-hash on cpython, but given that we don't see a similar slowdown on other dbs or on py/timing-attack-against-hash (same kind of change), it could be a fluke.

@michaelnebel
Copy link
Contributor

10s slowdown on py/possible-timing-attack-against-hash on cpython, but given that we don't see a similar slowdown on other dbs or on py/timing-attack-against-hash (same kind of change), it could be a fluke.

I don't think you need to worry about it. If it had been consistent across more repos, then it would have been worth investigating. DCA looks good to me.

@d10c d10c merged commit 4199859 into github:main Aug 18, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants