Skip to content

C++: Support access paths for sources and sinks - #22374

Merged
MathiasVP merged 20 commits into
github:mainfrom
MathiasVP:cpp-access-paths-for-sources-and-sinks-3
Aug 24, 2026
Merged

C++: Support access paths for sources and sinks#22374
MathiasVP merged 20 commits into
github:mainfrom
MathiasVP:cpp-access-paths-for-sources-and-sinks-3

Conversation

@MathiasVP

@MathiasVPMathiasVP commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Now that #22145 has been merged we can finally add MaD support for access paths at sources and sinks.

Commit-by-commit review recommended.

There's a small change to the toString on sources (and sinks) defined in MaD. Previously, it would be the toString of 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 toString changes). I'll do that once this PR has been reviewed to avoid unnecessary submodule bumps.

@MathiasVP
MathiasVPforce-pushed the cpp-access-paths-for-sources-and-sinks-3 branch from e164ec1 to 497d9b6CompareAugust 19, 2026 10:24
@MathiasVP
MathiasVPforce-pushed the cpp-access-paths-for-sources-and-sinks-3 branch from 497d9b6 to 5077b54CompareAugust 19, 2026 14:39
@MathiasVP
MathiasVP marked this pull request as ready for review August 19, 2026 15:10
@MathiasVP
MathiasVP requested a review from a team as a code ownerAugust 19, 2026 15:10
CopilotAI balanced review requested due to automatic review settings August 19, 2026 15:10

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
FileDescription
cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/ExposedSystemData.expectedUpdates expected source/sink labels.
cpp/ql/test/query-tests/Security/CWE/CWE-089/SqlTainted/SqlTainted.expectedUpdates SQL alert locations and labels.
cpp/ql/test/library-tests/dataflow/models-as-data/testModels.expectedRecords new flow-summary nodes.
cpp/ql/test/library-tests/dataflow/external-models/test.cppAdds source access-path test cases.
cpp/ql/test/library-tests/dataflow/external-models/sources.expectedUpdates expected modeled sources.
cpp/ql/test/library-tests/dataflow/external-models/sinks.expectedUpdates expected modeled sinks.
cpp/ql/test/library-tests/dataflow/external-models/flow.ext.ymlAdds field and callback source models.
cpp/ql/test/library-tests/dataflow/external-models/flow.expectedRecords updated flow graphs.
cpp/ql/src/Security/CWE/CWE-089/SqlTainted.qlSupports flow-summary sink nodes.
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qllRefactors argument and position abstractions.
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowNodes.qllExposes and labels source/sink summary nodes.
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowImplConsistency.qllExcludes summary nodes from consistency checks.
cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qllImplements C++ source/sink access-path reporting.
cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qllRoutes 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, because Cpp::Parameter only represents zero-based explicit parameters. However, decodePosition accepts Parameter[-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 implicit this parameter instead of routing it through Cpp::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

Comment threadcpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll Outdated
MathiasVPand others added 2 commits August 19, 2026 16:23
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@MathiasVP
MathiasVP marked this pull request as draft August 19, 2026 15:30
@MathiasVP
MathiasVPforce-pushed the cpp-access-paths-for-sources-and-sinks-3 branch from fd24dab to f0a4145CompareAugust 20, 2026 15:57
@MathiasVP
MathiasVP marked this pull request as ready for review August 20, 2026 19:48

@hvitvedhvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great, a couple of minor comments.

Comment threadcpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll Outdated
Comment threadcpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll Outdated
Comment threadcpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qll Outdated
Comment on lines +226 to +227
p.isParameterOf(e.getEnclosingCallable(), pos) and
result = p

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would think e = p.getParameter() to work.

@MathiasVPMathiasVPAug 21, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We still need to relate pos (i.e., the parameter position) and the parameter, though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What I meant was

sc = Impl::Private::SummaryComponent::parameter(_) and
e = p.getParameter()

Because of the bindingset, this should be OK.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, I see what you mean. I've fixed this in 6d91ebe (it requires a bit more code to handle the indirection of the parameter - which then required some join order gymnastics).

Comment threadcpp/ql/src/Security/CWE/CWE-089/SqlTainted.ql
isSink(sinkNode.getNode(), extraText) and
taintSource = sourceNode.getNode()
select taintedArg, sourceNode, sinkNode,
select sinkNode.getNode(), sourceNode, sinkNode,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this potentially give rise to new results?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@jketema

jketema commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

I'm happy with the changes here. I'll leave the actual approval to @hvitved.

@MathiasVPMathiasVP added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 21, 2026
@MathiasVP

Copy link
Copy Markdown
ContributorAuthor

The CI failures here are from the internal repo. The internal PR is green 🟢!

@MathiasVP

Copy link
Copy Markdown
ContributorAuthor
  1. The integration test is unrelated
  2. The language tests pass on the internal PR

Merging!

@MathiasVP
MathiasVP merged commit 782f1fc into github:mainAug 24, 2026
17 of 19 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++depends on internal PRThis PR should only be merged in sync with an internal Semmle PRdocumentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MathiasVP@jketema@hvitved@github-advanced-security