Skip to content

cpp: model Protocol Buffers parse/serialize taint flow - #22448

Open
kumarak wants to merge 4 commits into
github:mainfrom
trail-of-forks:kumarak/cpp-protobuf-flow-models
Open

cpp: model Protocol Buffers parse/serialize taint flow#22448
kumarak wants to merge 4 commits into
github:mainfrom
trail-of-forks:kumarak/cpp-protobuf-flow-models

Conversation

@kumarak

@kumarakkumarak commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Add flow summaries for the protobuf C++ API on google::protobuf::MessageLite (subtypes=true, so Message and all generated messages are covered):

  • ParseFrom*/MergeFrom* (string, array, Cord, istream, zero-copy and coded-stream forms) propagate taint from the encoded input to the message.
  • SerializeTo*/AppendTo* propagate taint from the message to the output buffer or stream; SerializeAs*/AppendTo* to the return value.
  • File-descriptor variants are omitted (the fd is an int, not a buffer).

@kumarak
cpp: model Protocol Buffers parse/serialize taint flow
ed540df
Add flow summaries for the protobuf C++ API on
google::protobuf::MessageLite (subtypes=true, so Message and all
generated messages are covered):
- ParseFrom*/MergeFrom* (string, array, Cord, istream, zero-copy and
coded-stream forms) propagate taint from the encoded input to the
message.
- SerializeTo*/AppendTo* propagate taint from the message to the output
buffer or stream; SerializeAs*/... to the return value.
File-descriptor variants are omitted (the fd is an int, not a buffer).
@kumarak
kumarak requested a review from a team as a code ownerAugust 27, 2026 15:30
CopilotAI balanced review requested due to automatic review settings August 27, 2026 15:30
CopilotAI reviewed Aug 27, 2026
View reviewed changes

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++ taint-flow summaries for Protocol Buffers MessageLite APIs and inherited generated message types.

Changes:

  • Models parse/merge and serialization flows.
  • Adds representative flow tests and expected results.
  • Documents the analysis improvement.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
FileDescription
cpp/ql/lib/ext/Protobuf.model.ymlDefines protobuf flow summaries.
cpp/ql/test/library-tests/dataflow/external-models/protobuf.cppAdds protobuf test fixtures.
cpp/ql/test/library-tests/dataflow/external-models/flow.expectedUpdates flow expectations.
cpp/ql/test/library-tests/dataflow/external-models/steps.expectedUpdates summary-step expectations.
cpp/ql/lib/change-notes/2026-08-27-protobuf-models.mdRecords the new models.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadcpp/ql/lib/ext/Protobuf.model.yml
Comment on lines +25 to +29
- ["google::protobuf", "MessageLite", True, "ParseFromZeroCopyStream", "", "", "Argument[*0]", "Argument[-1]", "taint", "manual"]
- ["google::protobuf", "MessageLite", True, "ParsePartialFromZeroCopyStream", "", "", "Argument[*0]", "Argument[-1]", "taint", "manual"]
- ["google::protobuf", "MessageLite", True, "ParseFromBoundedZeroCopyStream", "", "", "Argument[*0]", "Argument[-1]", "taint", "manual"]
- ["google::protobuf", "MessageLite", True, "ParsePartialFromBoundedZeroCopyStream", "", "", "Argument[*0]", "Argument[-1]", "taint", "manual"]
- ["google::protobuf", "MessageLite", True, "MergeFromBoundedZeroCopyStream", "", "", "Argument[*0]", "Argument[-1]", "taint", "manual"]
jketema
jketema requested changes Aug 28, 2026
View reviewed changes

@jketemajketema 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.

Thanks for this. I made a brief first pass over this, which should hopefully put you on the right path.

Comment threadcpp/ql/lib/ext/Protobuf.model.yml Outdated
Comment threadcpp/ql/test/library-tests/dataflow/external-models/protobuf.cpp Outdated
Comment threadcpp/ql/lib/change-notes/2026-08-27-protobuf-models.md Outdated
jketema
jketema reviewed Aug 31, 2026
View reviewed changes

@jketemajketema 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.

Some further comments. I think the .yml file looks good now. I would still significantly reduce the number of comments, which don't seem to add much.

Comment threadcpp/ql/lib/change-notes/2026-08-27-protobuf-models.md Outdated
Comment threadcpp/ql/lib/ext/Protobuf.model.yml Outdated
Comment threadcpp/ql/lib/ext/Protobuf.model.yml Outdated
Comment threadcpp/ql/lib/ext/Protobuf.model.yml Outdated
Comment threadcpp/ql/lib/ext/Protobuf.model.yml Outdated
Comment threadcpp/ql/test/library-tests/dataflow/external-models/protobuf.cpp Outdated
Comment threadcpp/ql/test/library-tests/dataflow/external-models/protobuf.cpp Outdated
Comment threadcpp/ql/test/library-tests/dataflow/external-models/protobuf.cpp Outdated
@kumarak
test(cpp): add sink-based protobuf model tests and trim comments
49ffde7
Replace the step-coverage function with one sink test per model row
using a template source, declare the Cord overloads of the ToString
methods in the stub, drop the incorrect istream comment from the
fixture, and shorten the model-file and change-note comments per review.
@jketema

jketema commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.ql fails. Otherwise this LGTM. I'll run some more internal testing.

@jketema

jketema commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Internal testing showed nothing out of the ordinary. So if you fix the test, then this can be merged.

jketema
jketema reviewed Sep 4, 2026
View reviewed changes

@jketemajketema 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.

LGTM. Going to fix the broken test afterwards to just get this in. Thanks for the contribution!

@jketema

jketema commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Going to fix the broken test afterwards to just get this in. Thanks for the contribution!

This apparently conflicts with the other PR of you that I just merged, and as don't have permission to push to your branch, I cannot resolve this myself.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jketemajketemajketema approved these changes

Assignees

No one assigned

Labels

C++documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kumarak@jketema