CPP: Add (partial) dataflow to OverflowStatic.ql#524
Conversation
|
@geoffw0: fully agree with your plan, could you please send me a ping as soon as you've opened a Jira issue? |
|
I've added some more pointers for further discussion in CPP-299. It'd be great to hear @geoffw0's thoughts on that issue before this PR gets merged. |
| } | ||
| int statedSizeValue() { | ||
| exists(Expr statedSizeSrc | | ||
| DataFlow::localFlowStep*(DataFlow::exprNode(statedSizeSrc), DataFlow::exprNode(statedSizeExpr())) and |
There was a problem hiding this comment.
You can write localFlow instead of localFlowStep*.
| error = call.statedSize() and | ||
| statedSize = call.statedSizeValue() and | ||
| statedSize > bufsize and | ||
| error = call.statedSizeExpr() and |
There was a problem hiding this comment.
Now that you've introduced data flow, we need to decide what to do when multiple values may flow to error. With this PR we'll start seeing false positives in code like
char buf10[10], buf20[20];
if (b) { size = 10; } else { size = 20; }
if (b) {
memcpy(buf10, src, size); // false positive
} else {
memcpy(buf20, src, size);
}
You can be more conservative, and still address CPP-299, by replacing call.statedSizeValue() with min(call.statedSizeValue()). That won't address https://github.com/MosheWagner/VulnC/blob/f787c51fbe09d2fb88403d3b01bca7709fd7ed9e/src/bad_read.c#L158-L169, but I would prefer to err on the side of false negatives until we run a CPP-Differences job and see that such FPs are rare in practice.
There was a problem hiding this comment.
OK, makes sense to be conservative at this time. Done.
|
Jira issue created here: https://jira.semmle.com/browse/CPP-304 |
|
Rebased to fix merge conflict (in change note). |
3 participants
Add a little bit of dataflow to the
OverflowStatic.qlquery, just enough to catch the cases described in https://jira.semmle.com/browse/CPP-299 (I was wrong when I saidBadlyBoundedWrite.qlshould catch these examples - because they are reads, and reads are plainly out of the scope of that query).In the long run (for 1.20 or 1.21?) I'd like to go a lot further:
OverflowStatic.qlinto three parts, one for each of the already distinct modes evident in the QL, so that they can be individually turned on/off/tagged/worked on.wrongBufferSizepart is largely a weaker version of theBadlyBoundedWrite.qlquery except that it looks at reads in addition to writes. I think a better design would be to create a newBadlyBoundedRead.qlquery such thatBadlyBoundedRead+BadlyBoundedWritecover everything we already cover (and more).overflowOffsetInLoopandoutOfBounds), either merge them into an existing query or make whatever improvements seem appropriate.If there's agreement I'll create a JIRA issue for this plan.