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-359/PrivateCleartextWrite.ql b/cpp/ql/src/experimental/Security/CWE/CWE-359/PrivateCleartextWrite.ql
index 60b13525aff..205f17c06c9 100644
--- a/cpp/ql/src/experimental/Security/CWE/CWE-359/PrivateCleartextWrite.ql
+++ b/cpp/ql/src/experimental/Security/CWE/CWE-359/PrivateCleartextWrite.ql
@@ -16,6 +16,6 @@ import DataFlow::PathGraph
from WriteConfig b, DataFlow::PathNode source, DataFlow::PathNode sink
where b.hasFlowPath(source, sink)
-select sink.getNode(),
- "This write into the external location '" + sink + "' may contain unencrypted data from $@",
- source, "this source."
+select sink.getNode(), source, sink,
+ "This write into the external location '" + sink.getNode() +
+ "' may contain unencrypted data from $@", source, "this source."
diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-401/MemoryLeakOnFailedCallToRealloc.ql b/cpp/ql/src/experimental/Security/CWE/CWE-401/MemoryLeakOnFailedCallToRealloc.ql
index da54d9a49bf..1578bff1407 100644
--- a/cpp/ql/src/experimental/Security/CWE/CWE-401/MemoryLeakOnFailedCallToRealloc.ql
+++ b/cpp/ql/src/experimental/Security/CWE/CWE-401/MemoryLeakOnFailedCallToRealloc.ql
@@ -12,6 +12,20 @@
*/
import cpp
+import semmle.code.cpp.controlflow.Guards
+
+/**
+ * A function call that potentially does not return (such as `exit`).
+ */
+class CallMayNotReturn extends FunctionCall {
+ CallMayNotReturn() {
+ // call that is known to not return
+ not exists(this.(ControlFlowNode).getASuccessor())
+ or
+ // call to another function that may not return
+ exists(CallMayNotReturn exit | getTarget() = exit.getEnclosingFunction())
+ }
+}
/**
* A call to `realloc` of the form `v = realloc(v, size)`, for some variable `v`.
@@ -30,40 +44,19 @@ class ReallocCallLeak extends FunctionCall {
)
}
- predicate isExistsIfWithExitCall() {
- exists(IfStmt ifc |
- this.getArgument(0) = v.getAnAccess() and
- ifc.getCondition().getAChild*() = v.getAnAccess() and
- ifc.getEnclosingFunction() = this.getEnclosingFunction() and
- ifc.getLocation().getStartLine() >= this.getArgument(0).getLocation().getStartLine() and
- exists(FunctionCall fc |
- fc.getTarget().hasName("exit") and
- fc.getEnclosingFunction() = this.getEnclosingFunction() and
- (ifc.getThen().getAChild*() = fc or ifc.getElse().getAChild*() = fc)
- )
- or
- exists(FunctionCall fc, FunctionCall ftmp1, FunctionCall ftmp2 |
- ftmp1.getTarget().hasName("exit") and
- ftmp2.(ControlFlowNode).getASuccessor*() = ftmp1 and
- fc = ftmp2.getEnclosingFunction().getACallToThisFunction() and
- fc.getEnclosingFunction() = this.getEnclosingFunction() and
- (ifc.getThen().getAChild*() = fc or ifc.getElse().getAChild*() = fc)
- )
- )
- }
-
- predicate isExistsAssertWithArgumentCall() {
- exists(FunctionCall fc |
- fc.getTarget().hasName("__assert_fail") and
- this.getEnclosingFunction() = fc.getEnclosingFunction() and
- fc.getLocation().getStartLine() > this.getArgument(0).getLocation().getEndLine() and
- fc.getArgument(0).toString().matches("%" + this.getArgument(0).toString() + "%")
+ /**
+ * Holds if failure of this allocation may be handled by termination, for
+ * example a call to `exit()`.
+ */
+ predicate mayHandleByTermination() {
+ exists(GuardCondition guard, CallMayNotReturn exit |
+ this.(ControlFlowNode).getASuccessor*() = guard and
+ guard.getAChild*() = v.getAnAccess() and
+ guard.controls(exit.getBasicBlock(), _)
)
}
}
from ReallocCallLeak rcl
-where
- not rcl.isExistsIfWithExitCall() and
- not rcl.isExistsAssertWithArgumentCall()
+where not rcl.mayHandleByTermination()
select rcl, "possible loss of original pointer on unsuccessful call realloc"
diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.c b/cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.c
new file mode 100644
index 00000000000..060a22b5c18
--- /dev/null
+++ b/cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.c
@@ -0,0 +1,4 @@
+
+strncat(dest, source, sizeof(dest) - strlen(dest)); // BAD: writes a zero byte past the `dest` buffer.
+
+strncat(dest, source, sizeof(dest) - strlen(dest) -1); // GOOD: Reserves space for the zero byte.
diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.qhelp
new file mode 100644
index 00000000000..5c2154097ec
--- /dev/null
+++ b/cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.qhelp
@@ -0,0 +1,32 @@
+
+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.