Merge branch 'main' into reuse-even-more-nodes

Author: MathiasVP

cpp/ql/lib/change-notes/2023-08-24-no-taint-argv-indirections.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Only the 2 level indirection of `argv` (corresponding to `**argv`) is consided for `FlowSource`.

cpp/ql/lib/semmle/code/cpp/security/FlowSources.qll

@@ -53,7 +53,7 @@ private class ArgvSource extends LocalFlowSource {
     exists(Function main, Parameter argv |
       main.hasGlobalName("main") and
       main.getParameter(1) = argv and
-      this.asParameter(_) = argv
+      this.asParameter(2) = argv
     )
   }
 

cpp/ql/src/Likely Bugs/Format/NonConstantFormat.ql

@@ -105,8 +105,6 @@ predicate isNonConst(DataFlow::Node node, boolean isIndirect) {
     or
     e instanceof NewArrayExpr
     or
-    e instanceof AssignExpr
-    or
     exists(Variable v | v = e.(VariableAccess).getTarget() |
       v.getType().(ArrayType).getBaseType() instanceof CharType and
       exists(AssignExpr ae |

cpp/ql/src/Security/CWE/CWE-089/SqlTainted.ql

@@ -14,41 +14,55 @@
 
 import cpp
 import semmle.code.cpp.security.Security
+import semmle.code.cpp.security.FlowSources
 import semmle.code.cpp.security.FunctionWithWrappers
-import semmle.code.cpp.ir.dataflow.internal.DefaultTaintTrackingImpl
-import TaintedWithPath
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.dataflow.TaintTracking
+import SqlTainted::PathGraph
 
 class SqlLikeFunction extends FunctionWithWrappers {
   SqlLikeFunction() { sqlArgument(this.getName(), _) }
 
   override predicate interestingArg(int arg) { sqlArgument(this.getName(), arg) }
 }
 
-class Configuration extends TaintTrackingConfiguration {
-  override predicate isSink(Element tainted) {
-    exists(SqlLikeFunction runSql | runSql.outermostWrapperFunctionCall(tainted, _))
+Expr asSinkExpr(DataFlow::Node node) {
+  result = node.asIndirectArgument()
+  or
+  // We want the conversion so we only get one node for the expression
+  result = node.asConvertedExpr()
+}
+
+module SqlTaintedConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { node instanceof FlowSource }
+
+  predicate isSink(DataFlow::Node node) {
+    exists(SqlLikeFunction runSql | runSql.outermostWrapperFunctionCall(asSinkExpr(node), _))
   }
 
-  override predicate isBarrier(Expr e) {
-    super.isBarrier(e)
-    or
-    e.getUnspecifiedType() instanceof IntegralType
-    or
+  predicate isBarrier(DataFlow::Node node) {
+    node.asExpr().getUnspecifiedType() instanceof IntegralType
+  }
+
+  predicate isBarrierIn(DataFlow::Node node) {
     exists(SqlBarrierFunction sql, int arg, FunctionInput input |
-      e = sql.getACallToThisFunction().getArgument(arg) and
+      node.asIndirectArgument() = sql.getACallToThisFunction().getArgument(arg) and
       input.isParameterDeref(arg) and
       sql.barrierSqlArgument(input, _)
     )
   }
 }
 
+module SqlTainted = TaintTracking::Global<SqlTaintedConfig>;
+
 from
-  SqlLikeFunction runSql, Expr taintedArg, Expr taintSource, PathNode sourceNode, PathNode sinkNode,
-  string taintCause, string callChain
+  SqlLikeFunction runSql, Expr taintedArg, FlowSource taintSource, SqlTainted::PathNode sourceNode,
+  SqlTainted::PathNode sinkNode, string callChain
 where
   runSql.outermostWrapperFunctionCall(taintedArg, callChain) and
-  taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and
-  isUserInput(taintSource, taintCause)
+  SqlTainted::flowPath(sourceNode, sinkNode) and
+  taintedArg = asSinkExpr(sinkNode.getNode()) and
+  taintSource = sourceNode.getNode()
 select taintedArg, sourceNode, sinkNode,
   "This argument to a SQL query function is derived from $@ and then passed to " + callChain + ".",
-  taintSource, "user input (" + taintCause + ")"
+  taintSource, "user input (" + taintSource.getSourceType() + ")"

cpp/ql/src/change-notes/2023-08-24-no-taint-argv-indirections.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Some queries that had repeated results corresponding to different levels of indirection for `argv` now only have a single result.

cpp/ql/src/change-notes/2023-08-24-remove-non-constant-assign-sources.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/non-constant-format` query no longer considers an assignment on the right-hand side of another assignment to be a source of non-constant format strings. As a result, the query may now produce fewer results.

cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.ql

@@ -40,7 +40,7 @@ module WordexpTaintConfig implements DataFlow::ConfigSig {
 
   predicate isSink(DataFlow::Node sink) {
     exists(FunctionCall fc | fc.getTarget() instanceof WordexpFunction |
-      fc.getArgument(0) = sink.asExpr() and
+      fc.getArgument(0) = sink.asIndirectArgument(1) and
       not isCommandSubstitutionDisabled(fc)
     )
   }

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-078/WordexpTainted.expected

@@ -1,16 +1,8 @@
 edges
-| test.cpp:22:27:22:30 | argv | test.cpp:29:13:29:20 | filePath |
-| test.cpp:22:27:22:30 | argv | test.cpp:29:13:29:20 | filePath |
-| test.cpp:22:27:22:30 | argv indirection | test.cpp:29:13:29:20 | filePath |
-| test.cpp:22:27:22:30 | argv indirection | test.cpp:29:13:29:20 | filePath |
+| test.cpp:22:27:22:30 | argv indirection | test.cpp:29:13:29:20 | filePath indirection |
 nodes
-| test.cpp:22:27:22:30 | argv | semmle.label | argv |
 | test.cpp:22:27:22:30 | 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 |
+| test.cpp:29:13:29:20 | filePath indirection | semmle.label | filePath indirection |
 subpaths
 #select
-| test.cpp:29:13:29:20 | filePath | test.cpp:22:27:22:30 | 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:22:27:22:30 | 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:22:27:22:30 | 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:22:27:22:30 | 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 indirection | test.cpp:22:27:22:30 | argv indirection | test.cpp:29:13:29:20 | filePath indirection | Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection. |

cpp/ql/test/library-tests/ir/ir/PrintAST.expected

@@ -1798,6 +1798,23 @@ ir.c:
 #   10|                 Type = [CTypedefType] MyCoords
 #   10|                 ValueCategory = lvalue
 #   11|     getStmt(3): [ReturnStmt] return ...
+#   13| [TopLevelFunction] void CStyleCast(void*)
+#   13|   <params>: 
+#   13|     getParameter(0): [Parameter] src
+#   13|         Type = [VoidPointerType] void *
+#   14|   getEntryPoint(): [BlockStmt] { ... }
+#   15|     getStmt(0): [DeclStmt] declaration
+#   15|       getDeclarationEntry(0): [VariableDeclarationEntry] definition of dst
+#   15|           Type = [CharPointerType] char *
+#   15|         getVariable().getInitializer(): [Initializer] initializer for dst
+#   15|           getExpr(): [VariableAccess] src
+#   15|               Type = [VoidPointerType] void *
+#   15|               ValueCategory = prvalue(load)
+#   15|           getExpr().getFullyConverted(): [CStyleCast] (char *)...
+#   15|               Conversion = [PointerConversion] pointer conversion
+#   15|               Type = [CharPointerType] char *
+#   15|               ValueCategory = prvalue
+#   16|     getStmt(1): [ReturnStmt] return ...
 ir.cpp:
 #    1| [TopLevelFunction] void Constants()
 #    1|   <params>: 

cpp/ql/test/library-tests/ir/ir/ir.c

@@ -9,3 +9,10 @@ void MyCoordsTest(int pos) {
   coords.x = coords.y = pos + 1;
   coords.x = getX(&coords);
 }
+
+void CStyleCast(void *src)
+{
+    char *dst = (char*)src;
+}
+
+// semmle-extractor-options: --microsoft