From 63311739a517dd74c0be3956764f2e5566572c83 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 23 Aug 2019 09:24:09 +0200 Subject: [PATCH 1/4] C++: Add localExprFlow and localExprTaint This is for ODASA-8053. --- change-notes/1.23/analysis-cpp.md | 3 +++ .../semmle/code/cpp/dataflow/internal/DataFlowUtil.qll | 6 ++++++ .../code/cpp/dataflow/internal/TaintTrackingUtil.qll | 8 ++++++++ .../semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 6 ++++++ 4 files changed, 23 insertions(+) diff --git a/change-notes/1.23/analysis-cpp.md b/change-notes/1.23/analysis-cpp.md index e883f885f0b6..62b59a7b8492 100644 --- a/change-notes/1.23/analysis-cpp.md +++ b/change-notes/1.23/analysis-cpp.md @@ -25,3 +25,6 @@ The following changes in version 1.23 affect C/C++ analysis in all applications. picture of the partial flow paths from a given source. The feature is disabled by default and can be enabled for individual configurations by overriding `int explorationLimit()`. +* There is now a `DataFlow::localExprFlow` predicate and a + `TaintTracking::localExprTaint` predicate to make it easy to use the most + common case of local data flow and taint: from one `Expr` to another. diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll index 13f5db09b6c8..9b57e0dbaa12 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll @@ -490,6 +490,12 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) { */ predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) } +/** + * Holds if data can flow from `e1` to `e2` in zero or more + * local (intra-procedural) steps. + */ +predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)) } + /** * Holds if the initial value of `v`, if it is a source, flows to `var`. */ diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/TaintTrackingUtil.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/TaintTrackingUtil.qll index e379157a6031..93910382cee9 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/TaintTrackingUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/TaintTrackingUtil.qll @@ -80,6 +80,14 @@ predicate localTaint(DataFlow::Node source, DataFlow::Node sink) { localTaintStep*(source, sink) } +/** + * Holds if taint can flow from `e1` to `e2` in zero or more + * local (intra-procedural) steps. + */ +predicate localExprTaint(Expr e1, Expr e2) { + localTaint(DataFlow::exprNode(e1), DataFlow::exprNode(e2)) +} + /** * Holds if we do not propagate taint from `fromExpr` to `toExpr` * even though `toExpr` is the AST parent of `fromExpr`. diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index e9715925432a..1db97a49c03b 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -178,6 +178,12 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) { */ predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) } +/** + * Holds if data can flow from `e1` to `e2` in zero or more + * local (intra-procedural) steps. + */ +predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)) } + /** * A guard that validates some expression. * From f1d7fde49d4f891bcb974ca9848d01a615152d05 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 23 Aug 2019 09:33:41 +0200 Subject: [PATCH 2/4] C++: Use localExprFlow in existing queries This shortens the queries a bit and ensures test coverage of the new predicate. --- cpp/ql/src/Critical/NewDelete.qll | 2 +- cpp/ql/src/Critical/OverflowStatic.ql | 2 +- .../src/Likely Bugs/Conversion/LossyFunctionResultCast.ql | 2 +- .../src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql | 2 +- .../src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql | 8 +++----- 5 files changed, 7 insertions(+), 9 deletions(-) diff --git a/cpp/ql/src/Critical/NewDelete.qll b/cpp/ql/src/Critical/NewDelete.qll index 5cd7ef646d11..39b265556b02 100644 --- a/cpp/ql/src/Critical/NewDelete.qll +++ b/cpp/ql/src/Critical/NewDelete.qll @@ -47,7 +47,7 @@ predicate allocExprOrIndirect(Expr alloc, string kind) { or exists(Expr e | allocExprOrIndirect(e, kind) and - DataFlow::localFlow(DataFlow::exprNode(e), DataFlow::exprNode(rtn.getExpr())) + DataFlow::localExprFlow(e, rtn.getExpr()) ) ) ) diff --git a/cpp/ql/src/Critical/OverflowStatic.ql b/cpp/ql/src/Critical/OverflowStatic.ql index a0917c470244..bd76173b6f49 100644 --- a/cpp/ql/src/Critical/OverflowStatic.ql +++ b/cpp/ql/src/Critical/OverflowStatic.ql @@ -95,7 +95,7 @@ class CallWithBufferSize extends FunctionCall { int statedSizeValue() { exists(Expr statedSizeSrc | - DataFlow::localFlow(DataFlow::exprNode(statedSizeSrc), DataFlow::exprNode(statedSizeExpr())) and + DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr()) and result = statedSizeSrc.getValue().toInt() ) } diff --git a/cpp/ql/src/Likely Bugs/Conversion/LossyFunctionResultCast.ql b/cpp/ql/src/Likely Bugs/Conversion/LossyFunctionResultCast.ql index 5a8c3ee84bcc..0b844bdcbf5c 100644 --- a/cpp/ql/src/Likely Bugs/Conversion/LossyFunctionResultCast.ql +++ b/cpp/ql/src/Likely Bugs/Conversion/LossyFunctionResultCast.ql @@ -55,7 +55,7 @@ predicate whiteListWrapped(FunctionCall fc) { whitelistPow(fc) or exists(Expr e, ReturnStmt rs | whiteListWrapped(e) and - DataFlow::localFlow(DataFlow::exprNode(e), DataFlow::exprNode(rs.getExpr())) and + DataFlow::localExprFlow(e, rs.getExpr()) and fc.getTarget() = rs.getEnclosingFunction() ) } diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql index e41b9f7113dc..ddc74cc62057 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql @@ -23,7 +23,7 @@ predicate isBoolean(Expr e1) { } predicate isStringCopyCastedAsBoolean(FunctionCall func, Expr expr1, string msg) { - DataFlow::localFlow(DataFlow::exprNode(func), DataFlow::exprNode(expr1)) and + DataFlow::localExprFlow(func, expr1) and isBoolean(expr1.getConversion*()) and func.getTarget() instanceof StrcpyFunction and msg = "Return value of " + func.getTarget().getName() + " used as a Boolean." diff --git a/cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql b/cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql index e45be7640bf1..d5ab739f3484 100644 --- a/cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql +++ b/cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql @@ -36,12 +36,10 @@ class MallocCall extends FunctionCall predicate terminationProblem(MallocCall malloc, string msg) { malloc.getAllocatedSize() instanceof StrlenCall and - not exists(DataFlow::Node def, DataFlow::Node use, FunctionCall fc, MemcpyFunction memcpy, int ix | - DataFlow::localFlow(def, use) and - def.asExpr() = malloc and + not exists(FunctionCall fc, MemcpyFunction memcpy, int ix | + DataFlow::localExprFlow(malloc, fc.getArgument(ix)) and fc.getTarget() = memcpy and - memcpy.hasArrayOutput(ix) and - use.asExpr() = fc.getArgument(ix) + memcpy.hasArrayOutput(ix) ) and msg = "This allocation does not include space to null-terminate the string." } From 26c81eaae92d6cffe0935cea83f9d5d8b31396ac Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 2 Sep 2019 09:42:44 +0200 Subject: [PATCH 3/4] C++: Mention localExpr{Flow,Taint} in module QLDoc --- cpp/ql/src/semmle/code/cpp/dataflow/DataFlow.qll | 6 ++++-- cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll | 7 +++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/DataFlow.qll b/cpp/ql/src/semmle/code/cpp/dataflow/DataFlow.qll index bfb93f77bfa7..3425e9b99016 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/DataFlow.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/DataFlow.qll @@ -11,8 +11,10 @@ * * To use global (interprocedural) data flow, extend the class * `DataFlow::Configuration` as documented on that class. To use local - * (intraprocedural) data flow, invoke `DataFlow::localFlow` or - * `DataFlow::LocalFlowStep` with arguments of type `DataFlow::Node`. + * (intraprocedural) data flow between expressions, call + * `DataFlow::localExprFlow`. For more general cases of local data flow, call + * `DataFlow::localFlow` or `DataFlow::localFlowStep` with arguments of type + * `DataFlow::Node`. */ import cpp diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll index 92cd085927a5..a40579adde16 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll @@ -6,6 +6,13 @@ * the information from the source is preserved at the sink. For example, taint * propagates from `x` to `x + 100`, but it does not propagate from `x` to `x > * 100` since we consider a single bit of information to be too little. + * + * To use global (interprocedural) taint tracking, extend the class + * `TaintTracking::Configuration` as documented on that class. To use local + * (intraprocedural) taint tracking between expressions, call + * `TaintTracking::localExprTaint`. For more general cases of local taint + * tracking, call `TaintTracking::localTaint` or + * `TaintTracking::localTaintStep` with arguments of type `DataFlow::Node`. */ import semmle.code.cpp.dataflow.DataFlow import semmle.code.cpp.dataflow.DataFlow2 From 8ee87fd9fcf7f343ca6eba9f32e15c96d2efa61f Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 2 Sep 2019 09:43:52 +0200 Subject: [PATCH 4/4] C++: Make TaintTracking2 QLDoc more like DataFlow2 --- .../semmle/code/cpp/dataflow/TaintTracking2.qll | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking2.qll b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking2.qll index f67a0e711002..bde6c3633471 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking2.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking2.qll @@ -1,11 +1,14 @@ /** - * Provides classes for performing local (intra-procedural) and - * global (inter-procedural) taint-tracking analyses. + * Provides a `TaintTracking2` module, which is a copy of the `TaintTracking` + * module. Use this class when data-flow configurations or taint-tracking + * configurations must depend on each other. Two classes extending + * `DataFlow::Configuration` should never depend on each other, but one of them + * should instead depend on a `DataFlow2::Configuration`, a + * `DataFlow3::Configuration`, or a `DataFlow4::Configuration`. The + * `TaintTracking::Configuration` class extends `DataFlow::Configuration`, and + * `TaintTracking2::Configuration` extends `DataFlow2::Configuration`. * - * We define _taint propagation_ informally to mean that a substantial part of - * the information from the source is preserved at the sink. For example, taint - * propagates from `x` to `x + 100`, but it does not propagate from `x` to `x > - * 100` since we consider a single bit of information to be too little. + * See `semmle.code.cpp.dataflow.TaintTracking` for the full documentation. */ module TaintTracking2 { import semmle.code.cpp.dataflow.internal.tainttracking2.TaintTrackingImpl