From d18dd5ab09021dcc0fbffd4e2a2979db5f323e5b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 15 Sep 2020 12:32:15 +0200 Subject: [PATCH 1/4] C++: Add testcase demonstrating the underlying problem in 6ca9c449af. --- .../dataflow/fields/dataflow-ir-consistency.expected | 1 + .../library-tests/dataflow/fields/flow-diff.expected | 1 + .../dataflow/fields/partial-definition-diff.expected | 1 + .../dataflow/fields/partial-definition-ir.expected | 1 + .../dataflow/fields/partial-definition.expected | 2 ++ .../library-tests/dataflow/fields/path-flow.expected | 10 ++++++++++ cpp/ql/test/library-tests/dataflow/fields/simple.cpp | 9 +++++++++ 7 files changed, 25 insertions(+) diff --git a/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected b/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected index 99d6808a0a25..7145de20ff16 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected @@ -20,6 +20,7 @@ localCallNodes postIsNotPre postHasUniquePre | simple.cpp:65:5:65:22 | Store | PostUpdateNode should have one pre-update node but has 0. | +| simple.cpp:92:5:92:22 | Store | PostUpdateNode should have one pre-update node but has 0. | uniquePostUpdate postIsInSameCallable reverseRead diff --git a/cpp/ql/test/library-tests/dataflow/fields/flow-diff.expected b/cpp/ql/test/library-tests/dataflow/fields/flow-diff.expected index 1da74bf6ff54..7dde7a4de0ae 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/flow-diff.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/flow-diff.expected @@ -37,5 +37,6 @@ | qualifiers.cpp:37:38:37:47 | call to user_input | qualifiers.cpp:38:23:38:23 | a | AST only | | qualifiers.cpp:42:29:42:38 | call to user_input | qualifiers.cpp:43:23:43:23 | a | AST only | | qualifiers.cpp:47:31:47:40 | call to user_input | qualifiers.cpp:48:23:48:23 | a | AST only | +| simple.cpp:92:11:92:20 | call to user_input | simple.cpp:94:13:94:13 | i | AST only | | struct_init.c:20:20:20:29 | call to user_input | struct_init.c:33:25:33:25 | a | AST only | | struct_init.c:40:20:40:29 | call to user_input | struct_init.c:15:12:15:12 | a | AST only | diff --git a/cpp/ql/test/library-tests/dataflow/fields/partial-definition-diff.expected b/cpp/ql/test/library-tests/dataflow/fields/partial-definition-diff.expected index c25eab1a4d04..d870657eb5a5 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/partial-definition-diff.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/partial-definition-diff.expected @@ -321,6 +321,7 @@ | simple.cpp:83:9:83:10 | this | AST only | | simple.cpp:83:12:83:13 | f1 | AST only | | simple.cpp:84:14:84:20 | this | AST only | +| simple.cpp:92:7:92:7 | i | AST only | | struct_init.c:15:8:15:9 | ab | AST only | | struct_init.c:15:12:15:12 | a | AST only | | struct_init.c:16:8:16:9 | ab | AST only | diff --git a/cpp/ql/test/library-tests/dataflow/fields/partial-definition-ir.expected b/cpp/ql/test/library-tests/dataflow/fields/partial-definition-ir.expected index 6ea7e129ca29..332e85244bd6 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/partial-definition-ir.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/partial-definition-ir.expected @@ -42,3 +42,4 @@ | simple.cpp:21:24:21:25 | this | | simple.cpp:65:5:65:5 | a | | simple.cpp:83:9:83:10 | f2 | +| simple.cpp:92:5:92:5 | a | diff --git a/cpp/ql/test/library-tests/dataflow/fields/partial-definition.expected b/cpp/ql/test/library-tests/dataflow/fields/partial-definition.expected index ba46d2e8e9d0..92ee4007ae55 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/partial-definition.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/partial-definition.expected @@ -365,6 +365,8 @@ | simple.cpp:83:9:83:10 | this | | simple.cpp:83:12:83:13 | f1 | | simple.cpp:84:14:84:20 | this | +| simple.cpp:92:5:92:5 | a | +| simple.cpp:92:7:92:7 | i | | struct_init.c:15:8:15:9 | ab | | struct_init.c:15:12:15:12 | a | | struct_init.c:16:8:16:9 | ab | diff --git a/cpp/ql/test/library-tests/dataflow/fields/path-flow.expected b/cpp/ql/test/library-tests/dataflow/fields/path-flow.expected index d505ff5d87e9..d06ec4ea8e2a 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/path-flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/path-flow.expected @@ -332,6 +332,10 @@ edges | simple.cpp:83:9:83:28 | ... = ... | simple.cpp:83:9:83:10 | f2 [post update] [f1] | | simple.cpp:83:17:83:26 | call to user_input | simple.cpp:83:9:83:28 | ... = ... | | simple.cpp:84:14:84:20 | this [f2, f1] | simple.cpp:84:14:84:20 | call to getf2f1 | +| simple.cpp:92:5:92:5 | a [post update] [i] | simple.cpp:94:10:94:11 | a2 [i] | +| simple.cpp:92:5:92:22 | ... = ... | simple.cpp:92:5:92:5 | a [post update] [i] | +| simple.cpp:92:11:92:20 | call to user_input | simple.cpp:92:5:92:22 | ... = ... | +| simple.cpp:94:10:94:11 | a2 [i] | simple.cpp:94:13:94:13 | i | | struct_init.c:14:24:14:25 | ab [a] | struct_init.c:15:8:15:9 | ab [a] | | struct_init.c:15:8:15:9 | ab [a] | struct_init.c:15:12:15:12 | a | | struct_init.c:20:17:20:36 | {...} [a] | struct_init.c:22:8:22:9 | ab [a] | @@ -732,6 +736,11 @@ nodes | simple.cpp:83:17:83:26 | call to user_input | semmle.label | call to user_input | | simple.cpp:84:14:84:20 | call to getf2f1 | semmle.label | call to getf2f1 | | simple.cpp:84:14:84:20 | this [f2, f1] | semmle.label | this [f2, f1] | +| simple.cpp:92:5:92:5 | a [post update] [i] | semmle.label | a [post update] [i] | +| simple.cpp:92:5:92:22 | ... = ... | semmle.label | ... = ... | +| simple.cpp:92:11:92:20 | call to user_input | semmle.label | call to user_input | +| simple.cpp:94:10:94:11 | a2 [i] | semmle.label | a2 [i] | +| simple.cpp:94:13:94:13 | i | semmle.label | i | | struct_init.c:14:24:14:25 | ab [a] | semmle.label | ab [a] | | struct_init.c:15:8:15:9 | ab [a] | semmle.label | ab [a] | | struct_init.c:15:12:15:12 | a | semmle.label | a | @@ -830,6 +839,7 @@ nodes | simple.cpp:29:12:29:12 | call to b | simple.cpp:42:12:42:21 | call to user_input | simple.cpp:29:12:29:12 | call to b | call to b flows from $@ | simple.cpp:42:12:42:21 | call to user_input | call to user_input | | simple.cpp:67:13:67:13 | i | simple.cpp:65:11:65:20 | call to user_input | simple.cpp:67:13:67:13 | i | i flows from $@ | simple.cpp:65:11:65:20 | call to user_input | call to user_input | | simple.cpp:84:14:84:20 | call to getf2f1 | simple.cpp:83:17:83:26 | call to user_input | simple.cpp:84:14:84:20 | call to getf2f1 | call to getf2f1 flows from $@ | simple.cpp:83:17:83:26 | call to user_input | call to user_input | +| simple.cpp:94:13:94:13 | i | simple.cpp:92:11:92:20 | call to user_input | simple.cpp:94:13:94:13 | i | i flows from $@ | simple.cpp:92:11:92:20 | call to user_input | call to user_input | | struct_init.c:15:12:15:12 | a | struct_init.c:20:20:20:29 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:20:20:20:29 | call to user_input | call to user_input | | struct_init.c:15:12:15:12 | a | struct_init.c:27:7:27:16 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:27:7:27:16 | call to user_input | call to user_input | | struct_init.c:15:12:15:12 | a | struct_init.c:40:20:40:29 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:40:20:40:29 | call to user_input | call to user_input | diff --git a/cpp/ql/test/library-tests/dataflow/fields/simple.cpp b/cpp/ql/test/library-tests/dataflow/fields/simple.cpp index 342a1100aa6b..d19eba8ed299 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/simple.cpp +++ b/cpp/ql/test/library-tests/dataflow/fields/simple.cpp @@ -85,4 +85,13 @@ struct C2 } }; +typedef A A_typedef; + +void single_field_test_typedef(A_typedef a) +{ + a.i = user_input(); + A_typedef a2 = a; + sink(a2.i); //$ast $f-:ir +} + } // namespace Simple From 265a641d06662ab7fb6d28c4caba93a0a31ea3b0 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 15 Sep 2020 12:49:16 +0200 Subject: [PATCH 2/4] C++: Use the underlying type to check whether a type is a single-field struct. --- .../src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 aedf3ab494af..3a68f25d9787 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 @@ -557,7 +557,7 @@ pragma[noinline] private predicate getFieldSizeOfClass(Class c, Type type, int size) { exists(Field f | f.getDeclaringType() = c and - f.getType() = type and + f.getUnderlyingType() = type and type.getSize() = size ) } @@ -601,7 +601,7 @@ private predicate simpleOperandLocalFlowStep(Instruction iFrom, Operand opTo) { exists(LoadInstruction load | load.getSourceValueOperand() = opTo and opTo.getAnyDef() = iFrom and - isSingleFieldClass(iFrom.getResultType(), opTo.getType()) + isSingleFieldClass(iFrom.getResultType(), opTo.getType().getUnderlyingType()) ) } From 0ba72c66850ce0833eee2b7364d635fc22de6915 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 15 Sep 2020 12:49:22 +0200 Subject: [PATCH 3/4] C++: Accept changes. --- .../test/library-tests/dataflow/fields/flow-diff.expected | 1 - .../library-tests/dataflow/fields/ir-path-flow.expected | 8 ++++++++ cpp/ql/test/library-tests/dataflow/fields/simple.cpp | 2 +- .../semmle/tainted/IntegerOverflowTainted.expected | 1 + 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/cpp/ql/test/library-tests/dataflow/fields/flow-diff.expected b/cpp/ql/test/library-tests/dataflow/fields/flow-diff.expected index 7dde7a4de0ae..1da74bf6ff54 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/flow-diff.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/flow-diff.expected @@ -37,6 +37,5 @@ | qualifiers.cpp:37:38:37:47 | call to user_input | qualifiers.cpp:38:23:38:23 | a | AST only | | qualifiers.cpp:42:29:42:38 | call to user_input | qualifiers.cpp:43:23:43:23 | a | AST only | | qualifiers.cpp:47:31:47:40 | call to user_input | qualifiers.cpp:48:23:48:23 | a | AST only | -| simple.cpp:92:11:92:20 | call to user_input | simple.cpp:94:13:94:13 | i | AST only | | struct_init.c:20:20:20:29 | call to user_input | struct_init.c:33:25:33:25 | a | AST only | | struct_init.c:40:20:40:29 | call to user_input | struct_init.c:15:12:15:12 | a | AST only | diff --git a/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected b/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected index efc02f23fb14..983616db6112 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected @@ -162,6 +162,9 @@ edges | simple.cpp:83:9:83:28 | Store | simple.cpp:83:9:83:28 | Chi [f1] | | simple.cpp:83:17:83:26 | call to user_input | simple.cpp:83:9:83:28 | Store | | simple.cpp:84:14:84:20 | Argument -1 indirection [f1] | simple.cpp:84:14:84:20 | call to getf2f1 | +| simple.cpp:92:5:92:22 | Store [i] | simple.cpp:93:20:93:20 | Store [i] | +| simple.cpp:92:11:92:20 | call to user_input | simple.cpp:92:5:92:22 | Store [i] | +| simple.cpp:93:20:93:20 | Store [i] | simple.cpp:94:13:94:13 | i | | struct_init.c:14:24:14:25 | *ab [a] | struct_init.c:15:12:15:12 | a | | struct_init.c:20:20:20:29 | Chi [a] | struct_init.c:24:10:24:12 | Argument 0 indirection [a] | | struct_init.c:20:20:20:29 | Store | struct_init.c:20:20:20:29 | Chi [a] | @@ -363,6 +366,10 @@ nodes | simple.cpp:83:17:83:26 | call to user_input | semmle.label | call to user_input | | simple.cpp:84:14:84:20 | Argument -1 indirection [f1] | semmle.label | Argument -1 indirection [f1] | | simple.cpp:84:14:84:20 | call to getf2f1 | semmle.label | call to getf2f1 | +| simple.cpp:92:5:92:22 | Store [i] | semmle.label | Store [i] | +| simple.cpp:92:11:92:20 | call to user_input | semmle.label | call to user_input | +| simple.cpp:93:20:93:20 | Store [i] | semmle.label | Store [i] | +| simple.cpp:94:13:94:13 | i | semmle.label | i | | struct_init.c:14:24:14:25 | *ab [a] | semmle.label | *ab [a] | | struct_init.c:15:12:15:12 | a | semmle.label | a | | struct_init.c:20:20:20:29 | Chi [a] | semmle.label | Chi [a] | @@ -417,6 +424,7 @@ nodes | simple.cpp:29:12:29:12 | call to b | simple.cpp:42:12:42:21 | call to user_input | simple.cpp:29:12:29:12 | call to b | call to b flows from $@ | simple.cpp:42:12:42:21 | call to user_input | call to user_input | | simple.cpp:67:13:67:13 | i | simple.cpp:65:11:65:20 | call to user_input | simple.cpp:67:13:67:13 | i | i flows from $@ | simple.cpp:65:11:65:20 | call to user_input | call to user_input | | simple.cpp:84:14:84:20 | call to getf2f1 | simple.cpp:83:17:83:26 | call to user_input | simple.cpp:84:14:84:20 | call to getf2f1 | call to getf2f1 flows from $@ | simple.cpp:83:17:83:26 | call to user_input | call to user_input | +| simple.cpp:94:13:94:13 | i | simple.cpp:92:11:92:20 | call to user_input | simple.cpp:94:13:94:13 | i | i flows from $@ | simple.cpp:92:11:92:20 | call to user_input | call to user_input | | struct_init.c:15:12:15:12 | a | struct_init.c:20:20:20:29 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:20:20:20:29 | call to user_input | call to user_input | | struct_init.c:15:12:15:12 | a | struct_init.c:27:7:27:16 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:27:7:27:16 | call to user_input | call to user_input | | struct_init.c:22:11:22:11 | a | struct_init.c:20:20:20:29 | call to user_input | struct_init.c:22:11:22:11 | a | a flows from $@ | struct_init.c:20:20:20:29 | call to user_input | call to user_input | diff --git a/cpp/ql/test/library-tests/dataflow/fields/simple.cpp b/cpp/ql/test/library-tests/dataflow/fields/simple.cpp index d19eba8ed299..3f7c91f77471 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/simple.cpp +++ b/cpp/ql/test/library-tests/dataflow/fields/simple.cpp @@ -91,7 +91,7 @@ void single_field_test_typedef(A_typedef a) { a.i = user_input(); A_typedef a2 = a; - sink(a2.i); //$ast $f-:ir + sink(a2.i); //$ast,ir } } // namespace Simple diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected index 556c4c19b3dc..e71cacde76ce 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected @@ -1,4 +1,5 @@ | test2.cpp:14:11:14:15 | ... * ... | $@ flows to here and is used in an expression which might overflow. | test2.cpp:25:22:25:23 | & ... | User-provided value | +| test2.cpp:15:11:15:19 | ... * ... | $@ flows to here and is used in an expression which might overflow. | test2.cpp:25:22:25:23 | & ... | User-provided value | | test2.cpp:16:11:16:21 | ... * ... | $@ flows to here and is used in an expression which might overflow. | test2.cpp:25:22:25:23 | & ... | User-provided value | | test2.cpp:17:11:17:22 | ... * ... | $@ flows to here and is used in an expression which might overflow. | test2.cpp:25:22:25:23 | & ... | User-provided value | | test3.c:12:31:12:34 | * ... | $@ flows to here and is used in an expression which might overflow negatively. | test3.c:11:15:11:18 | argv | User-provided value | From 3005f252ca55713f8eb7bb993ee359c19b7208d9 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 15 Sep 2020 13:34:50 +0200 Subject: [PATCH 4/4] C++: Fix annotation --- .../query-tests/Security/CWE/CWE-190/semmle/tainted/test2.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/test2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/test2.cpp index b22ae6ea5971..eabbd2747033 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/test2.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/test2.cpp @@ -12,7 +12,7 @@ typedef struct _myStruct { void test2_sink(s64 v, MyStruct s, MyStruct &s_r, MyStruct *s_p) { s64 v1 = v * 2; // bad - s64 v2 = s.val * 2; // bad [NOT DETECTED] + s64 v2 = s.val * 2; // bad s64 v3 = s_r.val * 2; // bad s64 v4 = s_p->val * 2; // bad }