C++: Stub replacement for security.TaintTracking #1912
Conversation
This commit adds a `semmle.code.cpp.ir.dataflow.DefaultTaintTracking` library that's API-compatible with the `semmle.code.cpp.security.TaintTracking` library. The new library is implemented on top of the IR data flow library. The idea is to evolve this library until it can replace `semmle.code.cpp.security.TaintTracking` without decreasing our SAMATE score. Then we'll have the IR in production use, and we will have one less taint-tracking library in production.
|
I've added this library without tests. As we talked about in yesterday's meeting, it's not obvious how to add tests without duplicating both queries and test results. At this point I don't think it's important to track test changes very closely since there are some obvious big-ticket items missing: flow through |
| * A variable that has any kind of upper-bound check anywhere in the program | ||
| */ | ||
| private predicate hasUpperBoundsCheck(Variable var) { | ||
| exists(BinaryOperation oper, VariableAccess access | |
There was a problem hiding this comment.
Use RelationalOperation rather than checking getOperator().
There was a problem hiding this comment.
Done. Also in the security.TaintTracking library from which I copied this predicate.
| oper.getOperator() = ">" or | ||
| oper.getOperator() = ">=" | ||
| ) and | ||
| oper.getLeftOperand() = access and |
There was a problem hiding this comment.
I think this is using left vs right operand as a proxy for whether the value is being checked or is the bound. Would being the lesser or greater operand be a better proxy? (testing could be a TODO)
There was a problem hiding this comment.
As you note below, checking the whole variable rather than one variable access is itself a proxy. I've added a todo discussing it.
| override predicate isBarrier(DataFlow::Node node) { | ||
| exists(Variable checkedVar | | ||
| accessesVariable(node.asInstruction(), checkedVar) and | ||
| hasUpperBoundsCheck(checkedVar) |
There was a problem hiding this comment.
In the long run, I'd like to use guards or range analysis here. Should that be a TODO in here or just a ticket on Jira?
There was a problem hiding this comment.
I've added that as a todo on hasUpperBoundsCheck.
| // of `returnArgument` to the `interfaces.Taint` library. | ||
| // | ||
| // TODO: Flow from input argument to output argument of known functions: Port | ||
| // missing parts of `copyValueBetweenArguments` to the `interfaces.Taint` |
There was a problem hiding this comment.
Some of these should go in interfaces.DataFlow instead
| exists(DefaultTaintTrackingCfg cfg, DataFlow::Node sink | | ||
| cfg.hasFlow(DataFlow::exprNode(source), sink) | ||
| | | ||
| sink.asExpr().getConversion*() = tainted |
There was a problem hiding this comment.
All the conversion instructions are UnaryInstructions and therefore covered by the clause in instructionTaintStep. We should be able to omit the getConversion*().
There was a problem hiding this comment.
I think there are some edge cases where it makes a difference. I've added a todo.
This commit adds a
semmle.code.cpp.ir.dataflow.DefaultTaintTrackinglibrary that's API-compatible with thesemmle.code.cpp.security.TaintTrackinglibrary. The new library is implemented on top of the IR data flow library.The idea is to evolve this library until it can replace
semmle.code.cpp.security.TaintTrackingwithout decreasing our SAMATE score. Then we'll have the IR in production use, and we will have one less taint-tracking library in production.