-Ensure that all execution paths deallocate the allocated memory at most once. If possible, reassign -the pointer to a null value after deallocating it. This will prevent double-free vulnerabilities since -most deallocation functions will perform a null-pointer check before attempting to deallocate the memory. +Ensure that all execution paths deallocate the allocated memory at most once. In complex cases it may +help to reassign a pointer to a null value after deallocating it. This will prevent double-free vulnerabilities +since most deallocation functions will perform a null-pointer check before attempting to deallocate memory.
+In the following example, buff is allocated and then freed twice:
+
+Reviewing the code above, the issue can be fixed by simply deleting the additional call to
+free(buff).
+
+In the next example, task may be deleted twice, if an exception occurs inside the try
+block after the first delete:
+
+The problem can be solved by assigning a null value to the pointer after the first delete, as
+calling delete a second time on the null pointer is harmless.
+
Review the format and arguments expected by the highlighted function calls. Update either -the format or the arguments so that the expected number of arguments are passed to the +
Review the format and arguments expected by the highlighted function calls. Update either +the format or the arguments so that the expected number of arguments are passed to the function.
@@ -30,11 +30,8 @@ function.Each call to the printf function or a related function should include
-the type and sequence of arguments defined by the format. If the function is passed arguments
+the type and sequence of arguments defined by the format. If the function is passed arguments
of a different type or in a different sequence then the arguments are reinterpreted to fit the type and sequence expected, resulting in unpredictable behavior.
Review the format and arguments expected by the highlighted function calls. Update either -the format or the arguments so that the expected type and sequence of arguments are passed to +
Review the format and arguments expected by the highlighted function calls. Update either +the format or the arguments so that the expected type and sequence of arguments are passed to the function.
In the following example, the wrong format specifier is given for an integer format argument:
+ +The corrected version uses %i as the format specifier for the integer format argument:
Carefully inspect the flagged expressions. Consider the intent in the code logic, and decide whether it is necessary to change the not operator.
Here is an example of this issue and how it can be fixed:
+ +In other cases, particularly when the expressions have bool type, the fix may instead be of the form a && !b.
The standard library function strncpy copies a source string to a destination buffer. The third argument defines the maximum number of characters to copy and should be less than
+
The standard library function strncpy copies a source string to a destination buffer. The third argument defines the maximum number of characters to copy and should be less than
or equal to the size of the destination buffer. Calls of the form strncpy(dest, src, strlen(src)) or strncpy(dest, src, sizeof(src)) incorrectly set the third argument to the size of the source buffer. Executing a call of this type may cause a buffer overflow. Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.
strncpy(
not the source buffer.
-
+In the following examples, the size of the source buffer is incorrectly used as a parameter to strncpy:
+The corrected version uses the size of the destination buffer, or a variable containing the size of the destination buffer as the size parameter to strncpy:
+
+
+
-cplusplus.com: strncpy.
+cplusplus.com: strncpy.
I. Gerg. An Overview and Example of the Buffer-Overflow Exploit. IANewsletter vol 7 no 4. 2005.
diff --git a/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgsBad.cpp b/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgsBad.cpp
new file mode 100644
index 00000000000..952550b2638
--- /dev/null
+++ b/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgsBad.cpp
@@ -0,0 +1,9 @@
+char src[256];
+char dest1[128];
+
+...
+
+strncpy(dest1, src, sizeof(src)); // wrong: size of dest should be used
+
+char *dest2 = (char *)malloc(sz1 + sz2 + sz3);
+strncpy(dest2, src, strlen(src)); // wrong: size of dest should be used
diff --git a/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgsGood.cpp b/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgsGood.cpp
new file mode 100644
index 00000000000..22fc4ebd222
--- /dev/null
+++ b/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgsGood.cpp
@@ -0,0 +1,10 @@
+char src[256];
+char dest1[128];
+
+...
+
+strncpy(dest1, src, sizeof(dest1)); // correct
+
+size_t destSize = sz1 + sz2 + sz3;
+char *dest2 = (char *)malloc(destSize);
+strncpy(dest2, src, destSize); // correct
diff --git a/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.c b/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.c
deleted file mode 100644
index 63856888ebb..00000000000
--- a/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.c
+++ /dev/null
@@ -1,22 +0,0 @@
-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);
- // 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+");
- }
-}
diff --git a/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp b/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp
index eba2ede58f5..4d6238ac335 100644
--- a/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp
+++ b/cpp/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp
@@ -3,36 +3,57 @@
"qhelp.dtd">
-Accessing paths controlled by users can allow an attacker to access unexpected resources. This
+
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.
-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.
+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.
-Validate user input before using it to construct a filepath. Ideally, follow these rules:
+Validate user input before using it to construct a file path.
-
-- Do not allow more than a single "." character.
-- Do not allow directory separators such as "/" or "\" (depending on the filesystem).
-- Do not rely on simply replacing problematic sequences such as "../". For example, after applying this filter to
-".../...//" the resulting string would still be "../".
-- Ideally use a whitelist of known good patterns.
-
+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.
+
+
+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.
+
+
+
+Note that removing "../" sequences is not 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.
+
+
+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.
-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.
+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".
-
+If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.
+
+
+
+If the input should be within a specific directory, you can check that the resolved path
+is still contained within that directory.
+
+
+
@@ -41,6 +62,7 @@ access all the system's passwords.
OWASP:
Path Traversal.