Merge pull request #16328 from geoffw0/pathinj

C++: Reduce duplication from cpp/path-injection

cpp/ql/src/Security/CWE/CWE-022/TaintedPath.ql

@@ -88,6 +88,11 @@ module TaintedPathConfig implements DataFlow::ConfigSig {
       hasUpperBoundsCheck(checkedVar)
     )
   }
+
+  predicate isBarrierOut(DataFlow::Node node) {
+    // make sinks barriers so that we only report the closest instance
+    isSink(node)
+  }
 }
 
 module TaintedPath = TaintTracking::Global<TaintedPathConfig>;

cpp/ql/src/change-notes/2024-04-25-path-injection.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Uncontrolled data used in path expression" query (`cpp/path-injection`) query produces fewer near-duplicate results.

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/TaintedPath.expected

@@ -1,11 +1,16 @@
 edges
 | test.c:8:27:8:30 | **argv | test.c:9:23:9:29 | *access to array | provenance |  |
 | test.c:8:27:8:30 | **argv | test.c:31:22:31:28 | *access to array | provenance |  |
-| test.c:8:27:8:30 | **argv | test.c:57:10:57:16 | *access to array | provenance |  |
+| test.c:8:27:8:30 | **argv | test.c:69:14:69:20 | *access to array | provenance |  |
 | test.c:9:23:9:29 | *access to array | test.c:17:11:17:18 | *fileName | provenance | TaintFunction |
 | test.c:31:22:31:28 | *access to array | test.c:32:11:32:18 | *fileName | provenance |  |
 | test.c:37:17:37:24 | scanf output argument | test.c:38:11:38:18 | *fileName | provenance |  |
 | test.c:43:17:43:24 | scanf output argument | test.c:44:11:44:18 | *fileName | provenance |  |
+| test.c:48:21:48:26 | *call to getenv | test.c:48:21:48:26 | *call to getenv | provenance |  |
+| test.c:48:21:48:26 | *call to getenv | test.c:49:11:49:17 | *tainted | provenance |  |
+| test.c:54:21:54:26 | *call to getenv | test.c:55:11:55:16 | *buffer | provenance | TaintFunction |
+| test.c:74:13:74:18 | read output argument | test.c:76:11:76:16 | *buffer | provenance |  |
+| test.c:75:13:75:18 | read output argument | test.c:76:11:76:16 | *buffer | provenance |  |
 nodes
 | test.c:8:27:8:30 | **argv | semmle.label | **argv |
 | test.c:9:23:9:29 | *access to array | semmle.label | *access to array |
@@ -16,11 +21,23 @@ nodes
 | test.c:38:11:38:18 | *fileName | semmle.label | *fileName |
 | test.c:43:17:43:24 | scanf output argument | semmle.label | scanf output argument |
 | test.c:44:11:44:18 | *fileName | semmle.label | *fileName |
-| test.c:57:10:57:16 | *access to array | semmle.label | *access to array |
+| test.c:48:21:48:26 | *call to getenv | semmle.label | *call to getenv |
+| test.c:48:21:48:26 | *call to getenv | semmle.label | *call to getenv |
+| test.c:49:11:49:17 | *tainted | semmle.label | *tainted |
+| test.c:54:21:54:26 | *call to getenv | semmle.label | *call to getenv |
+| test.c:55:11:55:16 | *buffer | semmle.label | *buffer |
+| test.c:69:14:69:20 | *access to array | semmle.label | *access to array |
+| test.c:74:13:74:18 | read output argument | semmle.label | read output argument |
+| test.c:75:13:75:18 | read output argument | semmle.label | read output argument |
+| test.c:76:11:76:16 | *buffer | semmle.label | *buffer |
 subpaths
 #select
 | test.c:17:11:17:18 | fileName | test.c:8:27:8:30 | **argv | test.c:17:11:17:18 | *fileName | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:8:27:8:30 | **argv | user input (a command-line argument) |
 | test.c:32:11:32:18 | fileName | test.c:8:27:8:30 | **argv | test.c:32:11:32:18 | *fileName | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:8:27:8:30 | **argv | user input (a command-line argument) |
 | test.c:38:11:38:18 | fileName | test.c:37:17:37:24 | scanf output argument | test.c:38:11:38:18 | *fileName | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:37:17:37:24 | scanf output argument | user input (value read by scanf) |
 | test.c:44:11:44:18 | fileName | test.c:43:17:43:24 | scanf output argument | test.c:44:11:44:18 | *fileName | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:43:17:43:24 | scanf output argument | user input (value read by scanf) |
