C++: Use TaintTracking::Configuration in TaintedAllocationSize #3519
Conversation
This brings the temporary conflation of pointers and objects at function arguments and parameters into TaintTrackingUtil, since we don't have precise flow for indirections yet.
This migrates TaintedAllocationSize to use the standard TaintTracking::Configuration mechanism rather than DefaultTaintTracking. This includes a new SanitizerGuard instance, but it has some false positives where a variable is bounds checked and reassigned if out of bounds.
This adds a copy of the test from before github#3511. The prior commit fixed the false positive in the modified version but not the original.
There was a problem hiding this comment.
Individual changes LGTM, but the three new false positives in the TaintedAllocationSize test are a problem (a setback for https://github.com/github/codeql-c-analysis-team/issues/80). Do you know what's causing them, and how easily they might be addressed?
|
Yes - they all have a fairly similar pattern: The issue is that the |
|
The security / defaulttainttracking library casts a wide 'benefit of doubt' around comparisons, by considering any read of a variable |
There was a problem hiding this comment.
Changes not already discussed LGTM, and I may have come across negatively in previous comments when I didn't intend to. This is a big and much needed improvement to TaintedAllocationSize. I like the splitting of taint sources into local and remote, I think that design will be helpful in future.
| @@ -76,7 +76,7 @@ void processData2(char *start, char *end) | |||
| { | |||
| char *copy; | |||
|
|
|||
| copy = new char[end - start]; // GOOD | |||
| copy = new char[end - start]; // GOOD [FALSE POSITIVE] | |||
There was a problem hiding this comment.
I think this new result is acceptable. I'm not entirely sure why the old version of the query didn't flag it, and I suspect it was a fluke.
There was a problem hiding this comment.
It was prevented by the predictability barrier in DefaultTaintTracking, which I did not carry over to this version. The taint source is actually the buffer filled by fread, not its return value. I think when #3123 is merged and we can remove pointer/object conflation on arguments, this should be fixed.
|
We've had a lot of FP reports on this particular query, so we can't ignore problems with comparison guards.
A major motivation for porting this query is to ensure that our libraries offer what they need for typical queries. Let's discuss in the next team meeting how we can make that happen. |
C++: Restore hasUpperBoundsCheck.
|
My concerns have been addressed. 👍 |
…ic-conf make submodule update a fast-forward
rdmarsh2
added
the
depends on internal PR
|
Internal PR: https://git.semmle.com/Semmle/code/pull/37036 |
|
Those changes from the CPP-Differences job don't look good. The new results are all false positives from added flow through pointer differences, and there's about a 1.5% slowdown to the overall suite because of this query, plus some other slowdowns that might be related |
This migrates TaintedAllocationSize to use the standard
TaintTracking::Configurationmechanism rather thanDefaultTaintTracking. It includes a new SanitizerGuard instance for bounds checks, but it has some false positives where a variable is bounds checked and reassigned if out of bounds.