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

Author: erik-krogh

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

@@ -3,36 +3,57 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>Accessing paths controlled by users can allow an attacker to access unexpected resources. This
+<p>Accessing paths controlled by users can allow an attacker to access unexpected resources. This 
 can result in sensitive information being revealed or deleted, or an attacker being able to influence
 behavior by modifying unexpected files.</p>
 
-<p>Paths that are naively constructed from data controlled by a user may contain unexpected special characters,
-such as "..". Such a path may potentially point to any directory on the filesystem.</p>
+<p>Paths that are naively constructed from data controlled by a user may be absolute paths, or may contain
+unexpected special characters such as "..". Such a path could point anywhere on the file system.</p>
 
 </overview>
 <recommendation>
 
-<p>Validate user input before using it to construct a filepath. Ideally, follow these rules:</p>
+<p>Validate user input before using it to construct a file path.</p>
 
-<ul>
-<li>Do not allow more than a single "." character.</li>
-<li>Do not allow directory separators such as "/" or "\" (depending on the filesystem).</li>
-<li>Do not rely on simply replacing problematic sequences such as "../". For example, after applying this filter to
-".../...//" the resulting string would still be "../".</li>
-<li>Ideally use a whitelist of known good patterns.</li>
-</ul>
+<p>Common validation methods include checking that the normalized path is relative and does not contain
+any ".." components, or checking that the path is contained within a safe folder. The method you should use depends 
+on how the path is used in the application, and whether the path should be a single path component.
+</p>
+
+<p>If the path should be a single path component (such as a file name), you can check for the existence 
+of any path separators ("/" or "\"), or ".." sequences in the input, and reject the input if any are found.
+</p>
+
+<p>
+Note that removing "../" sequences is <i>not</i> sufficient, since the input could still contain a path separator
+followed by "..". For example, the input ".../...//" would still result in the string "../" if only "../" sequences
+are removed.
+</p>
+
+<p>Finally, the simplest (but most restrictive) option is to use an allow list of safe patterns and make sure that
+the user input matches one of these patterns.</p>
 
 </recommendation>
 <example>
 
-<p>In this example, a username and file are read from the arguments to main and then used to access a file in the
-user's home directory. However, a malicious user could enter a filename which contains special
-characters. For example, the string "../../etc/passwd" will result in the code reading the file located at
-"/home/[user]/../../etc/passwd", which is the system's password file. This could potentially allow them to
-access all the system's passwords.</p>
+<p>In this example, a file name is read from a user and then used to access a file. 
+However, a malicious user could enter a file name anywhere on the file system,
+such as "/etc/passwd" or "../../../etc/passwd".</p>
+
+<sample src="examples/TaintedPath.c" />
+
+<p>
+If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.
+</p>
+
+<sample src="examples/TaintedPathNormalize.c" />
+
+<p>
+If the input should be within a specific directory, you can check that the resolved path 
+is still contained within that directory.
+</p>
 
-<sample src="TaintedPath.c" />
+<sample src="examples/TaintedPathFolder.c" />
 
 </example>
 <references>
@@ -41,6 +62,7 @@ access all the system's passwords.</p>
 OWASP:
 <a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
 </li>
+<li>Rails: <a href="https://api.rubyonrails.org/classes/ActiveStorage/Filename.html#method-i-sanitized">ActiveStorage::Filename#sanitized</a>.</li>
 
 </references>
 </qhelp>

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

@@ -9,14 +9,4 @@ int main(int argc, char** argv) {
     // BAD: a string from the user is used in a filename
     fopen(fileName, "wb+");
   }
-
-  {
-    char fileBuffer[FILENAME_MAX] = "/home/";
-    char *fileName = fileBuffer;
-    size_t len = strlen(fileName);
-    // GOOD: use a fixed file
-    char* fixed = "jim/file.txt";
-    strncat(fileName+len, fixed, FILENAME_MAX-len-1);
-    fopen(fileName, "wb+");
-  }
 }

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

@@ -0,0 +1,26 @@
+#include <stdio.h>
+#include <string.h>
+
+int main(int argc, char** argv) {
+    char *userAndFile = argv[2];
+    char baseDir[PATH_MAX] = "/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) == NULL) {
+        perror("Error resolving path");
+        return 1;
+    }
+
+    // Check if the resolved path starts with the base directory
+    if (strncmp(baseDir, resolvedPath, strlen(baseDir)) != 0) {
+        return 1;
+    }
+
+    // GOOD: Path is within the intended directory
+    FILE *file = fopen(resolvedPath, "wb+");
+}

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

@@ -0,0 +1,18 @@
+#include <stdio.h>
+#include <string.h>
+
+int main(int argc, char** argv) {
+
+    char *userAndFile = argv[2];
+    // Check for invalid sequences in the user input
+    if (strstr(userAndFile, "..") || strchr(userAndFile, '/') || strchr(userAndFile, '\\')) {
+        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);
+    // 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/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) {
@@ -56,6 +56,44 @@ int main(int argc, char** argv) {
     void read(const char *fileName);
     read(argv[1]); // BAD
   }
+
+  {
+    char *userAndFile = argv[2];
+    // Check for invalid sequences in the user input
+    if (strstr(userAndFile, "..") || strchr(userAndFile, '/') || strchr(userAndFile, '\\')) {
+        // 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);
+    // 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/";
+    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) {
+        return 1;
+    }
+
+    // Check if the resolved path starts with the base directory
+    if (strncmp(baseDir, resolvedPath, strlen(baseDir)) != 0) {
+        return 1;
+    }
+
+    // GOOD: Path is within the intended directory
+    FILE *file = fopen(resolvedPath, "wb+");
+  }
 }
 
 void read(char *fileName) {