From df526552a69d446d544cdd7c1da8c7cbf10feaec Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 16 Dec 2022 14:14:48 +0000 Subject: [PATCH 1/3] C++: Fix mapping between dataflow nodes and '{Crement, Assign}Operations'. --- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index 8730027ae45..96e2bdd3b10 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -821,9 +821,27 @@ private predicate exprNodeShouldBeIndirectOutNode(IndirectArgumentOutNode node, /** Holds if `node` should be an instruction node that maps `node.asExpr()` to `e`. */ predicate exprNodeShouldBeInstruction(Node node, Expr e) { - e = node.asInstruction().getConvertedResultExpression() and not exprNodeShouldBeOperand(_, e) and - not exprNodeShouldBeIndirectOutNode(_, e) + not exprNodeShouldBeIndirectOutNode(_, e) and + ( + e = node.asInstruction().getConvertedResultExpression() + or + // The instruction that contains the result of an `AssignOperation` is + // the unloaded left operand (see the comments in `TranslatedAssignOperation`). + // That means that for cases like + // ```cpp + // int x = ...; + // x += 1; + // ``` + // the result of `x += 1` is the `VariableAddressInstruction` that represents `x`. But + // that instruction doesn't receive the flow from this `AssignOperation`. So instead we + // map the operation to the `AddInstruction`. + node.asInstruction().getAst() = e.(AssignOperation) + or + // Same story for `CrementOperation`s (cf. the comments in the subclasses + // of `TranslatedCrementOperation`). + node.asInstruction().getAst() = e.(CrementOperation) + ) } /** Holds if `node` should be an `IndirectInstruction` that maps `node.asIndirectExpr()` to `e`. */ From 45f69be94ca201b11f4e7c00bab9faff0f431fb8 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 16 Dec 2022 14:14:58 +0000 Subject: [PATCH 2/3] C++: Accept test changes --- .../Format/NonConstantFormat/NonConstantFormat.expected | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected index cd78dd720d6..0ea73248a7d 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected @@ -12,7 +12,9 @@ | test.cpp:65:12:65:39 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:67:10:67:35 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:70:12:70:20 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. | +| test.cpp:76:12:76:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:82:12:82:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. | +| test.cpp:88:12:88:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:93:12:93:18 | ++ ... | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:100:12:100:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:110:12:110:24 | new[] | The format string argument to printf should be constant to prevent security issues and other potential errors. | From a7aa1a7d8b449e2512372deb92bddcc2d59e3188 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 16 Dec 2022 16:04:35 +0000 Subject: [PATCH 3/3] C++: Accept more test changes --- .../Security/CWE/CWE-078/WordexpTainted.expected | 10 ++++++++++ .../Adding365DaysPerYear/Adding365daysPerYear.expected | 4 ++++ .../CWE/CWE-134/semmle/argv/argvLocal.expected | 6 ++++++ 3 files changed, 20 insertions(+) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-078/WordexpTainted.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-078/WordexpTainted.expected index 922d428e369..cb2956b2a76 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-078/WordexpTainted.expected +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-078/WordexpTainted.expected @@ -1,11 +1,21 @@ edges | test.cpp:23:20:23:23 | argv | test.cpp:29:13:29:20 | filePath | | test.cpp:23:20:23:23 | argv | test.cpp:29:13:29:20 | filePath | +| test.cpp:23:20:23:23 | argv indirection | test.cpp:29:13:29:20 | filePath | +| test.cpp:23:20:23:23 | argv indirection | test.cpp:29:13:29:20 | filePath | +| test.cpp:23:20:23:23 | argv indirection | test.cpp:29:13:29:20 | filePath | +| test.cpp:23:20:23:23 | argv indirection | test.cpp:29:13:29:20 | filePath | nodes | test.cpp:23:20:23:23 | argv | semmle.label | argv | +| test.cpp:23:20:23:23 | argv indirection | semmle.label | argv indirection | +| test.cpp:23:20:23:23 | argv indirection | semmle.label | argv indirection | | test.cpp:29:13:29:20 | filePath | semmle.label | filePath | | test.cpp:29:13:29:20 | filePath | semmle.label | filePath | subpaths #select | test.cpp:29:13:29:20 | filePath | test.cpp:23:20:23:23 | argv | test.cpp:29:13:29:20 | filePath | Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection. | | test.cpp:29:13:29:20 | filePath | test.cpp:23:20:23:23 | argv | test.cpp:29:13:29:20 | filePath | Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection. | +| test.cpp:29:13:29:20 | filePath | test.cpp:23:20:23:23 | argv indirection | test.cpp:29:13:29:20 | filePath | Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection. | +| test.cpp:29:13:29:20 | filePath | test.cpp:23:20:23:23 | argv indirection | test.cpp:29:13:29:20 | filePath | Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection. | +| test.cpp:29:13:29:20 | filePath | test.cpp:23:20:23:23 | argv indirection | test.cpp:29:13:29:20 | filePath | Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection. | +| test.cpp:29:13:29:20 | filePath | test.cpp:23:20:23:23 | argv indirection | test.cpp:29:13:29:20 | filePath | Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.expected index 2d986e4b72f..898b0c32c5d 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.expected @@ -1,5 +1,9 @@ +| test.cpp:173:29:173:51 | ... & ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:170:2:170:47 | ... += ... | ... += ... | | test.cpp:173:29:173:51 | ... & ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:170:16:170:47 | ... * ... | ... * ... | +| test.cpp:174:30:174:45 | ... >> ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:170:2:170:47 | ... += ... | ... += ... | | test.cpp:174:30:174:45 | ... >> ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:170:16:170:47 | ... * ... | ... * ... | | test.cpp:193:15:193:24 | ... / ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:193:15:193:24 | ... / ... | ... / ... | +| test.cpp:217:29:217:51 | ... & ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:214:2:214:47 | ... += ... | ... += ... | | test.cpp:217:29:217:51 | ... & ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:214:16:214:47 | ... * ... | ... * ... | +| test.cpp:218:30:218:45 | ... >> ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:214:2:214:47 | ... += ... | ... += ... | | test.cpp:218:30:218:45 | ... >> ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:214:16:214:47 | ... * ... | ... * ... | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/argv/argvLocal.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/argv/argvLocal.expected index 35fc171319e..0bc33daca34 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/argv/argvLocal.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/argv/argvLocal.expected @@ -77,6 +77,10 @@ edges | argvLocal.c:115:13:115:16 | argv | argvLocal.c:135:9:135:12 | ... ++ | | argvLocal.c:115:13:115:16 | argv | argvLocal.c:135:9:135:12 | ... ++ | | argvLocal.c:115:13:115:16 | argv | argvLocal.c:135:9:135:12 | ... ++ | +| argvLocal.c:115:13:115:16 | argv | argvLocal.c:135:9:135:12 | ... ++ | +| argvLocal.c:115:13:115:16 | argv | argvLocal.c:135:9:135:12 | ... ++ | +| argvLocal.c:115:13:115:16 | argv | argvLocal.c:135:9:135:12 | ... ++ | +| argvLocal.c:115:13:115:16 | argv | argvLocal.c:135:9:135:12 | ... ++ | | argvLocal.c:115:13:115:16 | argv | argvLocal.c:136:15:136:18 | -- ... | | argvLocal.c:115:13:115:16 | argv | argvLocal.c:136:15:136:18 | -- ... | | argvLocal.c:115:13:115:16 | argv | argvLocal.c:136:15:136:18 | -- ... | @@ -197,6 +201,8 @@ nodes | argvLocal.c:135:9:135:12 | ... ++ | semmle.label | ... ++ | | argvLocal.c:135:9:135:12 | ... ++ | semmle.label | ... ++ | | argvLocal.c:135:9:135:12 | ... ++ | semmle.label | ... ++ | +| argvLocal.c:135:9:135:12 | ... ++ | semmle.label | ... ++ | +| argvLocal.c:135:9:135:12 | ... ++ | semmle.label | ... ++ | | argvLocal.c:136:15:136:18 | -- ... | semmle.label | -- ... | | argvLocal.c:136:15:136:18 | -- ... | semmle.label | -- ... | | argvLocal.c:136:15:136:18 | -- ... | semmle.label | -- ... |