-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.
+
An alternative solution is to allow log_with_timestamp to accept format arguments:
In this formulation, the non-constant format string to printf has been replaced with
-a non-constant format string to vprintf. Semmle will no longer consider the body of
+a non-constant format string to vprintf. The analysis will no longer consider the body of
log_with_timestamp to be a problem, and will instead check that every call to
log_with_timestamp passes a constant format string.
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.
-The expression highlighted by this rule creates a buffer that is of insufficient size to contain -the data being copied. This makes the code vulnerable to buffer overflow which can result in anything from a segmentation fault to a security vulnerability (particularly if the array is on stack-allocated memory). +The highlighted code segment creates a buffer without ensuring it's large enough to accommodate the copied data. +This leaves the code susceptible to a buffer overflow attack, which could lead to anything from program crashes to malicious code execution.
diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.qhelp b/cpp/ql/src/Security/CWE/CWE-416/IteratorToExpiredContainer.qhelp similarity index 84% rename from cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.qhelp rename to cpp/ql/src/Security/CWE/CWE-416/IteratorToExpiredContainer.qhelp index 19975b17493..87cc75c5f01 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.qhelp +++ b/cpp/ql/src/Security/CWE/CWE-416/IteratorToExpiredContainer.qhelp @@ -30,6 +30,12 @@ This is because the temporary container is not bound to a rvalue reference.
+To fix lifetime_of_temp_not_extended, consider rewriting the code so that the lifetime of the temporary object is extended.
+In fixed_lifetime_of_temp_not_extended, the lifetime of the temporary object has been extended by storing it in an rvalue reference.
+
std::string object is destroyed, the pointer returned by c_str is no
longer valid. If the pointer is used after the std::string object is destroyed, then the behavior is undefined.
+
+Typically, this problem occurs when a std::string is returned by a function call (or overloaded operator)
+by value, and the result is not immediately stored in a variable by value or reference in a way that extends the lifetime of
+the temporary object. The resulting temporary std::string object is destroyed at the end of the containing expression
+statement, along with any memory returned by a call to c_str.
+
The free function, which deallocates heap memory, may accept a NULL pointer and take no action. Therefore, it is unnecessary to check its argument for the value of NULL before a function call to free. As such, these guards may hinder performance and readability.
A function call to free should not depend upon the value of its argument. Delete the if condition preceeding a function call to free when its only purpose is to check the value of the pointer to be freed.
This rule finds a dereference of a function parameter, whose value comes from another function call that may return NULL, without checks in the meantime.
+A check should be added between the return of the function which may return NULL, and its use by the function dereferencing ths pointer.
+