From 262f724235f2aaaf7c61b5b1636088040274f987 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Mon, 22 Apr 2019 10:17:43 -0700 Subject: [PATCH 1/4] C++: add taint edges to DefinitionByReferenceNode --- .../code/cpp/dataflow/TaintTracking.qll | 38 ++++++++++++++++++- .../dataflow/taint-tests/taint.expected | 1 + 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll index 2d8181be945..885dce28148 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll @@ -9,6 +9,8 @@ */ import semmle.code.cpp.dataflow.DataFlow import semmle.code.cpp.dataflow.DataFlow2 +import semmle.code.cpp.models.interfaces.DataFlow +import semmle.code.cpp.models.interfaces.Taint module TaintTracking { @@ -187,6 +189,9 @@ module TaintTracking { exprFrom.(PostfixCrementOperation) ) ) + or + // Taint can flow through modeled functions + exprToDefinitionByReferenceStep(nodeFrom.asExpr(), nodeTo.asDefiningArgument()) } /** @@ -226,4 +231,35 @@ module TaintTracking { e instanceof AlignofOperator } -} \ No newline at end of file + private predicate exprToDefinitionByReferenceStep(Expr exprIn, Expr argOut) { + exists(DataFlowFunction f, Call call, FunctionOutput outModel, int argOutIndex | + call.getTarget() = f and + argOut = call.getArgument(argOutIndex) and + outModel.isOutParameterPointer(argOutIndex) and + exists(int argInIndex, FunctionInput inModel | + f.hasDataFlow(inModel, outModel) + | + inModel.isInParameterPointer(argInIndex) and + exprIn = call.getArgument(argInIndex) + ) + ) + or + exists(TaintFunction f, Call call, FunctionOutput outModel, int argOutIndex | + call.getTarget() = f and + argOut = call.getArgument(argOutIndex) and + outModel.isOutParameterPointer(argOutIndex) and + exists(int argInIndex, FunctionInput inModel | + f.hasTaintFlow(inModel, outModel) + | + inModel.isInParameterPointer(argInIndex) and + exprIn = call.getArgument(argInIndex) + or + inModel.isInParameterPointer(argInIndex) and + call.passesByReference(argInIndex, exprIn) + or + inModel.isInParameter(argInIndex) and + exprIn = call.getArgument(argInIndex) + ) + ) + } +} diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected index 5b3972062c9..d9743bfd536 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected @@ -10,4 +10,5 @@ | taint.cpp:151:7:151:12 | call to select | taint.cpp:151:20:151:25 | call to source | | taint.cpp:167:8:167:13 | call to source | taint.cpp:167:8:167:13 | call to source | | taint.cpp:168:8:168:14 | tainted | taint.cpp:164:19:164:24 | call to source | +| taint.cpp:173:8:173:13 | buffer | taint.cpp:164:19:164:24 | call to source | | taint.cpp:181:8:181:9 | * ... | taint.cpp:185:11:185:16 | call to source | From 34f865397938c85c68fe6466fe56b64b53bdf219 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Mon, 22 Apr 2019 10:46:36 -0700 Subject: [PATCH 2/4] C++: change note for taint def-by-ref --- change-notes/1.21/analysis-cpp.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/change-notes/1.21/analysis-cpp.md b/change-notes/1.21/analysis-cpp.md index 8372087a915..b92fba11578 100644 --- a/change-notes/1.21/analysis-cpp.md +++ b/change-notes/1.21/analysis-cpp.md @@ -24,3 +24,6 @@ | Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | Fewer false positive results | Non-standard uses of %L are now understood. | ## Changes to QL libraries +- Additional support for definition by reference has been added to the `semmle.code.cpp.dataflow.TaintTracking` library. + - The taint tracking library now includes taint-specific edges for functions modeled in `semmle.code.cpp.models.interfaces.DataFlow`. + - The taint tracking library adds flow through library functions that are modeled in `semmle.code.cpp.models.interfaces.Taint`. Queries can add subclasses of `TaintFunction` to specify additional flow. From 919f5c616fe53805b191e24d9b9e2087a8f9f828 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Tue, 23 Apr 2019 11:17:18 -0700 Subject: [PATCH 3/4] C++: comment and test for taint flow via memcpy --- .../src/semmle/code/cpp/dataflow/TaintTracking.qll | 2 ++ .../dataflow/taint-tests/localTaint.expected | 12 ++++++++++++ .../library-tests/dataflow/taint-tests/taint.cpp | 10 +++++++++- .../dataflow/taint-tests/taint.expected | 2 ++ .../dataflow/taint-tests/test_diff.expected | 3 +++ 5 files changed, 28 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll index 885dce28148..baec8836aed 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll @@ -239,6 +239,8 @@ module TaintTracking { exists(int argInIndex, FunctionInput inModel | f.hasDataFlow(inModel, outModel) | + // Taint flows from a pointer to a dereference, which DataFlow does not handle + // memcpy(&dest_var, tainted_ptr, len) inModel.isInParameterPointer(argInIndex) and exprIn = call.getArgument(argInIndex) ) diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index 6cf3e7bda22..7dd0cec2b86 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -137,15 +137,27 @@ | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:171:8:171:13 | buffer | | | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:172:10:172:15 | buffer | | | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | +| taint.cpp:170:18:170:26 | Hello, | taint.cpp:170:10:170:15 | ref arg buffer | TAINT | | taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:171:8:171:13 | buffer | | | taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:172:10:172:15 | buffer | | | taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:3:172:8 | call to strcat | | +| taint.cpp:172:10:172:15 | buffer | taint.cpp:172:10:172:15 | ref arg buffer | TAINT | | taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:172:10:172:15 | buffer | | | taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | +| taint.cpp:172:18:172:24 | tainted | taint.cpp:172:10:172:15 | ref arg buffer | TAINT | | taint.cpp:173:8:173:13 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:180:19:180:19 | p | taint.cpp:181:9:181:9 | p | | | taint.cpp:181:9:181:9 | p | taint.cpp:181:8:181:9 | * ... | TAINT | | taint.cpp:185:11:185:16 | call to source | taint.cpp:186:11:186:11 | x | | | taint.cpp:186:10:186:11 | ref arg & ... | taint.cpp:186:11:186:11 | x | | | taint.cpp:186:11:186:11 | x | taint.cpp:186:10:186:11 | & ... | TAINT | +| taint.cpp:192:23:192:28 | source | taint.cpp:194:13:194:18 | source | | +| taint.cpp:193:6:193:6 | x | taint.cpp:194:10:194:10 | x | | +| taint.cpp:193:6:193:6 | x | taint.cpp:195:7:195:7 | x | | +| taint.cpp:194:9:194:10 | & ... | taint.cpp:194:2:194:7 | call to memcpy | | +| taint.cpp:194:9:194:10 | ref arg & ... | taint.cpp:194:10:194:10 | x | | +| taint.cpp:194:9:194:10 | ref arg & ... | taint.cpp:195:7:195:7 | x | | +| taint.cpp:194:10:194:10 | x | taint.cpp:194:9:194:10 | & ... | TAINT | +| taint.cpp:194:13:194:18 | source | taint.cpp:194:9:194:10 | ref arg & ... | TAINT | +| taint.cpp:194:21:194:31 | sizeof(int) | taint.cpp:194:9:194:10 | ref arg & ... | TAINT | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp index c9c028efe8a..fc0912cb9df 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp @@ -170,7 +170,7 @@ namespace strings strcpy(buffer, "Hello, "); sink(buffer); strcat(buffer, tainted); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted } } @@ -186,3 +186,11 @@ namespace refs { callee(&x); } } + +void *memcpy(void *dest, void *src, int len); + +void test_memcpy(int *source) { + int x; + memcpy(&x, source, sizeof(int)); + sink(x); +} diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected index d9743bfd536..93bc8084c87 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected @@ -12,3 +12,5 @@ | taint.cpp:168:8:168:14 | tainted | taint.cpp:164:19:164:24 | call to source | | taint.cpp:173:8:173:13 | buffer | taint.cpp:164:19:164:24 | call to source | | taint.cpp:181:8:181:9 | * ... | taint.cpp:185:11:185:16 | call to source | +| taint.cpp:195:7:195:7 | x | taint.cpp:192:23:192:28 | source | +| taint.cpp:195:7:195:7 | x | taint.cpp:193:6:193:6 | x | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected index 041c6f7920e..cce0efdcfc8 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected @@ -3,4 +3,7 @@ | taint.cpp:37:22:37:27 | taint.cpp:43:7:43:13 | AST only | | taint.cpp:120:11:120:16 | taint.cpp:137:7:137:9 | AST only | | taint.cpp:127:8:127:13 | taint.cpp:130:7:130:9 | IR only | +| taint.cpp:164:19:164:24 | taint.cpp:173:8:173:13 | AST only | | taint.cpp:185:11:185:16 | taint.cpp:181:8:181:9 | AST only | +| taint.cpp:192:23:192:28 | taint.cpp:195:7:195:7 | AST only | +| taint.cpp:193:6:193:6 | taint.cpp:195:7:195:7 | AST only | From f5c57b77e657500045bd29ab0196a1d5cfab20dd Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Thu, 25 Apr 2019 16:16:27 -0700 Subject: [PATCH 4/4] C++: fix whitespace --- cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll index baec8836aed..224e3858a2a 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/TaintTracking.qll @@ -239,7 +239,7 @@ module TaintTracking { exists(int argInIndex, FunctionInput inModel | f.hasDataFlow(inModel, outModel) | - // Taint flows from a pointer to a dereference, which DataFlow does not handle + // Taint flows from a pointer to a dereference, which DataFlow does not handle // memcpy(&dest_var, tainted_ptr, len) inModel.isInParameterPointer(argInIndex) and exprIn = call.getArgument(argInIndex)