-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C++: Mark the write to fprintf
's 0'th argument as partial
#20242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fprintf
's 0'th argument as a partial writefprintf
's 0'th argument as patial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
isPartialWrite
method in theFormattingFunction
class 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This fixes a FN that @jketema noticed on the Coding Standards queries.
The new results in
cpp/non-constant-format
andcpp/tainted-format-string
highlight some really weird code:We get new results on
stderr
. It took me a second to spot the issue, but it looks likedie
expands to:Which seems very bad! So this looks like TPs 🎉