diff --git a/cpp/ql/lib/change-notes/2023-04-28-static-local-dataflow.md b/cpp/ql/lib/change-notes/2023-04-28-static-local-dataflow.md new file mode 100644 index 00000000000..be4c4e73ed0 --- /dev/null +++ b/cpp/ql/lib/change-notes/2023-04-28-static-local-dataflow.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The new dataflow (`semmle.code.cpp.dataflow.new.DataFlow`) and taint-tracking libraries (`semmle.code.cpp.dataflow.new.TaintTracking`) now support tracking flow through static local variables. diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index 6d17e85863f..efd33b82a89 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -607,13 +607,21 @@ OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) { result.getReturnKind() = kind } +/** A variable that behaves like a global variable. */ +class GlobalLikeVariable extends Variable { + GlobalLikeVariable() { + this instanceof Cpp::GlobalOrNamespaceVariable or + this instanceof Cpp::StaticLocalVariable + } +} + /** * Holds if data can flow from `node1` to `node2` in a way that loses the * calling context. For example, this would happen with flow through a * global or static variable. */ predicate jumpStep(Node n1, Node n2) { - exists(Cpp::GlobalOrNamespaceVariable v | + exists(GlobalLikeVariable v | exists(Ssa::GlobalUse globalUse | v = globalUse.getVariable() and n1.(FinalGlobalValue).getGlobalUse() = globalUse diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll index ee958431b69..a1cfa44bb8e 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll @@ -145,14 +145,14 @@ private newtype TDefOrUseImpl = or // Since the pruning stage doesn't know about global variables we can't use the above check to // rule out dead assignments to globals. - base.(VariableAddressInstruction).getAstVariable() instanceof Cpp::GlobalOrNamespaceVariable + base.(VariableAddressInstruction).getAstVariable() instanceof GlobalLikeVariable ) } or TUseImpl(Operand operand, int indirectionIndex) { isUse(_, operand, _, _, indirectionIndex) and not isDef(_, _, operand, _, _, _) } or - TGlobalUse(Cpp::GlobalOrNamespaceVariable v, IRFunction f, int indirectionIndex) { + TGlobalUse(GlobalLikeVariable v, IRFunction f, int indirectionIndex) { // Represents a final "use" of a global variable to ensure that // the assignment to a global variable isn't ruled out as dead. exists(VariableAddressInstruction vai, int defIndex | @@ -162,7 +162,7 @@ private newtype TDefOrUseImpl = indirectionIndex = [0 .. defIndex] + 1 ) } or - TGlobalDefImpl(Cpp::GlobalOrNamespaceVariable v, IRFunction f, int indirectionIndex) { + TGlobalDefImpl(GlobalLikeVariable v, IRFunction f, int indirectionIndex) { // Represents the initial "definition" of a global variable when entering // a function body. exists(VariableAddressInstruction vai | @@ -458,7 +458,7 @@ class FinalParameterUse extends UseImpl, TFinalParameterUse { } class GlobalUse extends UseImpl, TGlobalUse { - Cpp::GlobalOrNamespaceVariable global; + GlobalLikeVariable global; IRFunction f; GlobalUse() { this = TGlobalUse(global, f, ind) } @@ -468,7 +468,7 @@ class GlobalUse extends UseImpl, TGlobalUse { override int getIndirection() { result = ind + 1 } /** Gets the global variable associated with this use. */ - Cpp::GlobalOrNamespaceVariable getVariable() { result = global } + GlobalLikeVariable getVariable() { result = global } /** Gets the `IRFunction` whose body is exited from after this use. */ IRFunction getIRFunction() { result = f } @@ -496,14 +496,14 @@ class GlobalUse extends UseImpl, TGlobalUse { } class GlobalDefImpl extends DefOrUseImpl, TGlobalDefImpl { - Cpp::GlobalOrNamespaceVariable global; + GlobalLikeVariable global; IRFunction f; int indirectionIndex; GlobalDefImpl() { this = TGlobalDefImpl(global, f, indirectionIndex) } /** Gets the global variable associated with this definition. */ - Cpp::GlobalOrNamespaceVariable getVariable() { result = global } + GlobalLikeVariable getVariable() { result = global } /** Gets the `IRFunction` whose body is evaluated after this definition. */ IRFunction getIRFunction() { result = f } @@ -760,13 +760,14 @@ private predicate variableWriteCand(IRBlock bb, int i, SourceVariable v) { } private predicate sourceVariableIsGlobal( - SourceVariable sv, Cpp::GlobalOrNamespaceVariable global, IRFunction func, int indirectionIndex + SourceVariable sv, GlobalLikeVariable global, IRFunction func, int indirectionIndex ) { exists(IRVariable irVar, BaseIRVariable base | sourceVariableHasBaseAndIndex(sv, base, indirectionIndex) and irVar = base.getIRVariable() and irVar.getEnclosingIRFunction() = func and - global = irVar.getAst() + global = irVar.getAst() and + not irVar instanceof IRDynamicInitializationFlag ) } @@ -919,7 +920,7 @@ class GlobalDef extends TGlobalDef, SsaDefOrUse { IRFunction getIRFunction() { result = global.getIRFunction() } /** Gets the global variable associated with this definition. */ - Cpp::GlobalOrNamespaceVariable getVariable() { result = global.getVariable() } + GlobalLikeVariable getVariable() { result = global.getVariable() } } class Phi extends TPhi, SsaDefOrUse { diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected index 0750c1af392..e913ac5e0fa 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected @@ -115,6 +115,16 @@ postWithInFlow | test.cpp:602:3:602:7 | access to array [post update] | PostUpdateNode should not be the target of local flow. | | test.cpp:608:3:608:4 | * ... [post update] | PostUpdateNode should not be the target of local flow. | | test.cpp:608:4:608:4 | p [inner post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:639:3:639:3 | x [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:646:3:646:3 | x [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:652:3:652:3 | x [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:653:3:653:3 | x [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:659:3:659:3 | x [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:660:3:660:3 | x [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:671:3:671:3 | s [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:681:3:681:3 | s [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:689:3:689:3 | s [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:690:3:690:3 | s [post update] | PostUpdateNode should not be the target of local flow. | viableImplInCallContextTooLarge uniqueParameterNodeAtPosition uniqueParameterNodePosition 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 36b78896179..5fae604f4d9 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -627,4 +627,66 @@ void test_def_via_phi_read(bool b) } intPointerSource(buffer); sink(buffer); // $ ast,ir -} \ No newline at end of file +} + +void test_static_local_1() { + static int x = source(); + sink(x); // $ ast,ir +} + +void test_static_local_2() { + static int x = source(); + x = 0; + sink(x); // clean +} + +void test_static_local_3() { + static int x = 0; + sink(x); // $ ir MISSING: ast + x = source(); +} + +void test_static_local_4() { + static int x = 0; + sink(x); // clean + x = source(); + x = 0; +} + +void test_static_local_5() { + static int x = 0; + sink(x); // $ ir MISSING: ast + x = 0; + x = source(); +} + +void test_static_local_6() { + static int s = source(); + static int* ptr_to_s = &s; + sink(*ptr_to_s); // $ ir MISSING: ast +} + +void test_static_local_7() { + static int s = source(); + s = 0; + static int* ptr_to_s = &s; + sink(*ptr_to_s); // clean +} + +void test_static_local_8() { + static int s; + static int* ptr_to_s = &s; + sink(*ptr_to_s); // $ ir MISSING: ast + + s = source(); +} + +void test_static_local_9() { + static int s; + static int* ptr_to_s = &s; + sink(*ptr_to_s); // clean + + s = source(); + s = 0; +} + diff --git a/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected b/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected index fc71b416281..6ba1cdf8ed7 100644 --- a/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected +++ b/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected @@ -4,7 +4,9 @@ uniqueType uniqueNodeLocation missingLocation uniqueNodeToString +| cpp11.cpp:50:15:50:16 | (no string representation) | Node should have one toString but has 0. | missingToString +| Nodes without toString: 1 | parameterCallable localFlowIsLocal readStepIsLocal 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 0ea73248a7d..dde3d703fc4 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 @@ -3,6 +3,7 @@ | nested.cpp:21:23:21:26 | fmt0 | The format string argument to snprintf should be constant to prevent security issues and other potential errors. | | nested.cpp:79:32:79:38 | call to get_fmt | The format string argument to diagnostic should be constant to prevent security issues and other potential errors. | | nested.cpp:87:18:87:20 | fmt | The format string argument to diagnostic should be constant to prevent security issues and other potential errors. | +| test.cpp:51:10:51:21 | call to make_message | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:57:12:57:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:60:12:60:21 | call to const_wash | The format string argument to printf should be constant to prevent security issues and other potential errors. | | test.cpp:61:12:61:26 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp index 75c02296d7e..f42d6835aa7 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp @@ -48,7 +48,7 @@ int main(int argc, char **argv) { printf(choose_message(argc - 1), argc - 1); // GOOD printf(messages[1]); // GOOD printf(message); // GOOD - printf(make_message(argc - 1)); // BAD [NOT DETECTED] + printf(make_message(argc - 1)); // BAD printf("Hello, World\n"); // GOOD printf(_("Hello, World\n")); // GOOD {