Merge pull request #16131 from erik-krogh/cpp-path

C++: Improve the cpp/path-injection qhelp

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

@@ -2,6 +2,8 @@ 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:69:14:69:20 | *access to array | provenance |  |
+| test.c:8:27:8:30 | **argv | test.c:80:25:80:31 | *access to array | provenance |  |
+| test.c:8:27:8:30 | **argv | test.c:88:22:88:28 | *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 |  |
@@ -11,6 +13,8 @@ edges
 | 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 |  |
+| test.c:80:25:80:31 | *access to array | test.c:84:11:84:20 | *fileBuffer | provenance | TaintFunction |
+| test.c:88:22:88:28 | *access to array | test.c:98:24:98:33 | *fileBuffer | provenance | TaintFunction |
 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 |
@@ -30,6 +34,10 @@ nodes
 | 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 |
+| test.c:80:25:80:31 | *access to array | semmle.label | *access to array |
+| test.c:84:11:84:20 | *fileBuffer | semmle.label | *fileBuffer |
+| test.c:88:22:88:28 | *access to array | semmle.label | *access to array |
+| test.c:98:24:98:33 | *fileBuffer | semmle.label | *fileBuffer |
 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) |
@@ -41,3 +49,5 @@ subpaths
 | 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) |
+| test.c:84:11:84:20 | fileBuffer | test.c:8:27:8:30 | **argv | test.c:84:11:84:20 | *fileBuffer | 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:98:24:98:33 | fileBuffer | test.c:8:27:8:30 | **argv | test.c:98:24:98:33 | *fileBuffer | 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) |

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

@@ -2,7 +2,7 @@
 // Associated with CWE-022: Improper Limitation of a Pathname to a Restricted Directory. http://cwe.mitre.org/data/definitions/22.html
 
 #include "stdlib.h"
-
+#define PATH_MAX 4096
 ///// Test code /////
 
 int main(int argc, char** argv) {
@@ -75,6 +75,55 @@ int main(int argc, char** argv) {
     read(0, buffer, 1024);
     fopen(buffer, "wb+"); // BAD [duplicated with both sources]
   }
+
+  {
+    char *userAndFile = argv[2];
+    char fileBuffer[PATH_MAX];
+    snprintf(fileBuffer, sizeof(fileBuffer), "/home/%s", userAndFile);
+    // BAD: a string from the user is used in a filename
+    fopen(fileBuffer, "wb+");
+  }
+
+  {
+    char *fileName = argv[2];
+    // Check for invalid sequences in the user input
+    if (strstr(fileName , "..") || strchr(fileName , '/') || strchr(fileName , '\\')) {
+        printf("Invalid filename.\n");
+        return 1;
+    }
+
+    char fileBuffer[PATH_MAX];
+    snprintf(fileBuffer, sizeof(fileBuffer), "/home/user/files/%s", fileName);
+    // GOOD: We know that the filename is safe and stays within the public folder. But we currently get an FP here.
+    FILE *file = fopen(fileBuffer, "wb+");
+  }
+
+  {
+    char *userAndFile = argv[2];
+    const char *baseDir = "/home/user/public/";
+    char fullPath[PATH_MAX];
+
+    // Attempt to concatenate the base directory and the user-supplied path
+    snprintf(fullPath, sizeof(fullPath), "%s%s", baseDir, userAndFile);
+
+    // Resolve the absolute path, normalizing any ".." or "."
+    char *resolvedPath = realpath(fullPath, 0); // <- we're using `NULL` in the example, but 0 here to get it to compile. Same for next line.
+    if (resolvedPath == 0) {
+        perror("Error resolving path");
+        return 1;
+    }
+
+    // Check if the resolved path starts with the base directory
+    if (strncmp(baseDir, resolvedPath, strlen(baseDir)) != 0) {
+        free(resolvedPath);
+        return 1;
+    }
+
+    // GOOD: Path is within the intended directory
+    FILE *file = fopen(resolvedPath, "wb+");
+    free(resolvedPath);
+    
+  }
 }
 
 void readFile(char *fileName) {