Skip to content

C++: Rewrite cpp/user-controlled-null-termination-tainted away from DefaultTaintTracking #14881

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

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 22, 2023

This query logic in this query was kinda odd. The query starts by tracking flow from a subset of flow sources (the ones that can leave the variable with a non-zero terminated string) to a specific kind of VariableAccess (places where we know a zero-terminated string is expceted). That's all well and good. The problem is that, to weed out false positives, the query adds barrier logic to the query by reinventing a query-specific dataflow library. This dataflow library is implemented as a less-than-ideal transitive closure over the old exprDefinition predicate from DefinitionsAndUses library.

This PR rewrites the query to be a standard taint-tracking query. This is not totally what the old query was doing (since it used taint-tracking to track the flow, but used dataflow to implement a barrier), but I think this is within the spirit of the query.

There are a lot of lost results on MRVA. Most of the ones I've seen are due to fixed pointer conflation. There are a few new results (about 200 out of 23000 the result changes), and the ones I've looked at look like TP which was blocked by the queries incorrect handling of barriers.

I don't think we should spend too much time on this query as it's not part of any suite (it has no precision).

@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Nov 22, 2023
@github-actions github-actions bot added the C++ label Nov 22, 2023
@MathiasVP
Copy link
Contributor Author

I've looked through quite a lot of the lost results on vim (which went from having 2457 results to 0 results), and they all seem like fixed FPs 🎉. The previous FPs existed because the incorrect handling of barriers.

@MathiasVP MathiasVP marked this pull request as ready for review November 23, 2023 13:38
@MathiasVP MathiasVP requested a review from a team as a code owner November 23, 2023 13:38
MathiasVP and others added 2 commits November 23, 2023 14:10
Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
@MathiasVP MathiasVP merged commit 149fb7b into github:main Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants