Merge branch 'github:main' into main-1

Author: krishnprakash

cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql

@@ -49,11 +49,17 @@ predicate interestingConcatenation(DataFlow::Node incoming, DataFlow::Node outgo
     call.getTarget() = op and
     op.hasQualifiedName("std", "operator+") and
     op.getType().(UserType).hasQualifiedName("std", "basic_string") and
-    incoming.asIndirectArgument() = call.getArgument(1) and // left operand
+    incoming.asIndirectArgument() = call.getArgument(1) and // right operand
     call = outgoing.asInstruction().getUnconvertedResultExpression()
   )
 }
 
+/**
+ * A state will represent the most recent concatenation that occurred in the data flow.
+ *  - `TConcatState` if the concetenation has not yet occurred.
+ *  - `TExecState(incoming, outgoing)`, representing the concatenation of data from `incoming`
+ *    into result `outgoing`.
+ */
 newtype TState =
   TConcatState() or
   TExecState(DataFlow::Node incoming, DataFlow::Node outgoing) {
@@ -74,7 +80,9 @@ class ExecState extends TExecState {
 
   DataFlow::Node getOutgoingNode() { result = outgoing }
 
-  /** Holds if this is a possible `ExecState` for `sink`. */
+  /**
+   * Holds if this is a possible `ExecState` at `sink`, that is, if `outgoing` flows to `sink`.
+   */
   predicate isFeasibleForSink(DataFlow::Node sink) { ExecState::flow(outgoing, sink) }
 
   string toString() { result = "ExecState" }
@@ -110,6 +118,12 @@ module ExecStateConfig implements DataFlow::ConfigSig {
 
 module ExecState = TaintTracking::Global<ExecStateConfig>;
 
+/**
+ * A full `TaintTracking` configuration from source to concatenation to sink, using a flow
+ * state to remember the concatenation. It's important that we track flow to the sink even though
+ * as soon as we reach the concatenation we know it will get there (due to the check of
+ * `isFeasibleForSink`), because this way we get a complete flow path.
+ */
 module ExecTaintConfig implements DataFlow::StateConfigSig {
   class FlowState = TState;
 

cpp/ql/test/query-tests/Security/CWE/CWE-078/semmle/ExecTainted/ExecTainted.expected

@@ -14,7 +14,7 @@ extern void encodeShellString(char *shellStr, int maxChars, const char* cStr);
 
 int main(int argc, char** argv) {
   char *userName = argv[2];
-  
+
   {
     // BAD: a string from the user is injected directly into
     // a command.
@@ -23,10 +23,10 @@ int main(int argc, char** argv) {
     system(command1);
   }
 
-  {  
+  {
     // GOOD: the user string is encoded by a library routine.
     char userNameQuoted[1000] = {0};
-    encodeShellString(userNameQuoted, 1000, userName); 
+    encodeShellString(userNameQuoted, 1000, userName);
     char command2[1000] = {0};
     sprintf(command2, "userinfo -v %s", userNameQuoted);
     system(command2);
@@ -36,16 +36,16 @@ int main(int argc, char** argv) {
 void test2(char* arg2) {
   // GOOD?: the user string is the *first* part of the command, like $CC in many environments
   char *envCC = getenv("CC");
-  
+
   char command[1000];
-  sprintf("%s %s", envCC, arg2);
+  sprintf(command, "%s %s", envCC, arg2);
   system(command);
 }
 
 void test3(char* arg1) {
   // GOOD?: the user string is a `$CFLAGS` environment variable
   char *envCflags = getenv("CFLAGS");
-  
+
   char command[1000];
   sprintf(command, "%s %s", arg1, envCflags);
   system(command);
@@ -54,6 +54,7 @@ void test3(char* arg1) {
 typedef unsigned long size_t;
 typedef void FILE;
 size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
+char *strncpy(char *s1, const char *s2, size_t n);
 char *strncat(char *s1, const char *s2, size_t n);
 
 void test4(FILE *f) {
@@ -160,7 +161,7 @@ void test15(FILE *f) {
   fread(temp, 1, 10, f);
 
   int x = atoi(temp);
-  
+
   char temp2[10];
   sprintf(temp2, "%d", x);
     sprintf(command, "tail -n %s foo.log", temp2);
@@ -222,4 +223,42 @@ void test19(FILE *f) {
   execl("/bin/sh", "sh", "-c", command);
 }
 
+void test20() {
+  // BAD: the user strings `var_b`, `var_c` are injected directly into a command
+  char buffer[1024 * 4];
+
+  strncpy(buffer, getenv("var_a"), 1024);
+  strncat(buffer, getenv("var_b"), 1024);
+  strncat(buffer, getenv("var_c"), 1024);
+  strncat(buffer, " ", 1024);
+  system(buffer);
+}
+
+void test21() {
+  // BAD: the user strings `var_b`, `var_c` are injected directly into a command
+  char buffer1[1024];
+  char buffer2[1024];
+
+  sprintf(buffer1, "%s %s",
+    getenv("var_a"),
+    getenv("var_b"));
+  sprintf(buffer2, "%s %s %s",
+    " ",
+    buffer1,
+    getenv("var_c"));
+  system(buffer2);
+}
+
+void test22() {
+  // BAD: the user strings `var_a` are injected directly into a command
+  char buffer[1024 * 11];
+  int i;
+
+  strncpy(buffer, "command ", 1024);
+  for (i = 0; i < 10; i++) {
+    strncat(buffer, getenv("var_a"), 1024);
+  }
+  system(buffer);
+}
+
 // open question: do we want to report certain sources even when they're the start of the string?

cpp/ql/test/query-tests/Security/CWE/CWE-078/semmle/ExecTainted/test.cpp

@@ -68,7 +68,6 @@
 
 ### Minor Analysis Improvements
 
-* .NET 9 is now required to build the C# extractor.
 * The Models as Data models for .NET 8 Runtime now include generated models for higher order methods.
 
 ## 3.1.0

csharp/ql/lib/CHANGELOG.md

@@ -2,5 +2,4 @@
 
 ### Minor Analysis Improvements
 
-* .NET 9 is now required to build the C# extractor.
 * The Models as Data models for .NET 8 Runtime now include generated models for higher order methods.

csharp/ql/lib/change-notes/released/3.1.1.md

@@ -734,15 +734,15 @@ private predicate variableReadPseudo(ControlFlow::BasicBlock bb, int i, Ssa::Sou
 }
 
 pragma[noinline]
-private predicate adjacentDefRead(
+deprecated private predicate adjacentDefRead(
   Definition def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2,
   SsaInput::SourceVariable v
 ) {
   Impl::adjacentDefRead(def, bb1, i1, bb2, i2) and
   v = def.getSourceVariable()
 }
 
-private predicate adjacentDefReachesRead(
+deprecated private predicate adjacentDefReachesRead(
   Definition def, SsaInput::SourceVariable v, SsaInput::BasicBlock bb1, int i1,
   SsaInput::BasicBlock bb2, int i2
 ) {
@@ -760,18 +760,7 @@ private predicate adjacentDefReachesRead(
   )
 }
 
-/** Same as `adjacentDefRead`, but skips uncertain reads. */
-pragma[nomagic]
-private predicate adjacentDefSkipUncertainReads(
-  Definition def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2
-) {
-  exists(SsaInput::SourceVariable v |
-    adjacentDefReachesRead(def, v, bb1, i1, bb2, i2) and
-    SsaInput::variableRead(bb2, i2, v, true)
-  )
-}
-
-private predicate adjacentDefReachesUncertainRead(
+deprecated private predicate adjacentDefReachesUncertainRead(
   Definition def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2
 ) {
   exists(SsaInput::SourceVariable v |
@@ -933,10 +922,8 @@ private module Cached {
    */
   cached
   predicate firstReadSameVar(Definition def, ControlFlow::Node cfn) {
-    exists(ControlFlow::BasicBlock bb1, int i1, ControlFlow::BasicBlock bb2, int i2 |
-      def.definesAt(_, bb1, i1) and
-      adjacentDefSkipUncertainReads(def, bb1, i1, bb2, i2) and
-      cfn = bb2.getNode(i2)
+    exists(ControlFlow::BasicBlock bb, int i |
+      Impl::firstUse(def, bb, i, true) and cfn = bb.getNode(i)
     )
   }
 
@@ -947,25 +934,17 @@ private module Cached {
    */
   cached
   predicate adjacentReadPairSameVar(Definition def, ControlFlow::Node cfn1, ControlFlow::Node cfn2) {
-    exists(ControlFlow::BasicBlock bb1, int i1, ControlFlow::BasicBlock bb2, int i2 |
+    exists(
+      ControlFlow::BasicBlock bb1, int i1, ControlFlow::BasicBlock bb2, int i2,
+      Ssa::SourceVariable v
+    |
+      Impl::ssaDefReachesRead(v, def, bb1, i1) and
+      Impl::adjacentUseUse(bb1, i1, bb2, i2, v, true) and
       cfn1 = bb1.getNode(i1) and
-      variableReadActual(bb1, i1, _) and
-      adjacentDefSkipUncertainReads(def, bb1, i1, bb2, i2) and
       cfn2 = bb2.getNode(i2)
     )
   }
 
-  cached
-  predicate lastRefBeforeRedef(Definition def, ControlFlow::BasicBlock bb, int i, Definition next) {
-    Impl::lastRefRedef(def, bb, i, next) and
-    not SsaInput::variableRead(bb, i, def.getSourceVariable(), false)
-    or
-    exists(SsaInput::BasicBlock bb0, int i0 |
-      Impl::lastRefRedef(def, bb0, i0, next) and
-      adjacentDefReachesUncertainRead(def, bb, i, bb0, i0)
-    )
-  }
-
   cached
   Definition uncertainWriteDefinitionInput(UncertainWriteDefinition def) {
     Impl::uncertainWriteDefinitionInput(def, result)