+This rule finds relational comparisons between the result of an unsigned subtraction and the value 0.
+Such comparisons are likely to be wrong as the value of an unsigned subtraction can never be negative. So the
+relational comparison ends up checking whether the result of the subtraction is equal to 0.
+This is probably not what the programmer intended.
+
If a relational comparison is intended, consider casting the result of the subtraction to a signed type. + If the intention was to test for equality, consider replacing the relational comparison with an equality test. +
+ +Compiler optimization will exclude the cleaning of private information.
+Using the memset function to clear private data in a variable that has no subsequent use is potentially dangerous, since the compiler can remove the call.
+For some compilers, optimization is also possible when using calls to free memory after the memset function.
It is possible to miss detection of vulnerabilities if used to clear fields of structures or parts of a buffer.
+ +We recommend to use the RtlSecureZeroMemory or memset_s functions, or compilation flags that exclude optimization of memset calls (e.g. -fno-builtin-memset).
The following example demonstrates an erroneous and corrected use of the memset function.
memset the function to clear private data in a variable that has no subsequent use
+ * is potentially dangerous because the compiler can remove the call.
+ * @kind problem
+ * @id cpp/compiler-removal-of-code-to-clear-buffers
+ * @problem.severity warning
+ * @precision medium
+ * @tags security
+ * external/cwe/cwe-14
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.dataflow.StackAddress
+
+/**
+ * A call to `memset` of the form `memset(ptr, value, num)`, for some local variable `ptr`.
+ */
+class CompilerRemovaMemset extends FunctionCall {
+ CompilerRemovaMemset() {
+ this.getTarget().hasGlobalOrStdName("memset") and
+ exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Expr exp |
+ DataFlow::localFlow(source, sink) and
+ this.getArgument(0) = isv.getAnAccess() and
+ (
+ source.asExpr() = exp
+ or
+ // handle the case where exp is defined by an address being passed into some function.
+ source.asDefiningArgument() = exp
+ ) and
+ exp.getLocation().getEndLine() < this.getArgument(0).getLocation().getStartLine() and
+ sink.asExpr() = this.getArgument(0)
+ )
+ }
+
+ predicate isExistsAllocForThisVariable() {
+ exists(AllocationExpr alloc, Variable v |
+ alloc = v.getAnAssignedValue() and
+ this.getArgument(0) = v.getAnAccess() and
+ alloc.getASuccessor+() = this
+ )
+ or
+ not stackPointerFlowsToUse(this.getArgument(0), _, _, _)
+ }
+
+ predicate isExistsFreeForThisVariable() {
+ exists(DeallocationExpr free, Variable v |
+ this.getArgument(0) = v.getAnAccess() and
+ free.getFreedExpr() = v.getAnAccess() and
+ this.getASuccessor+() = free
+ )
+ }
+
+ predicate isExistsCallWithThisVariableExcludingDeallocationCalls() {
+ exists(FunctionCall fc, Variable v |
+ not fc instanceof DeallocationExpr and
+ this.getArgument(0) = v.getAnAccess() and
+ fc.getAnArgument() = v.getAnAccess() and
+ this.getASuccessor+() = fc
+ )
+ }
+
+ predicate isVariableUseAfterMemsetExcludingCalls() {
+ exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Expr exp |
+ DataFlow::localFlow(source, sink) and
+ this.getArgument(0) = isv.getAnAccess() and
+ source.asExpr() = isv.getAnAccess() and
+ exp.getLocation().getStartLine() > this.getArgument(2).getLocation().getEndLine() and
+ not exp.getParent() instanceof FunctionCall and
+ sink.asExpr() = exp
+ )
+ }
+
+ predicate isVariableUseBoundWithArgumentFunction() {
+ exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Parameter p, Expr exp |
+ DataFlow::localFlow(source, sink) and
+ this.getArgument(0) = isv.getAnAccess() and
+ this.getEnclosingFunction().getAParameter() = p and
+ exp.getAChild*() = p.getAnAccess() and
+ source.asExpr() = exp and
+ sink.asExpr() = isv.getAnAccess()
+ )
+ }
+
+ predicate isVariableUseBoundWithGlobalVariable() {
+ exists(
+ DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, GlobalVariable gv, Expr exp
+ |
+ DataFlow::localFlow(source, sink) and
+ this.getArgument(0) = isv.getAnAccess() and
+ exp.getAChild*() = gv.getAnAccess() and
+ source.asExpr() = exp and
+ sink.asExpr() = isv.getAnAccess()
+ )
+ }
+
+ predicate isExistsCompilationFlagsBlockingRemoval() {
+ exists(Compilation c |
+ c.getAFileCompiled() = this.getFile() and
+ c.getAnArgument() = "-fno-builtin-memset"
+ )
+ }
+
+ predicate isUseVCCompilation() {
+ exists(Compilation c |
+ c.getAFileCompiled() = this.getFile() and
+ (
+ c.getArgument(2).matches("%gcc%") or
+ c.getArgument(2).matches("%g++%") or
+ c.getArgument(2).matches("%clang%") or
+ c.getArgument(2) = "--force-recompute"
+ )
+ )
+ }
+}
+
+from CompilerRemovaMemset fc
+where
+ not (fc.isExistsAllocForThisVariable() and not fc.isExistsFreeForThisVariable()) and
+ not (fc.isExistsFreeForThisVariable() and not fc.isUseVCCompilation()) and
+ not fc.isVariableUseAfterMemsetExcludingCalls() and
+ not fc.isExistsCallWithThisVariableExcludingDeallocationCalls() and
+ not fc.isVariableUseBoundWithArgumentFunction() and
+ not fc.isVariableUseBoundWithGlobalVariable() and
+ not fc.isExistsCompilationFlagsBlockingRemoval()
+select fc.getArgument(0), "This variable will not be cleared."
diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.cpp b/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.cpp
new file mode 100644
index 00000000000..bde49c946ea
--- /dev/null
+++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.cpp
@@ -0,0 +1,19 @@
+
+image::image(int width, int height)
+{
+ int x, y;
+
+ // allocate width * height pixels
+ pixels = new uint32_t[width * height];
+
+ // fill width * height pixels
+ for (y = 0; y < height; y++)
+ {
+ for (x = 0; x < width; x++)
+ {
+ pixels[(y * width) + height] = 0;
+ }
+ }
+
+ // ...
+}
diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.qhelp
new file mode 100644
index 00000000000..97bd393d5a0
--- /dev/null
+++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.qhelp
@@ -0,0 +1,25 @@
+
+The result of a multiplication is used in the size of an allocation. If the multiplication can be made to overflow, a much smaller amount of memory may be allocated than the rest of the code expects. This may lead to overflowing writes when the buffer is accessed later.
+To fix this issue, ensure that the arithmetic used in the size of an allocation cannot overflow before memory is allocated.
+In the following example, an array of size width * height is allocated and stored as pixels. If width and height are set such that the multiplication overflows and wraps to a small value (say, 4) then the initialization code that follows the allocation will write beyond the end of the array.
The code compares the unsigned difference with zero. -It is highly probable that the condition is wrong if the difference expression has the unsigned type. -The condition holds in all the cases when difference is not equal to zero. -It means that we may use condition not equal. But the programmer probably wanted to compare the difference of elements.
- -False positives include code in which the first difference element is always greater than or equal to the second. -For comparison, ">" such conditions are equivalent to "! =", And are recommended for replacement. -For comparison "> =", the conditions are always true and are recommended to be excluded.
- -Use a simple comparison of two elements, instead of comparing their difference to zero.
- -The following example demonstrates an erroneous and corrected use of comparison.
-Memory leak on failed call to realloc.
+The expression mem = realloc (mem, size) is potentially dangerous, if the call fails, we will lose the pointer to the memory block.
+An unsuccessful call is possible not only when trying to allocate a large amount of memory, but also when the process memory is strongly segmented.
False positives include code in which immediately after calling the realloc function, the pointer is manipulated without first checking for validity. +In this case, an exception will occur in the program and it will terminate. +But from the point of view of safe coding, these places require the attention of developers. +At this stage, false positives are also possible in situations where the exception handling is quite complicated and occurs outside the base block in which memory is redistributed.
+ +We recommend storing the result in a temporary variable and eliminating memory leak.
+ +The following example demonstrates an erroneous and corrected use of the realloc function.
When using the new operator to allocate memory, you need to pay attention to the different ways of detecting errors. ::operator new(std::size_t) throws an exception on error, whereas ::operator new(std::size_t, const std::nothrow_t &) returns zero on error. The programmer can get confused and check the error that occurs when allocating memory incorrectly. That can lead to an unhandled program termination or to a violation of the program logic.
Use the correct error detection method corresponding with the memory allocation.
+ +The following example demonstrates various approaches to detecting memory allocation errors using the new operator.
Potentially dangerous use of the strlen function to calculate the length of a string.
+The expression buffer[strlen(buffer)] = 0 is potentially dangerous, if the variable buffer does not have a terminal zero, then access beyond the bounds of the allocated memory is possible, which will lead to undefined behavior.
+If terminal zero is present, then the specified expression is meaningless.
False positives include heavily nested strlen. This situation is unlikely.
+ +We recommend using another method for calculating the string length
+ +The following example demonstrates an erroneous and corrected use of the strlen function.
+The standard library function strncat(dest, source, count) appends the source string to the dest string. count specifies the maximum number of characters to append and must be less than the remaining space in the target buffer. Calls of the form strncat (dest, source, sizeof (dest) - strlen (dest)) set the third argument to one more than possible. So when the dest is full, the expression sizeof (dest) - strlen (dest) will be equal to one, and not zero as the programmer might think. Making a call of this type may result in a zero byte being written just outside the dest buffer.
We recommend subtracting one from the third argument. For example, replace strncat(dest, source, sizeof(dest)-strlen(dest)) with strncat(dest, source, sizeof(dest)-strlen(dest)-1).
The following example demonstrates an erroneous and corrected use of the strncat function.
typeof(System.Collections.Generic.List.Enumerator)
+ /// -> System.Collections.Generic.List`1+Enumerator[[System.Int32, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e
+ /// typeof(System.Collections.Generic.List<>.Enumerator)
+ /// -> System.Collections.Generic.List`1+Enumerator, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e
+ ///
+ ///
-The format string supplied to formatting methods (such as string.Format())
-must be formatted correctly, otherwise the exception System.FormatException
-will be thrown.
+When using string formatting methods (such as string.Format()), the following
+should be taken into account:
System.FormatException will be thrown.
+ System.FormatException exception
+ being thrown.
+ -Change the format string so that it is correctly formatted. Ensure that each -format item adheres to the syntax: -
- --- -{index[,alignment][:formatString]}
-
-When literals { or } are required, replace them with {{ and
-}}, respectively, or supply them as arguments.
-
++ When literals{index[,alignment][:formatString]}
+
{ or } are required, replace them with {{ and
+ }}, respectively, or supply them as arguments.
+ +Here are three examples where the format string does not use all the arguments. +
+ex.HResult) is not logged.ex) is not logged but the second
+ argument (ex.HResult) is logged twice.
+Here are two examples where the call to String.Format() is missing arguments.
+
last) is not supplied.{1} and {2},
+ instead of {0} and {1} as they should be.+In the revised example, both arguments are supplied. +
+
-Formatting methods (such as String.Format()) that are missing arguments will
-throw the exception System.FormatException. This is caused by the format
-string not matching the actual arguments supplied or an incorrect format string.
-
-Supply the correct number of arguments to the format method, or change the format string -to use the correct arguments. -
-
-Here are two examples where the call to String.Format() is missing arguments.
-
last) is not supplied.{1} and {2},
- instead of {0} and {1} as they should be.-In the revised example, both arguments are supplied. -
-
-Arguments which are passed to formatting methods (such as String.Format())
-but are not used, are either unnecessary or mean that the format string is incorrect. The result
-is that the argument will be ignored, which may not be the intended behavior.
-
-Change the format string to use the highlighted argument, or remove the unnecessary argument. -
--Here are three examples where the format string does not use all the arguments. -
-ex.HResult) is not logged.ex) is not logged but the second
- argument (ex.HResult) is logged twice.