C++: Support access paths for sources and sinks - #22374
Conversation
e164ec1 to
497d9b6
Compare
497d9b6 to
5077b54
Compare
There was a problem hiding this comment.
Pull request overview
Adds C++ models-as-data support for source and sink access paths in the IR data-flow framework.
Changes:
- Introduces flow-summary reporting nodes for modeled sources and sinks.
- Refactors argument/parameter node handling and consistency checks.
- Updates SQL injection handling, tests, and expected outputs.
Show a summary per file
| File | Description |
|---|---|
cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/ExposedSystemData.expected |
Updates expected source/sink labels. |
cpp/ql/test/query-tests/Security/CWE/CWE-089/SqlTainted/SqlTainted.expected |
Updates SQL alert locations and labels. |
cpp/ql/test/library-tests/dataflow/models-as-data/testModels.expected |
Records new flow-summary nodes. |
cpp/ql/test/library-tests/dataflow/external-models/test.cpp |
Adds source access-path test cases. |
cpp/ql/test/library-tests/dataflow/external-models/sources.expected |
Updates expected modeled sources. |
cpp/ql/test/library-tests/dataflow/external-models/sinks.expected |
Updates expected modeled sinks. |
cpp/ql/test/library-tests/dataflow/external-models/flow.ext.yml |
Adds field and callback source models. |
cpp/ql/test/library-tests/dataflow/external-models/flow.expected |
Records updated flow graphs. |
cpp/ql/src/Security/CWE/CWE-089/SqlTainted.ql |
Supports flow-summary sink nodes. |
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll |
Refactors argument and position abstractions. |
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowNodes.qll |
Exposes and labels source/sink summary nodes. |
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowImplConsistency.qll |
Excludes summary nodes from consistency checks. |
cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qll |
Implements C++ source/sink access-path reporting. |
cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll |
Routes modeled sources and sinks through summary nodes. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll:629
- This cannot resolve index
-1, becauseCpp::Parameteronly represents zero-based explicit parameters. However,decodePositionacceptsParameter[-1]and callback-self positions also use-1, so models rooted at that source parameter and source/sink callback paths targeting a member function's receiver cannot obtain a reporting element and silently disappear. Add an explicit representation/mapping for the implicitthisparameter instead of routing it throughCpp::Parameter.
Parameter getParameter(Function f) {
result.getFunction() = f and
this.getArgumentIndex() = result.getIndex()
}
- Files reviewed: 14/14 changed files
- Comments generated: 4
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
fd24dab to
f0a4145
Compare
hvitved
left a comment
There was a problem hiding this comment.
Looks great, a couple of minor comments.
| p.isParameterOf(e.getEnclosingCallable(), pos) and | ||
| result = p |
There was a problem hiding this comment.
I would think e = p.getParameter() to work.
There was a problem hiding this comment.
We still need to relate pos (i.e., the parameter position) and the parameter, though.
There was a problem hiding this comment.
What I meant was
sc = Impl::Private::SummaryComponent::parameter(_) and
e = p.getParameter()
Because of the bindingset, this should be OK.
| isSink(sinkNode.getNode(), extraText) and | ||
| taintSource = sourceNode.getNode() | ||
| select taintedArg, sourceNode, sinkNode, | ||
| select sinkNode.getNode(), sourceNode, sinkNode, |
There was a problem hiding this comment.
Could this potentially give rise to new results?
There was a problem hiding this comment.
I was actually expecting 0 alert changes from this, but DCA does reveal a single alert change since we picked somehow picked the location of the unconverted expression, and we now pick the location of the converted expression.
But I think that's the only case, as we don't see the location changing in any query tests.
|
I'm happy with the changes here. I'll leave the actual approval to @hvitved. |
|
The CI failures here are from the internal repo. The internal PR is green 🟢! |
Now that #22145 has been merged we can finally add MaD support for access paths at sources and sinks.
Commit-by-commit review recommended.
ParameterNodes andArgumentNodes in dataflow which will make the main commit simpler.There's a small change to the
toStringon sources (and sinks) defined in MaD. Previously, it would be thetoStringof the underlying dataflow node. However, due to the way MaD works it's hard (impossible?) to keep this behavior.I need to update an .expected file in the internal repo as well (because of the
toStringchanges). I'll do that once this PR has been reviewed to avoid unnecessary submodule bumps.