Python: Diff-informed queries: phase 3 (non-trivial locations) - #20079
Conversation
Sorry, something went wrong.
…urce (TODO: add source with true positive)
There was a problem hiding this comment.
- 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.
Sorry, something went wrong.
|
Started a security-experimental DCA run (see above); rerunning failed workflow. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
michaelnebel
left a comment
There was a problem hiding this comment.
LGTM!
Can be merged unless DCA indicates otherwise.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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.
Potentially tricky cases: