Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The metric includes disabled sources, excludes valid nodes, and omits its denominator row.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds Unified taint-reach metrics for DCA reporting.
Changes:
- Computes tainted-node counts and per-million reach.
- Exposes reach metrics through summary statistics.
| File | Description |
|---|---|
unified/ql/lib/codeql/unified/internal/AnalysisQuality.qll |
Adds taint-reach analysis and reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| module TaintReach { | ||
| private class Candidate extends DataFlow::Node { | ||
| Candidate() { exists(this.asExpr()) } |
There was a problem hiding this comment.
I would have written any() here as well I think. But if there's a reason you restricted candidate nodes to those corresponding with expressions, I think it's fine and we still have a useful metric.
There was a problem hiding this comment.
We avoid counting synthetic nodes, so we don't move the goal posts whenever synthetic nodes are added/removed. E.g. something like the introduction of use-use flow can add a large number of synthetic nodes, but then you can't compare the taint reach numbers across that change.
IMO Rust should should also stop measuring synthetic nodes.
There was a problem hiding this comment.
Ah, then I agree with you, that has been a problem with the rust taint reach statistic.
👍
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
LGTM.
Getting DCA to show the numbers will require some (minor) changes in that repo. Let me know if you've hit problems there. Or I'm happy to do it, I'm quite familiar with how that stuff is wired.
| module TaintReachConfig implements DataFlow::ConfigSig { | ||
| predicate isSource(DataFlow::Node node) { Models::isSource(node, _) } | ||
|
|
||
| predicate isSink(DataFlow::Node node) { node instanceof Candidate } |
There was a problem hiding this comment.
This might work for now, but I would expect performance to eventually be horrendous to the point of breaking dca. If you were to try this on e.g. Java or C# then I'd be fairly certain that it just wouldn't work. We could perhaps draw the numbers from an early data flow stage - that would be a coarse over-approximation, but could guard against severe performance degradation.
There was a problem hiding this comment.
It's somewhat alleviated by the use of flowTo instead of flowPath and for the languages where we do this (JS, Ruby, Swift, and Rust) it hasn't actually been that bad. I don't think unified needs to be different than the other languages in this respect.
But it would be great we could get the data flow library to report a better version of taint reach.
There was a problem hiding this comment.
But it would be great we could get the data flow library to report a better version of taint reach.
See below for a stage-2-representation. I don't think it's possible in general, otherwise we could calculate taint up front and share it across queries instead of having individual configurations.
There was a problem hiding this comment.
I'd expect stage 3 and beyond to be infeasible for a large chunk of repos. And given that I expect the trouble to begin already there, the improvement of using flowTo instead of flowPath won't help.
There was a problem hiding this comment.
Huh, I just tested on a couple of Java repos and I must admit that this was surprisingly more feasible than I had imagined. It won't work for customer dbs, but I'll concede that it indeed might work on dca.
There was a problem hiding this comment.
I can actually calculate stage 4 on a very large Java db (but stage 5 failed).
There was a problem hiding this comment.
I guess that's perhaps somewhat explainable - stage 4 limits the access path representation to the (precise) head, so type pruning likely prevents cartesian blowups in many places.
In any case, that's probably enough of my hypothetical rambling - let's go with what we have if it works, and then we can always limit it to e.g. stage 4 if necessary.
|
|
||
| module TaintReachFlow = TaintTracking::Global<TaintReachConfig>; | ||
|
|
||
| DataFlow::Node taintedNode() { TaintReachFlow::flowTo(result) } |
There was a problem hiding this comment.
Possibly something like this:
| DataFlow::Node taintedNode() { TaintReachFlow::flowTo(result) } | |
| DataFlow::Node taintedNode() { | |
| exists(TaintReachFlow::Stages::Stage2::Graph::Public::PathNode sink | | |
| sink.isSink() and result = sink.getNode() | |
| ) | |
| } |
Adds taint reach stats.
I've tested locally but there are still issues with getting DCA to show the numbers.