From 754d7f0be847fbd9f0517435360ea34145f972ea Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 14 May 2020 15:18:12 +0100 Subject: [PATCH 1/8] C++: More test cases for TaintedAllocationSize. --- .../TaintedAllocationSize.expected | 65 +++++++++++++++++++ .../semmle/TaintedAllocationSize/test.cpp | 62 ++++++++++++++++++ 2 files changed, 127 insertions(+) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected index 773a969e9ad..e08db7a9c3e 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected @@ -59,6 +59,32 @@ edges | test.cpp:227:24:227:37 | (const char *)... | test.cpp:237:10:237:19 | (size_t)... | | test.cpp:235:11:235:20 | (size_t)... | test.cpp:214:23:214:23 | s | | test.cpp:237:10:237:19 | (size_t)... | test.cpp:220:21:220:21 | s | +| test.cpp:241:2:241:32 | Chi | test.cpp:271:17:271:20 | get_size output argument | +| test.cpp:241:2:241:32 | Chi | test.cpp:279:17:279:20 | get_size output argument | +| test.cpp:241:2:241:32 | Chi | test.cpp:287:18:287:21 | get_size output argument | +| test.cpp:241:2:241:32 | Chi | test.cpp:295:18:295:21 | get_size output argument | +| test.cpp:241:18:241:23 | call to getenv | test.cpp:241:2:241:32 | Chi | +| test.cpp:241:18:241:31 | (const char *)... | test.cpp:241:2:241:32 | Chi | +| test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | +| test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | +| test.cpp:249:20:249:25 | call to getenv | test.cpp:257:11:257:29 | ... * ... | +| test.cpp:249:20:249:25 | call to getenv | test.cpp:257:11:257:29 | ... * ... | +| test.cpp:249:20:249:33 | (const char *)... | test.cpp:253:11:253:29 | ... * ... | +| test.cpp:249:20:249:33 | (const char *)... | test.cpp:253:11:253:29 | ... * ... | +| test.cpp:249:20:249:33 | (const char *)... | test.cpp:257:11:257:29 | ... * ... | +| test.cpp:249:20:249:33 | (const char *)... | test.cpp:257:11:257:29 | ... * ... | +| test.cpp:261:19:261:24 | call to getenv | test.cpp:266:10:266:27 | ... * ... | +| test.cpp:261:19:261:24 | call to getenv | test.cpp:266:10:266:27 | ... * ... | +| test.cpp:261:19:261:32 | (const char *)... | test.cpp:266:10:266:27 | ... * ... | +| test.cpp:261:19:261:32 | (const char *)... | test.cpp:266:10:266:27 | ... * ... | +| test.cpp:271:17:271:20 | get_size output argument | test.cpp:273:11:273:28 | ... * ... | +| test.cpp:271:17:271:20 | get_size output argument | test.cpp:273:11:273:28 | ... * ... | +| test.cpp:279:17:279:20 | get_size output argument | test.cpp:281:11:281:28 | ... * ... | +| test.cpp:279:17:279:20 | get_size output argument | test.cpp:281:11:281:28 | ... * ... | +| test.cpp:287:18:287:21 | get_size output argument | test.cpp:290:10:290:27 | ... * ... | +| test.cpp:287:18:287:21 | get_size output argument | test.cpp:290:10:290:27 | ... * ... | +| test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... | +| test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... | nodes | test.cpp:39:21:39:24 | argv | semmle.label | argv | | test.cpp:39:21:39:24 | argv | semmle.label | argv | @@ -122,6 +148,38 @@ nodes | test.cpp:231:9:231:24 | call to get_tainted_size | semmle.label | call to get_tainted_size | | test.cpp:235:11:235:20 | (size_t)... | semmle.label | (size_t)... | | test.cpp:237:10:237:19 | (size_t)... | semmle.label | (size_t)... | +| test.cpp:241:2:241:32 | Chi | semmle.label | Chi | +| test.cpp:241:18:241:23 | call to getenv | semmle.label | call to getenv | +| test.cpp:241:18:241:31 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:249:20:249:25 | call to getenv | semmle.label | call to getenv | +| test.cpp:249:20:249:33 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... | +| test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... | +| test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... | +| test.cpp:257:11:257:29 | ... * ... | semmle.label | ... * ... | +| test.cpp:257:11:257:29 | ... * ... | semmle.label | ... * ... | +| test.cpp:257:11:257:29 | ... * ... | semmle.label | ... * ... | +| test.cpp:261:19:261:24 | call to getenv | semmle.label | call to getenv | +| test.cpp:261:19:261:32 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:266:10:266:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:266:10:266:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:266:10:266:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:271:17:271:20 | get_size output argument | semmle.label | get_size output argument | +| test.cpp:273:11:273:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:273:11:273:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:273:11:273:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:279:17:279:20 | get_size output argument | semmle.label | get_size output argument | +| test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:287:18:287:21 | get_size output argument | semmle.label | get_size output argument | +| test.cpp:290:10:290:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:290:10:290:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:290:10:290:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:295:18:295:21 | get_size output argument | semmle.label | get_size output argument | +| test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | #select | test.cpp:42:31:42:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | | test.cpp:43:31:43:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:43:38:43:63 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | @@ -136,3 +194,10 @@ nodes | test.cpp:221:14:221:19 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:221:21:221:21 | s | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (getenv) | | test.cpp:229:2:229:7 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:229:9:229:18 | local_size | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (getenv) | | test.cpp:231:2:231:7 | call to malloc | test.cpp:201:14:201:19 | call to getenv | test.cpp:231:9:231:24 | call to get_tainted_size | This allocation size is derived from $@ and might overflow | test.cpp:201:14:201:19 | call to getenv | user input (getenv) | +| test.cpp:253:4:253:9 | call to malloc | test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:249:20:249:25 | call to getenv | user input (getenv) | +| test.cpp:257:4:257:9 | call to malloc | test.cpp:249:20:249:25 | call to getenv | test.cpp:257:11:257:29 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:249:20:249:25 | call to getenv | user input (getenv) | +| test.cpp:266:3:266:8 | call to malloc | test.cpp:261:19:261:24 | call to getenv | test.cpp:266:10:266:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:261:19:261:24 | call to getenv | user input (getenv) | +| test.cpp:273:4:273:9 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:273:11:273:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | +| test.cpp:281:4:281:9 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:281:11:281:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | +| test.cpp:290:3:290:8 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:290:10:290:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | +| test.cpp:298:3:298:8 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:298:10:298:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp index cb21e8f1915..ad1153dd910 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp @@ -236,3 +236,65 @@ void more_cases() { my_func(100); // GOOD my_func(local_size); // GOOD } + +bool get_size(int &out_size) { + out_size = atoi(getenv("USER")); + + return true; +} + +void equality_cases() { + { + int size1 = atoi(getenv("USER")); + int size2 = atoi(getenv("USER")); + + if (size1 == 100) + { + malloc(size2 * sizeof(int)); // BAD + } + if (size2 == 100) + { + malloc(size2 * sizeof(int)); // GOOD [FALSE POSITIVE] + } + } + { + int size = atoi(getenv("USER")); + + if (size != 100) + return; + + malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE] + } + { + int size; + + if ((get_size(size)) && (size == 100)) + { + malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE] + } + } + { + int size; + + if ((get_size(size)) && (size != 100)) + { + malloc(size * sizeof(int)); // BAD + } + } + { + int size; + + if ((!get_size(size)) || (size != 100)) + return; + + malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE] + } + { + int size; + + if ((!get_size(size)) || (size == 100)) + return; + + malloc(size * sizeof(int)); // BAD + } +} From 4a6021fb61440f705293d0acc8a410746c47ed42 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 14 May 2020 18:19:32 +0100 Subject: [PATCH 2/8] C++: Allow equality checking to block taint flow. --- .../cpp/ir/dataflow/DefaultTaintTracking.qll | 18 ++++++++++ .../TaintedAllocationSize.expected | 34 ------------------- .../semmle/TaintedAllocationSize/test.cpp | 8 ++--- 3 files changed, 22 insertions(+), 38 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll index dbeefae4880..c98a13a23bd 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll @@ -5,6 +5,7 @@ private import semmle.code.cpp.ir.dataflow.DataFlow2 private import semmle.code.cpp.ir.dataflow.DataFlow3 private import semmle.code.cpp.ir.IR private import semmle.code.cpp.ir.dataflow.internal.DataFlowDispatch as Dispatch +private import semmle.code.cpp.controlflow.IRGuards private import semmle.code.cpp.models.interfaces.Taint private import semmle.code.cpp.models.interfaces.DataFlow @@ -175,6 +176,23 @@ private predicate nodeIsBarrier(DataFlow::Node node) { readsVariable(node.asInstruction(), checkedVar) and hasUpperBoundsCheck(checkedVar) ) + or + exists(Variable checkedVar, IRGuardCondition guard, Operand access, Operand other | + /* + * This node is guarded by a condition that forces the accessed variable + * to equal something else. For example: + * ``` + * x = taintsource() + * if (x == 10) { + * taintsink(x); // not considered tainted + * } + * ``` + */ + + readsVariable(node.asInstruction(), checkedVar) and + readsVariable(access.getDef(), checkedVar) and + guard.ensuresEq(access, other, _, node.asInstruction().getBlock(), true) + ) } private predicate nodeIsBarrierIn(DataFlow::Node node) { diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected index e08db7a9c3e..ed154eca0bb 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected @@ -59,30 +59,16 @@ edges | test.cpp:227:24:227:37 | (const char *)... | test.cpp:237:10:237:19 | (size_t)... | | test.cpp:235:11:235:20 | (size_t)... | test.cpp:214:23:214:23 | s | | test.cpp:237:10:237:19 | (size_t)... | test.cpp:220:21:220:21 | s | -| test.cpp:241:2:241:32 | Chi | test.cpp:271:17:271:20 | get_size output argument | | test.cpp:241:2:241:32 | Chi | test.cpp:279:17:279:20 | get_size output argument | -| test.cpp:241:2:241:32 | Chi | test.cpp:287:18:287:21 | get_size output argument | | test.cpp:241:2:241:32 | Chi | test.cpp:295:18:295:21 | get_size output argument | | test.cpp:241:18:241:23 | call to getenv | test.cpp:241:2:241:32 | Chi | | test.cpp:241:18:241:31 | (const char *)... | test.cpp:241:2:241:32 | Chi | | test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | | test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | -| test.cpp:249:20:249:25 | call to getenv | test.cpp:257:11:257:29 | ... * ... | -| test.cpp:249:20:249:25 | call to getenv | test.cpp:257:11:257:29 | ... * ... | | test.cpp:249:20:249:33 | (const char *)... | test.cpp:253:11:253:29 | ... * ... | | test.cpp:249:20:249:33 | (const char *)... | test.cpp:253:11:253:29 | ... * ... | -| test.cpp:249:20:249:33 | (const char *)... | test.cpp:257:11:257:29 | ... * ... | -| test.cpp:249:20:249:33 | (const char *)... | test.cpp:257:11:257:29 | ... * ... | -| test.cpp:261:19:261:24 | call to getenv | test.cpp:266:10:266:27 | ... * ... | -| test.cpp:261:19:261:24 | call to getenv | test.cpp:266:10:266:27 | ... * ... | -| test.cpp:261:19:261:32 | (const char *)... | test.cpp:266:10:266:27 | ... * ... | -| test.cpp:261:19:261:32 | (const char *)... | test.cpp:266:10:266:27 | ... * ... | -| test.cpp:271:17:271:20 | get_size output argument | test.cpp:273:11:273:28 | ... * ... | -| test.cpp:271:17:271:20 | get_size output argument | test.cpp:273:11:273:28 | ... * ... | | test.cpp:279:17:279:20 | get_size output argument | test.cpp:281:11:281:28 | ... * ... | | test.cpp:279:17:279:20 | get_size output argument | test.cpp:281:11:281:28 | ... * ... | -| test.cpp:287:18:287:21 | get_size output argument | test.cpp:290:10:290:27 | ... * ... | -| test.cpp:287:18:287:21 | get_size output argument | test.cpp:290:10:290:27 | ... * ... | | test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... | | test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... | nodes @@ -156,26 +142,10 @@ nodes | test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... | | test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... | | test.cpp:253:11:253:29 | ... * ... | semmle.label | ... * ... | -| test.cpp:257:11:257:29 | ... * ... | semmle.label | ... * ... | -| test.cpp:257:11:257:29 | ... * ... | semmle.label | ... * ... | -| test.cpp:257:11:257:29 | ... * ... | semmle.label | ... * ... | -| test.cpp:261:19:261:24 | call to getenv | semmle.label | call to getenv | -| test.cpp:261:19:261:32 | (const char *)... | semmle.label | (const char *)... | -| test.cpp:266:10:266:27 | ... * ... | semmle.label | ... * ... | -| test.cpp:266:10:266:27 | ... * ... | semmle.label | ... * ... | -| test.cpp:266:10:266:27 | ... * ... | semmle.label | ... * ... | -| test.cpp:271:17:271:20 | get_size output argument | semmle.label | get_size output argument | -| test.cpp:273:11:273:28 | ... * ... | semmle.label | ... * ... | -| test.cpp:273:11:273:28 | ... * ... | semmle.label | ... * ... | -| test.cpp:273:11:273:28 | ... * ... | semmle.label | ... * ... | | test.cpp:279:17:279:20 | get_size output argument | semmle.label | get_size output argument | | test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... | | test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... | | test.cpp:281:11:281:28 | ... * ... | semmle.label | ... * ... | -| test.cpp:287:18:287:21 | get_size output argument | semmle.label | get_size output argument | -| test.cpp:290:10:290:27 | ... * ... | semmle.label | ... * ... | -| test.cpp:290:10:290:27 | ... * ... | semmle.label | ... * ... | -| test.cpp:290:10:290:27 | ... * ... | semmle.label | ... * ... | | test.cpp:295:18:295:21 | get_size output argument | semmle.label | get_size output argument | | test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | | test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | @@ -195,9 +165,5 @@ nodes | test.cpp:229:2:229:7 | call to malloc | test.cpp:227:24:227:29 | call to getenv | test.cpp:229:9:229:18 | local_size | This allocation size is derived from $@ and might overflow | test.cpp:227:24:227:29 | call to getenv | user input (getenv) | | test.cpp:231:2:231:7 | call to malloc | test.cpp:201:14:201:19 | call to getenv | test.cpp:231:9:231:24 | call to get_tainted_size | This allocation size is derived from $@ and might overflow | test.cpp:201:14:201:19 | call to getenv | user input (getenv) | | test.cpp:253:4:253:9 | call to malloc | test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:249:20:249:25 | call to getenv | user input (getenv) | -| test.cpp:257:4:257:9 | call to malloc | test.cpp:249:20:249:25 | call to getenv | test.cpp:257:11:257:29 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:249:20:249:25 | call to getenv | user input (getenv) | -| test.cpp:266:3:266:8 | call to malloc | test.cpp:261:19:261:24 | call to getenv | test.cpp:266:10:266:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:261:19:261:24 | call to getenv | user input (getenv) | -| test.cpp:273:4:273:9 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:273:11:273:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | | test.cpp:281:4:281:9 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:281:11:281:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | -| test.cpp:290:3:290:8 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:290:10:290:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | | test.cpp:298:3:298:8 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:298:10:298:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp index ad1153dd910..4e984a20c7c 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp @@ -254,7 +254,7 @@ void equality_cases() { } if (size2 == 100) { - malloc(size2 * sizeof(int)); // GOOD [FALSE POSITIVE] + malloc(size2 * sizeof(int)); // GOOD } } { @@ -263,14 +263,14 @@ void equality_cases() { if (size != 100) return; - malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE] + malloc(size * sizeof(int)); // GOOD } { int size; if ((get_size(size)) && (size == 100)) { - malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE] + malloc(size * sizeof(int)); // GOOD } } { @@ -287,7 +287,7 @@ void equality_cases() { if ((!get_size(size)) || (size != 100)) return; - malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE] + malloc(size * sizeof(int)); // GOOD } { int size; From df5e16c45d9f1ccebabbdc6f5743f964ec4064e0 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 14 May 2020 18:41:14 +0100 Subject: [PATCH 3/8] C++: Add a 1.25 change note file (didn't we used to have templates for these?). --- change-notes/1.25/analysis-cpp.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 change-notes/1.25/analysis-cpp.md diff --git a/change-notes/1.25/analysis-cpp.md b/change-notes/1.25/analysis-cpp.md new file mode 100644 index 00000000000..f0fe2e06642 --- /dev/null +++ b/change-notes/1.25/analysis-cpp.md @@ -0,0 +1,19 @@ +# Improvements to C/C++ analysis + +The following changes in version 1.25 affect C/C++ analysis in all applications. + +## General improvements + + +## New queries + +| **Query** | **Tags** | **Purpose** | +|-----------------------------|-----------|--------------------------------------------------------------------| + +## Changes to existing queries + +| **Query** | **Expected impact** | **Change** | +|----------------------------|------------------------|------------------------------------------------------------------| + +## Changes to libraries + From 6579c71866f86a2967caf722ca8e6e94eae17ab8 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 14 May 2020 18:44:06 +0100 Subject: [PATCH 4/8] C++: Change note. --- change-notes/1.25/analysis-cpp.md | 1 + 1 file changed, 1 insertion(+) diff --git a/change-notes/1.25/analysis-cpp.md b/change-notes/1.25/analysis-cpp.md index f0fe2e06642..8e1123b2247 100644 --- a/change-notes/1.25/analysis-cpp.md +++ b/change-notes/1.25/analysis-cpp.md @@ -17,3 +17,4 @@ The following changes in version 1.25 affect C/C++ analysis in all applications. ## Changes to libraries +* The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) now considers that equality checks may block the flow of taint. This results in fewer false positive results from queries that use this library. From edd09f09cd80ec253d469d618bb05aee86c738ac Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 15 May 2020 17:00:42 +0100 Subject: [PATCH 5/8] C++: Add test cases where several specific values are permitted. --- .../TaintedAllocationSize.expected | 20 +++++++++++++++++++ .../semmle/TaintedAllocationSize/test.cpp | 16 +++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected index 000658da82c..26d49b1f90b 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected @@ -80,6 +80,14 @@ edges | test.cpp:279:17:279:20 | get_size output argument | test.cpp:281:11:281:28 | ... * ... | | test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... | | test.cpp:295:18:295:21 | get_size output argument | test.cpp:298:10:298:27 | ... * ... | +| test.cpp:301:19:301:24 | call to getenv | test.cpp:305:11:305:28 | ... * ... | +| test.cpp:301:19:301:24 | call to getenv | test.cpp:305:11:305:28 | ... * ... | +| test.cpp:301:19:301:32 | (const char *)... | test.cpp:305:11:305:28 | ... * ... | +| test.cpp:301:19:301:32 | (const char *)... | test.cpp:305:11:305:28 | ... * ... | +| test.cpp:309:19:309:24 | call to getenv | test.cpp:314:10:314:27 | ... * ... | +| test.cpp:309:19:309:24 | call to getenv | test.cpp:314:10:314:27 | ... * ... | +| test.cpp:309:19:309:32 | (const char *)... | test.cpp:314:10:314:27 | ... * ... | +| test.cpp:309:19:309:32 | (const char *)... | test.cpp:314:10:314:27 | ... * ... | nodes | field_conflation.c:12:22:12:27 | call to getenv | semmle.label | call to getenv | | field_conflation.c:12:22:12:34 | (const char *)... | semmle.label | (const char *)... | @@ -168,6 +176,16 @@ nodes | test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | | test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | | test.cpp:298:10:298:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:301:19:301:24 | call to getenv | semmle.label | call to getenv | +| test.cpp:301:19:301:32 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:305:11:305:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:305:11:305:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:305:11:305:28 | ... * ... | semmle.label | ... * ... | +| test.cpp:309:19:309:24 | call to getenv | semmle.label | call to getenv | +| test.cpp:309:19:309:32 | (const char *)... | semmle.label | (const char *)... | +| test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... | +| test.cpp:314:10:314:27 | ... * ... | semmle.label | ... * ... | #select | field_conflation.c:20:3:20:8 | call to malloc | field_conflation.c:12:22:12:27 | call to getenv | field_conflation.c:20:13:20:13 | x | This allocation size is derived from $@ and might overflow | field_conflation.c:12:22:12:27 | call to getenv | user input (getenv) | | test.cpp:42:31:42:36 | call to malloc | test.cpp:39:21:39:24 | argv | test.cpp:42:38:42:44 | tainted | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) | @@ -186,3 +204,5 @@ nodes | test.cpp:253:4:253:9 | call to malloc | test.cpp:249:20:249:25 | call to getenv | test.cpp:253:11:253:29 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:249:20:249:25 | call to getenv | user input (getenv) | | test.cpp:281:4:281:9 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:281:11:281:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | | test.cpp:298:3:298:8 | call to malloc | test.cpp:241:18:241:23 | call to getenv | test.cpp:298:10:298:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:241:18:241:23 | call to getenv | user input (getenv) | +| test.cpp:305:4:305:9 | call to malloc | test.cpp:301:19:301:24 | call to getenv | test.cpp:305:11:305:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:301:19:301:24 | call to getenv | user input (getenv) | +| test.cpp:314:3:314:8 | call to malloc | test.cpp:309:19:309:24 | call to getenv | test.cpp:314:10:314:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:309:19:309:24 | call to getenv | user input (getenv) | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp index 4e984a20c7c..0683f7211e3 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp @@ -297,4 +297,20 @@ void equality_cases() { malloc(size * sizeof(int)); // BAD } + { + int size = atoi(getenv("USER")); + + if ((size == 50) || (size == 100)) + { + malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE] + } + } + { + int size = atoi(getenv("USER")); + + if (size != 50 && size != 100) + return; + + malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE] + } } From fdf4e83c25723f851b064ce142f51a702f572a80 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 19 May 2020 16:59:37 +0100 Subject: [PATCH 6/8] C++: Solve tuple count bulge that may affect performance. --- .../code/cpp/ir/dataflow/DefaultTaintTracking.qll | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll index c98a13a23bd..1de41288867 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll @@ -171,13 +171,18 @@ private predicate hasUpperBoundsCheck(Variable var) { ) } +private predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) { + readsVariable(node.asInstruction(), checkedVar) and + any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true) +} + private predicate nodeIsBarrier(DataFlow::Node node) { exists(Variable checkedVar | readsVariable(node.asInstruction(), checkedVar) and hasUpperBoundsCheck(checkedVar) ) or - exists(Variable checkedVar, IRGuardCondition guard, Operand access, Operand other | + exists(Variable checkedVar, Operand access | /* * This node is guarded by a condition that forces the accessed variable * to equal something else. For example: @@ -189,9 +194,8 @@ private predicate nodeIsBarrier(DataFlow::Node node) { * ``` */ - readsVariable(node.asInstruction(), checkedVar) and - readsVariable(access.getDef(), checkedVar) and - guard.ensuresEq(access, other, _, node.asInstruction().getBlock(), true) + nodeIsBarrierEqualityCandidate(node, access, checkedVar) and + readsVariable(access.getDef(), checkedVar) ) } From f2436ff713417d130231230fb4477ac54878824b Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 20 May 2020 12:39:54 +0100 Subject: [PATCH 7/8] C++: Autoformat. --- .../src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll index 1de41288867..8186ac9268f 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll @@ -171,7 +171,9 @@ private predicate hasUpperBoundsCheck(Variable var) { ) } -private predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) { +private predicate nodeIsBarrierEqualityCandidate( + DataFlow::Node node, Operand access, Variable checkedVar +) { readsVariable(node.asInstruction(), checkedVar) and any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true) } From 9babd5dc1035c354b6853ac6d9e01c7efd85eb65 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 20 May 2020 12:49:01 +0100 Subject: [PATCH 8/8] C++: Another positive effect of the change. --- .../CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected | 1 - .../test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected index 20e5eafbd3b..a46371f36b6 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected @@ -5,5 +5,4 @@ | test.c:63:3:63:5 | sc8 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:62:9:62:16 | - ... | Extreme value | | test.c:75:3:75:5 | sc1 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:74:9:74:16 | 127 | Extreme value | | test.c:76:3:76:5 | sc1 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:74:9:74:16 | 127 | Extreme value | -| test.c:114:9:114:9 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:108:17:108:23 | 2147483647 | Extreme value | | test.c:124:9:124:9 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:118:17:118:23 | 2147483647 | Extreme value | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c index e17d413e3fd..8c40d984ee0 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c @@ -111,7 +111,7 @@ void test_guards3(int cond) { if (x != 0) return; - return x + 1; // GOOD [FALSE POSITIVE] + return x + 1; // GOOD } void test_guards4(int cond) {