C++ Improvements to TaintedAllocationSize.ql #3241
Conversation
| */ | ||
| predicate allocExprOrIndirect(Expr e, Expr size, Expr mid, AllocationExpr alloc) { | ||
| isAllocationExpr(alloc) and | ||
| size = alloc.getSizeExpr() and |
There was a problem hiding this comment.
Can we reduce the false postive rate even more by excluding the allocations with a size we can prove will never overflow (e.g., with SimpleRangeAnalysis.qll?)
I tried doing a (very naive) comparison test between this new version of the query with and without the use of range analysis, and it does exclude some results: https://lgtm.com/query/7744692820054815595/
I haven't verified whether those results are meaningful yet, though.
There was a problem hiding this comment.
Thanks for this suggestion, I'll come back to it after the milestone.
| ) { | ||
| isUserInput(source, taintCause) and | ||
| exists(Expr tainted | | ||
| taintedChild(e, tainted) and | ||
| allocExprOrIndirectRoot(alloc, tainted, _) and |
There was a problem hiding this comment.
I think this has false negatives if there's multiple layers of wrappers that are used by non-library code - e.g. if there's a my_calloc that calls my_malloc that calls malloc, tainted sizes in direct invocations of my_malloc could be missed.
There was a problem hiding this comment.
This commit has been removed from the PR, but I'll come back to it at some point. I think you're right, and it certainly deserves a test case.
|
I'd like to get this query fixed in 1.24, but I agree that this PR is too risky for 1.24. If you drop 90e77d9, I'd be fine with merging the rest. I think it satisfies the hotfix criteria. Since we now have path explanations for this query, the lack of allocation wrapper support is surely not as bad as it used to be. In any case, it's not a regression since 1.23, right? |
Done (via a rebase), and updated the change note.
I agree, with the slight qualification that I haven't seen how clearly the path explanations are actually presented in LGTM right now. |
|
Added the corresponding test changes in the code repo: https://git.semmle.com/Semmle/code/pull/36694 (these two PRs should be merged at the same time). |
geoffw0
added
the
depends on internal PR
Improvements to
TaintedAllocationSize.ql:x * sizeof(y)expressions (a clever trick that worked remarkably well, but we have better models and taint flow so we no longer need it; just leads to duplicate and less clear results now)mallocwrappers (i.e. at the place where the wrapper's called rather than inside it)This was supposed to be in 1.24 as there are multiple user requests for improvements, however I think it's too complex to rush in that quickly now. 1.24 already contains #3062 and improvements to models and taint, so hopefully progress will be apparent.
Before this is merged I need to make a PR for the code repo (some tests are affected), add more tests of the edge cases, and most likely move the change note from 1.24 to 1.25. But I'd like to solicit feedback and iterate before I get caught up in the submodule update war.