C++: Mark the write to fprintf's 0'th argument as partial#20242
C++: Mark the write to fprintf's 0'th argument as partial#20242MathiasVP merged 3 commits intogithub:mainfrom
fprintf's 0'th argument as partial#20242Conversation
fprintf's 0'th argument as a partial writefprintf's 0'th argument as patial
There was a problem hiding this comment.
Pull Request Overview
This PR improves taint tracking for formatting functions like fprintf by marking writes to their FILE stream parameter as partial writes. This ensures that taint flows through the stream parameter correctly when it's used as both input and output.
- Adds a new test case demonstrating taint flow through
fprintf's FILE parameter - Implements
isPartialWritemethod in theFormattingFunctionclass to mark stream outputs as partial - Updates the SSA implementation to properly handle the partial flow logic
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| FormattingFunction.qll | Implements isPartialWrite to mark stream parameters as partial writes |
| SsaImpl.qll | Fixes variable reference in modeledFlowBarrier predicate |
| taint.cpp | Adds test case for taint flow through fprintf FILE parameter |
| taint.ql | Adds support for indirect_sink in test queries |
| *.expected | Updated test expectations reflecting the new taint flow behavior |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
fprintf's 0'th argument as patialfprintf's 0'th argument as partial
|
|
||
| final override predicate isPartialWrite(FunctionOutput output) { | ||
| exists(int outputParameterIndex | | ||
| output.isParameterDeref(outputParameterIndex) and |
There was a problem hiding this comment.
Given where you added this, this presumably also leads more flow on the first argument of functions like sprintf. Is that correct? If so, is that what we want?
There was a problem hiding this comment.
The this.getOutputParameterIndex(true) conjunct only holds for formatting functions which specify that the output parameter is a stream, and sprintf implements this predicate with isStream = false. So this shouldn't affect sprintf.
There was a problem hiding this comment.
Thanks for the clarification!
This fixes a FN that @jketema noticed on the Coding Standards queries.
The new results in
cpp/non-constant-formatandcpp/tainted-format-stringhighlight some really weird code:We get new results on
stderr. It took me a second to spot the issue, but it looks likedieexpands to:Which seems very bad! So this looks like TPs 🎉