From 67a1fb1d0ed4fae8c3da170ddebe639453e5c983 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 19 Aug 2019 11:47:49 +0200 Subject: [PATCH 1/3] C++: Test to show false flow through object copy --- .../dataflow/fields/aliasing.cpp | 63 +++++++++++++++++++ .../dataflow/fields/flow.expected | 26 ++++++++ 2 files changed, 89 insertions(+) create mode 100644 cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp diff --git a/cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp b/cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp new file mode 100644 index 000000000000..364db7aae737 --- /dev/null +++ b/cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp @@ -0,0 +1,63 @@ +int user_input(); +void sink(int); + +struct S { + int m1, m2; +}; + +void pointerSetter(S *s) { + s->m1 = user_input(); +} + +void referenceSetter(S &s) { + s.m1 = user_input(); +} + +void copySetter(S s) { + s.m1 = user_input(); +} + +void callSetters() { + S s1 = { 0, 0 }; + S s2 = { 0, 0 }; + S s3 = { 0, 0 }; + + pointerSetter(&s1); + referenceSetter(s2); + copySetter(s3); + + sink(s1.m1); // flow + sink(s2.m1); // flow + sink(s3.m1); // no flow +} + +void assignAfterAlias() { + S s1 = { 0, 0 }; + S &ref1 = s1; + ref1.m1 = user_input(); + sink(s1.m1); // flow + + S s2 = { 0, 0 }; + S &ref2 = s2; + s2.m1 = user_input(); + sink(ref2.m1); // flow +} + +void assignAfterCopy() { + S s1 = { 0, 0 }; + S copy1 = s1; + copy1.m1 = user_input(); + sink(s1.m1); // no flow [FALSE POSITIVE] + + S s2 = { 0, 0 }; + S copy2 = s2; + s2.m1 = user_input(); + sink(copy2.m1); // no flow [FALSE POSITIVE] +} + +void assignBeforeCopy() { + S s2 = { 0, 0 }; + s2.m1 = user_input(); + S copy2 = s2; + sink(copy2.m1); // flow +} diff --git a/cpp/ql/test/library-tests/dataflow/fields/flow.expected b/cpp/ql/test/library-tests/dataflow/fields/flow.expected index de790fa56ad3..5a39aa43c103 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/flow.expected @@ -86,6 +86,25 @@ edges | C.cpp:24:16:24:25 | new [void] | C.cpp:24:5:24:25 | ... = ... [void] | | C.cpp:27:8:27:11 | `this` parameter in func [s1, ... (1)] | file://:0:0:0:0 | this [s1, ... (1)] | | C.cpp:27:8:27:11 | `this` parameter in func [s3, ... (1)] | file://:0:0:0:0 | this [s3, ... (1)] | +| aliasing.cpp:9:3:9:3 | s [post update] [m1, ... (1)] | aliasing.cpp:25:17:25:19 | ref arg & ... [m1, ... (1)] | +| aliasing.cpp:9:3:9:22 | ... = ... [void] | aliasing.cpp:9:3:9:3 | s [post update] [m1, ... (1)] | +| aliasing.cpp:9:11:9:20 | call to user_input [void] | aliasing.cpp:9:3:9:22 | ... = ... [void] | +| aliasing.cpp:13:3:13:3 | s [post update] [m1, ... (1)] | aliasing.cpp:26:19:26:20 | ref arg s2 [m1, ... (1)] | +| aliasing.cpp:13:3:13:21 | ... = ... [void] | aliasing.cpp:13:3:13:3 | s [post update] [m1, ... (1)] | +| aliasing.cpp:13:10:13:19 | call to user_input [void] | aliasing.cpp:13:3:13:21 | ... = ... [void] | +| aliasing.cpp:25:17:25:19 | ref arg & ... [m1, ... (1)] | aliasing.cpp:29:8:29:9 | s1 [m1, ... (1)] | +| aliasing.cpp:26:19:26:20 | ref arg s2 [m1, ... (1)] | aliasing.cpp:30:8:30:9 | s2 [m1, ... (1)] | +| aliasing.cpp:29:8:29:9 | s1 [m1, ... (1)] | aliasing.cpp:29:11:29:12 | m1 | +| aliasing.cpp:30:8:30:9 | s2 [m1, ... (1)] | aliasing.cpp:30:11:30:12 | m1 | +| aliasing.cpp:37:13:37:22 | call to user_input [void] | aliasing.cpp:38:11:38:12 | m1 | +| aliasing.cpp:42:11:42:20 | call to user_input [void] | aliasing.cpp:43:13:43:14 | m1 | +| aliasing.cpp:49:14:49:23 | call to user_input [void] | aliasing.cpp:50:11:50:12 | m1 | +| aliasing.cpp:54:11:54:20 | call to user_input [void] | aliasing.cpp:55:14:55:15 | m1 | +| aliasing.cpp:60:3:60:4 | s2 [post update] [m1, ... (1)] | aliasing.cpp:62:8:62:12 | copy2 [m1, ... (1)] | +| aliasing.cpp:60:3:60:22 | ... = ... [void] | aliasing.cpp:60:3:60:4 | s2 [post update] [m1, ... (1)] | +| aliasing.cpp:60:11:60:20 | call to user_input [void] | aliasing.cpp:60:3:60:22 | ... = ... [void] | +| aliasing.cpp:60:11:60:20 | call to user_input [void] | aliasing.cpp:62:14:62:15 | m1 | +| aliasing.cpp:62:8:62:12 | copy2 [m1, ... (1)] | aliasing.cpp:62:14:62:15 | m1 | | constructors.cpp:26:15:26:15 | f [a_, ... (1)] | constructors.cpp:28:10:28:10 | f [a_, ... (1)] | | constructors.cpp:26:15:26:15 | f [b_, ... (1)] | constructors.cpp:29:10:29:10 | f [b_, ... (1)] | | constructors.cpp:28:10:28:10 | f [a_, ... (1)] | constructors.cpp:28:12:28:12 | call to a | @@ -148,6 +167,13 @@ edges | B.cpp:19:20:19:24 | elem2 | B.cpp:15:15:15:27 | new [void] | B.cpp:19:20:19:24 | elem2 | elem2 flows from $@ | B.cpp:15:15:15:27 | new [void] | new [void] | | C.cpp:29:10:29:11 | s1 | C.cpp:22:12:22:21 | new [void] | C.cpp:29:10:29:11 | s1 | s1 flows from $@ | C.cpp:22:12:22:21 | new [void] | new [void] | | C.cpp:31:10:31:11 | s3 | C.cpp:24:16:24:25 | new [void] | C.cpp:31:10:31:11 | s3 | s3 flows from $@ | C.cpp:24:16:24:25 | new [void] | new [void] | +| aliasing.cpp:29:11:29:12 | m1 | aliasing.cpp:9:11:9:20 | call to user_input [void] | aliasing.cpp:29:11:29:12 | m1 | m1 flows from $@ | aliasing.cpp:9:11:9:20 | call to user_input [void] | call to user_input [void] | +| aliasing.cpp:30:11:30:12 | m1 | aliasing.cpp:13:10:13:19 | call to user_input [void] | aliasing.cpp:30:11:30:12 | m1 | m1 flows from $@ | aliasing.cpp:13:10:13:19 | call to user_input [void] | call to user_input [void] | +| aliasing.cpp:38:11:38:12 | m1 | aliasing.cpp:37:13:37:22 | call to user_input [void] | aliasing.cpp:38:11:38:12 | m1 | m1 flows from $@ | aliasing.cpp:37:13:37:22 | call to user_input [void] | call to user_input [void] | +| aliasing.cpp:43:13:43:14 | m1 | aliasing.cpp:42:11:42:20 | call to user_input [void] | aliasing.cpp:43:13:43:14 | m1 | m1 flows from $@ | aliasing.cpp:42:11:42:20 | call to user_input [void] | call to user_input [void] | +| aliasing.cpp:50:11:50:12 | m1 | aliasing.cpp:49:14:49:23 | call to user_input [void] | aliasing.cpp:50:11:50:12 | m1 | m1 flows from $@ | aliasing.cpp:49:14:49:23 | call to user_input [void] | call to user_input [void] | +| aliasing.cpp:55:14:55:15 | m1 | aliasing.cpp:54:11:54:20 | call to user_input [void] | aliasing.cpp:55:14:55:15 | m1 | m1 flows from $@ | aliasing.cpp:54:11:54:20 | call to user_input [void] | call to user_input [void] | +| aliasing.cpp:62:14:62:15 | m1 | aliasing.cpp:60:11:60:20 | call to user_input [void] | aliasing.cpp:62:14:62:15 | m1 | m1 flows from $@ | aliasing.cpp:60:11:60:20 | call to user_input [void] | call to user_input [void] | | constructors.cpp:28:12:28:12 | call to a | constructors.cpp:34:11:34:20 | call to user_input [void] | constructors.cpp:28:12:28:12 | call to a | call to a flows from $@ | constructors.cpp:34:11:34:20 | call to user_input [void] | call to user_input [void] | | constructors.cpp:28:12:28:12 | call to a | constructors.cpp:36:11:36:20 | call to user_input [void] | constructors.cpp:28:12:28:12 | call to a | call to a flows from $@ | constructors.cpp:36:11:36:20 | call to user_input [void] | call to user_input [void] | | constructors.cpp:29:12:29:12 | call to b | constructors.cpp:35:14:35:23 | call to user_input [void] | constructors.cpp:29:12:29:12 | call to b | call to b flows from $@ | constructors.cpp:35:14:35:23 | call to user_input [void] | call to user_input [void] | From e5f55087cc58f42561fd1cc2a5e31cd64d1f7b94 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 19 Aug 2019 13:57:23 +0200 Subject: [PATCH 2/3] C++: Remove the primitive field flow in FlowVar Now that we have proper flow through fields in the interprocedural data flow library, it's time to remove the imprecise local flow through fields in `FlowVar.qll`. This removes all flow through fields in `DataFlow::localFlow` and other local-only predicates. The gain is that we no longer get the false positives that came from ignoring the qualifiers of field accesses. Performance of `getAReachedBlockVarSBB` is also much improved. --- cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll | 1 + .../test/library-tests/dataflow/dataflow-tests/test.cpp | 2 +- .../library-tests/dataflow/dataflow-tests/test.expected | 1 - .../dataflow/dataflow-tests/test_diff.expected | 1 - cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp | 8 ++++---- cpp/ql/test/library-tests/dataflow/fields/flow.expected | 9 --------- 6 files changed, 6 insertions(+), 16 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll index fa47c5af49f0..3130e64d7d65 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll @@ -212,6 +212,7 @@ module FlowVar_internal { or TBlockVar(SubBasicBlock sbb, Variable v) { not fullySupportedSsaVariable(v) and + not v instanceof Field and // Fields are interprocedural data flow, not local reachable(sbb) and ( initializer(sbb.getANode(), v, _) diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp index c37a36bc2183..5ba1b0d2b36a 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -137,7 +137,7 @@ void following_pointers( sink(sourceStruct1_ptr->m1); // flow sink(sourceStruct1_ptr->getFirst()); // flow [NOT DETECTED with IR] sink(sourceStruct1_ptr->m2); // no flow - sink(sourceStruct1.m1); // flow (due to lack of no-alias tracking) + sink(sourceStruct1.m1); // no flow twoIntFields s = { source(), source() }; diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected index d04fec5514b9..23f55edfcc44 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected @@ -22,7 +22,6 @@ | test.cpp:126:8:126:19 | sourceArray1 | test.cpp:120:9:120:20 | sourceArray1 | | test.cpp:137:27:137:28 | m1 | test.cpp:136:27:136:32 | call to source | | test.cpp:138:27:138:34 | call to getFirst | test.cpp:136:27:136:32 | call to source | -| test.cpp:140:22:140:23 | m1 | test.cpp:136:27:136:32 | call to source | | test.cpp:145:10:145:11 | m2 | test.cpp:142:32:142:37 | call to source | | test.cpp:153:17:153:18 | m2 | test.cpp:151:35:151:40 | call to source | | test.cpp:188:8:188:8 | y | test.cpp:186:27:186:32 | call to source | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected index f87f7e20703c..41611009590f 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected @@ -7,7 +7,6 @@ | test.cpp:109:9:109:14 | test.cpp:110:10:110:12 | IR only | | test.cpp:136:27:136:32 | test.cpp:137:27:137:28 | AST only | | test.cpp:136:27:136:32 | test.cpp:138:27:138:34 | AST only | -| test.cpp:136:27:136:32 | test.cpp:140:22:140:23 | AST only | | test.cpp:395:17:395:22 | test.cpp:397:10:397:18 | AST only | | test.cpp:407:13:407:18 | test.cpp:413:10:413:14 | AST only | | test.cpp:421:13:421:18 | test.cpp:417:10:417:14 | AST only | diff --git a/cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp b/cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp index 364db7aae737..bfe1a92b4b44 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp +++ b/cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp @@ -35,24 +35,24 @@ void assignAfterAlias() { S s1 = { 0, 0 }; S &ref1 = s1; ref1.m1 = user_input(); - sink(s1.m1); // flow + sink(s1.m1); // flow [NOT DETECTED] S s2 = { 0, 0 }; S &ref2 = s2; s2.m1 = user_input(); - sink(ref2.m1); // flow + sink(ref2.m1); // flow [NOT DETECTED] } void assignAfterCopy() { S s1 = { 0, 0 }; S copy1 = s1; copy1.m1 = user_input(); - sink(s1.m1); // no flow [FALSE POSITIVE] + sink(s1.m1); // no flow S s2 = { 0, 0 }; S copy2 = s2; s2.m1 = user_input(); - sink(copy2.m1); // no flow [FALSE POSITIVE] + sink(copy2.m1); // no flow } void assignBeforeCopy() { diff --git a/cpp/ql/test/library-tests/dataflow/fields/flow.expected b/cpp/ql/test/library-tests/dataflow/fields/flow.expected index 5a39aa43c103..6a0ff543635b 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/flow.expected @@ -96,14 +96,9 @@ edges | aliasing.cpp:26:19:26:20 | ref arg s2 [m1, ... (1)] | aliasing.cpp:30:8:30:9 | s2 [m1, ... (1)] | | aliasing.cpp:29:8:29:9 | s1 [m1, ... (1)] | aliasing.cpp:29:11:29:12 | m1 | | aliasing.cpp:30:8:30:9 | s2 [m1, ... (1)] | aliasing.cpp:30:11:30:12 | m1 | -| aliasing.cpp:37:13:37:22 | call to user_input [void] | aliasing.cpp:38:11:38:12 | m1 | -| aliasing.cpp:42:11:42:20 | call to user_input [void] | aliasing.cpp:43:13:43:14 | m1 | -| aliasing.cpp:49:14:49:23 | call to user_input [void] | aliasing.cpp:50:11:50:12 | m1 | -| aliasing.cpp:54:11:54:20 | call to user_input [void] | aliasing.cpp:55:14:55:15 | m1 | | aliasing.cpp:60:3:60:4 | s2 [post update] [m1, ... (1)] | aliasing.cpp:62:8:62:12 | copy2 [m1, ... (1)] | | aliasing.cpp:60:3:60:22 | ... = ... [void] | aliasing.cpp:60:3:60:4 | s2 [post update] [m1, ... (1)] | | aliasing.cpp:60:11:60:20 | call to user_input [void] | aliasing.cpp:60:3:60:22 | ... = ... [void] | -| aliasing.cpp:60:11:60:20 | call to user_input [void] | aliasing.cpp:62:14:62:15 | m1 | | aliasing.cpp:62:8:62:12 | copy2 [m1, ... (1)] | aliasing.cpp:62:14:62:15 | m1 | | constructors.cpp:26:15:26:15 | f [a_, ... (1)] | constructors.cpp:28:10:28:10 | f [a_, ... (1)] | | constructors.cpp:26:15:26:15 | f [b_, ... (1)] | constructors.cpp:29:10:29:10 | f [b_, ... (1)] | @@ -169,10 +164,6 @@ edges | C.cpp:31:10:31:11 | s3 | C.cpp:24:16:24:25 | new [void] | C.cpp:31:10:31:11 | s3 | s3 flows from $@ | C.cpp:24:16:24:25 | new [void] | new [void] | | aliasing.cpp:29:11:29:12 | m1 | aliasing.cpp:9:11:9:20 | call to user_input [void] | aliasing.cpp:29:11:29:12 | m1 | m1 flows from $@ | aliasing.cpp:9:11:9:20 | call to user_input [void] | call to user_input [void] | | aliasing.cpp:30:11:30:12 | m1 | aliasing.cpp:13:10:13:19 | call to user_input [void] | aliasing.cpp:30:11:30:12 | m1 | m1 flows from $@ | aliasing.cpp:13:10:13:19 | call to user_input [void] | call to user_input [void] | -| aliasing.cpp:38:11:38:12 | m1 | aliasing.cpp:37:13:37:22 | call to user_input [void] | aliasing.cpp:38:11:38:12 | m1 | m1 flows from $@ | aliasing.cpp:37:13:37:22 | call to user_input [void] | call to user_input [void] | -| aliasing.cpp:43:13:43:14 | m1 | aliasing.cpp:42:11:42:20 | call to user_input [void] | aliasing.cpp:43:13:43:14 | m1 | m1 flows from $@ | aliasing.cpp:42:11:42:20 | call to user_input [void] | call to user_input [void] | -| aliasing.cpp:50:11:50:12 | m1 | aliasing.cpp:49:14:49:23 | call to user_input [void] | aliasing.cpp:50:11:50:12 | m1 | m1 flows from $@ | aliasing.cpp:49:14:49:23 | call to user_input [void] | call to user_input [void] | -| aliasing.cpp:55:14:55:15 | m1 | aliasing.cpp:54:11:54:20 | call to user_input [void] | aliasing.cpp:55:14:55:15 | m1 | m1 flows from $@ | aliasing.cpp:54:11:54:20 | call to user_input [void] | call to user_input [void] | | aliasing.cpp:62:14:62:15 | m1 | aliasing.cpp:60:11:60:20 | call to user_input [void] | aliasing.cpp:62:14:62:15 | m1 | m1 flows from $@ | aliasing.cpp:60:11:60:20 | call to user_input [void] | call to user_input [void] | | constructors.cpp:28:12:28:12 | call to a | constructors.cpp:34:11:34:20 | call to user_input [void] | constructors.cpp:28:12:28:12 | call to a | call to a flows from $@ | constructors.cpp:34:11:34:20 | call to user_input [void] | call to user_input [void] | | constructors.cpp:28:12:28:12 | call to a | constructors.cpp:36:11:36:20 | call to user_input [void] | constructors.cpp:28:12:28:12 | call to a | call to a flows from $@ | constructors.cpp:36:11:36:20 | call to user_input [void] | call to user_input [void] | From 2f1a35481e0d8e5def0268c0e8c22496ac35f2cb Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 20 Aug 2019 09:25:39 +0200 Subject: [PATCH 3/3] C++: Accept a false negative in OverrunWrite.ql We can recover this result when we switch `localFlow` to be IR-based. --- .../Security/CWE/CWE-120/semmle/tests/OverrunWrite.expected | 1 - .../test/query-tests/Security/CWE/CWE-120/semmle/tests/unions.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWrite.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWrite.expected index 52fa2f376b03..7d986595cce4 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWrite.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/OverrunWrite.expected @@ -9,5 +9,4 @@ | tests.c:136:2:136:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. | | unions.c:26:2:26:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | | unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. | -| unions.c:32:2:32:7 | call to strcpy | This 'call to strcpy' operation requires 31 bytes but the destination is only 25 bytes. | | var_size_struct.cpp:22:3:22:8 | call to strcpy | This 'call to strcpy' operation requires 10 bytes but the destination is only 9 bytes. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/unions.c b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/unions.c index b31adf5e6b4e..68c9aff9c2b5 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/unions.c +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/unions.c @@ -29,5 +29,5 @@ void unions_test(MyUnion *mu) mu->ptr = buffer; strcpy(mu->ptr, "1234567890"); // GOOD strcpy(mu->ptr, "12345678901234567890"); // GOOD - strcpy(mu->ptr, "123456789012345678901234567890"); // BAD + strcpy(mu->ptr, "123456789012345678901234567890"); // BAD [NOT DETECTED] }