hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-20 10:46:30 +01:00
Code Issues Packages Projects Releases Wiki Activity

C++: Interprocedural indirection taint tracking

Browse Source 
As a temporary workaround in the `DefaultTaintTracking` library, we
funnel flow across calls by conflating pointer and object both at the
caller and the callee.

The three cases in `adjustedSink` were deleted because they are now
covered by the one case for `ReadSideEffectInstruction` in
`instructionTaintStep`.

When enabling `DefaultTaintTracking`, this commit on top of #2736 has
the effect effect of recovering two lost results:

    --- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowDestination.expected
    +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowDestination.expected
    @@ -1,2 +1,4 @@
     | overflowdestination.cpp:30:2:30:8 | call to strncpy | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
     | overflowdestination.cpp:46:2:46:7 | call to memcpy | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
    +| overflowdestination.cpp:53:2:53:7 | call to memcpy | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |
    +| overflowdestination.cpp:64:2:64:7 | call to memcpy | To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size. |

In the internal repo, we recover one lost result. Additionally, there
are two queries that gain an extra source for an existing sink. I'll
classify that as noise. The new results look like this:

    foo(argv); // this `argv` is a new source for the sink in `bar`
    bar(argv); // this `argv` is the existing source for the sink in `bar`
This commit is contained in:
Jonas Jensen
2020-01-31 13:31:34 +01:00
parent 72fddaf5ed
commit 83f807f182
1 changed files with 28 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
View File

@@ -151,6 +151,22 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
// from `a`.
i2.(PointerAddInstruction).getLeft() = i1
or
// Until we have from through indirections across calls, we'll take flow out
// of the parameter and into its indirection.
exists(IRFunction f, Parameter parameter |
initializeParameter(f, parameter, i1) and
initializeIndirection(f, parameter, i2)
)
or
// Until we have flow through indirections across calls, we'll take flow out
// of the indirection and into the argument.
// When we get proper flow through indirections across calls, this code can be
// moved to `adjusedSink` or possibly into the `DataFlow::ExprNode` class.
exists(ReadSideEffectInstruction read |
read.getAnOperand().(SideEffectOperand).getAnyDef() = i1 and
read.getArgumentDef() = i2
)
or
// Flow from argument to return value
i2 =
any(CallInstruction call |
@@ -176,6 +192,18 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
)
}
pragma[noinline]
private predicate initializeIndirection(IRFunction f, Parameter p, InitializeIndirectionInstruction instr) {
instr.getParameter() = p and
instr.getEnclosingIRFunction() = f
}
pragma[noinline]
private predicate initializeParameter(IRFunction f, Parameter p, InitializeParameterInstruction instr) {
instr.getParameter() = p and
instr.getEnclosingIRFunction() = f
}
/**
* Get an instruction that goes into argument `argumentIndex` of `call`. This
* can be either directly or through one pointer indirection.
@@ -273,23 +301,6 @@ private Element adjustedSink(DataFlow::Node sink) {
// For compatibility, send flow into a `NotExpr` even if it's part of a
// short-circuiting condition and thus might get skipped.
result.(NotExpr).getOperand() = sink.asExpr()
or
// For compatibility, send flow from argument read side effects to their
// corresponding argument expression
exists(IndirectReadSideEffectInstruction read |
read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
read.getArgumentDef().getUnconvertedResultExpression() = result
)
or
exists(BufferReadSideEffectInstruction read |
read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
read.getArgumentDef().getUnconvertedResultExpression() = result
)
or
exists(SizedBufferReadSideEffectInstruction read |
read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
read.getArgumentDef().getUnconvertedResultExpression() = result
)
}
predicate tainted(Expr source, Element tainted) {