-| test.c:57:10:57:16 | access to array | test.c:8:27:8:30 | **argv | test.c:57:10:57:16 | *access to array | This argument to a file access function is derived from $@ and then passed to read(fileName), which calls fopen(filename). | test.c:8:27:8:30 | **argv | user input (a command-line argument) |
+| test.c:49:11:49:17 | tainted | test.c:48:21:48:26 | *call to getenv | test.c:49:11:49:17 | *tainted | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:48:21:48:26 | *call to getenv | user input (an environment variable) |
+| test.c:55:11:55:16 | buffer | test.c:54:21:54:26 | *call to getenv | test.c:55:11:55:16 | *buffer | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:54:21:54:26 | *call to getenv | user input (an environment variable) |
+| test.c:69:14:69:20 | access to array | test.c:8:27:8:30 | **argv | test.c:69:14:69:20 | *access to array | This argument to a file access function is derived from $@ and then passed to readFile(fileName), which calls fopen(filename). | test.c:8:27:8:30 | **argv | user input (a command-line argument) |
+| test.c:76:11:76:16 | buffer | test.c:74:13:74:18 | read output argument | test.c:76:11:76:16 | *buffer | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:74:13:74:18 | read output argument | user input (buffer read by read) |
+| test.c:76:11:76:16 | buffer | test.c:75:13:75:18 | read output argument | test.c:76:11:76:16 | *buffer | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:75:13:75:18 | read output argument | user input (buffer read by read) |

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/stdlib.h

@@ -6,6 +6,7 @@
 typedef struct {} FILE;
 #define FILENAME_MAX 1000
 typedef unsigned long size_t;
+typedef signed long ssize_t;
 
 FILE *fopen(const char *filename, const char *mode);
 int sprintf(char *s, const char *format, ...);
@@ -15,3 +16,4 @@ int scanf(const char *format, ...);
 void *malloc(size_t size);
 double strtod(const char *ptr, char **endptr);
 char *getenv(const char *name);
+ssize_t read(int fd, void *buffer, size_t count);

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/test.c

@@ -7,7 +7,7 @@
 
 int main(int argc, char** argv) {
   char *userAndFile = argv[2];
-  
+
   {
     char fileBuffer[FILENAME_MAX] = "/home/";
     char *fileName = fileBuffer;
@@ -44,6 +44,18 @@ int main(int argc, char** argv) {
     fopen(fileName, "wb+"); // BAD
   }
 
+  {
+    char *tainted = getenv("A_STRING");
+    fopen(tainted, "wb+"); // BAD
+  }
+
+  {
+    char buffer[1024];
+    strncpy(buffer, getenv("A_STRING"), 1024);
+    fopen(buffer, "wb+"); // BAD
+    fopen(buffer, "wb+"); // (we don't want a duplicate result here)
+  }
+
   {
     char *aNumber = getenv("A_NUMBER");
     double number = strtod(aNumber, 0);
@@ -53,11 +65,18 @@ int main(int argc, char** argv) {
   }
 
   {
-    void read(const char *fileName);
-    read(argv[1]); // BAD
+    void readFile(const char *fileName);
+    readFile(argv[1]); // BAD
+  }
+
+  {
+    char buffer[1024];
+    read(0, buffer, 1024);
+    read(0, buffer, 1024);
+    fopen(buffer, "wb+"); // BAD [duplicated with both sources]
   }
 }
 
-void read(char *fileName) {
+void readFile(char *fileName) {
   fopen(fileName, "wb+");
 }