apply suggestions from code review, and the examples to the test

Author: erik-krogh

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

@@ -2,11 +2,9 @@ int main(int argc, char** argv) {
   char *userAndFile = argv[2];
 
   {
-    char fileBuffer[FILENAME_MAX] = "/home/";
-    char *fileName = fileBuffer;
-    size_t len = strlen(fileName);
-    strncat(fileName+len, userAndFile, FILENAME_MAX-len-1);
+    char fileBuffer[PATH_MAX];
+    snprintf(fileBuffer, sizeof(fileBuffer), "/home/%s", userAndFile);
     // BAD: a string from the user is used in a filename
-    fopen(fileName, "wb+");
+    fopen(fileBuffer, "wb+");
   }
 }

cpp/ql/src/Security/CWE/CWE-022/examples/TaintedPathFolder.c

@@ -3,7 +3,7 @@
 
 int main(int argc, char** argv) {
     char *userAndFile = argv[2];
-    char baseDir[PATH_MAX] = "/home/user/public/";
+    const char *baseDir = "/home/user/public/";
     char fullPath[PATH_MAX];
 
     // Attempt to concatenate the base directory and the user-supplied path

cpp/ql/src/Security/CWE/CWE-022/examples/TaintedPathNormalize.c

@@ -2,13 +2,15 @@
 #include <string.h>
 
 int main(int argc, char** argv) {
-
-    char *userAndFile = argv[2];
+    char *fileName = argv[2];
     // Check for invalid sequences in the user input
-    if (strstr(userAndFile, "..") || strchr(userAndFile, '/') || strchr(userAndFile, '\\')) {
+    if (strstr(fileName , "..") || strchr(fileName , '/') || strchr(fileName , '\\')) {
         printf("Invalid filename.\n");
         return 1;
     }
 
-    // use `userAndFile` as a safe filename
+    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
+    FILE *file = fopen(fileBuffer, "wb+");
 }

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

@@ -3,6 +3,7 @@ edges
 | 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 |  |
@@ -12,7 +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:91:24:91:33 | *fileBuffer | provenance | TaintFunction |
+| 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 |
@@ -33,7 +35,9 @@ nodes
 | 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:91:24:91:33 | *fileBuffer | semmle.label | *fileBuffer |
+| 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) |
@@ -45,4 +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:91:24:91:33 | fileBuffer | test.c:8:27:8:30 | **argv | test.c:91:24:91: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) |
+| 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

@@ -78,40 +78,51 @@ int main(int argc, char** argv) {
 
   {
     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(userAndFile, "..") || strchr(userAndFile, '/') || strchr(userAndFile, '\\')) {
-        // printf("Invalid filename.\n");
+    if (strstr(fileName , "..") || strchr(fileName , '/') || strchr(fileName , '\\')) {
+        printf("Invalid filename.\n");
         return 1;
     }
 
-    char fileBuffer[FILENAME_MAX] = "/home/user/files/";
-    // Ensure buffer overflow is prevented
-    strncat(fileBuffer, userAndFile, FILENAME_MAX - strlen(fileBuffer) - 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];
-    char baseDir[PATH_MAX] = "/home/user/public/";
+    const char *baseDir = "/home/user/public/";
     char fullPath[PATH_MAX];
-    char resolvedPath[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 "."
-    if (realpath(fullPath, resolvedPath) == 0) {
+    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);
+    
   }
 }