C++: Avoid partial chi flow to struct/class#3219
Merged
MathiasVP merged 1 commit intoApr 8, 2020
Merged
Conversation
Flow through partial chi-instruction operands was introduced to make definition-by-reference work, but its implementation also allowed all other partial writes to propagate. In particular, tainting a field would taint the whole struct, which in turn led to taint propagating across unrelated fields of a struct. The security test `CWE-134/semmle/argv/argvLocal.c` shows that we also want to propagate taint from an array element to the whole array, and it also seems right to propagate taint from a union member to the whole union.
Contributor
Author
|
Started https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1021/ I'm hoping this will remove the FPs we have on git/git in (at least) |
Merged
Contributor
Author
Contributor
Author
|
The |
jbj
commented
Apr 7, 2020
| t instanceof ArrayType | ||
| or | ||
| // Buffers or unknown size | ||
| t instanceof UnknownType |
Contributor
Author
There was a problem hiding this comment.
For a struct pointer parameter, I think its chi nodes will have UnknownType. How can we find the struct type?
Contributor
Author
|
These are the result differences: I've looked through the path explanations for all three queries on both projects, and I'll claim that all the lost results are FPs due to field conflation, while all the remaining results are TPs (in the sense that the query is working as it's designed to work). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Taint through partial chi-instruction operands was introduced to make definition-by-reference work, but its implementation also allowed all other partial writes to propagate. In particular, tainting a field would taint the whole struct, which in turn led to taint propagating across unrelated fields of a struct.
The security test
CWE-134/semmle/argv/argvLocal.cshows that we also want to propagate taint from an array element to the whole array, and it also seems right to propagate taint from a union member to the whole union.