Merge pull request github#5256 from MathiasVP/promote-insecure-memset-query

C++: Promote insecure removal of memset query

Author: jbj

cpp/change-notes/2021-02-24-memset-may-be-deleted.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* A new query (`cpp/memset-may-be-deleted`) is added to the default query suite. The query finds calls to `memset` that may be removed by the compiler. This behavior can make information-leak vulnerabilities easier to exploit. This query was originally [submitted as an experimental query by @ihsinme](https://github.com/github/codeql/pull/4953).

cpp/ql/src/Security/CWE/CWE-014/MemsetMayBeDeleted-bad.c

@@ -0,0 +1,3 @@
+char password[MAX_PASSWORD_LENGTH];
+// read and verify password
+memset(password, 0, MAX_PASSWORD_LENGTH);

cpp/ql/src/Security/CWE/CWE-014/MemsetMayBeDeleted-good.c

@@ -0,0 +1,3 @@
+char password[MAX_PASSWORD_LENGTH];
+// read and verify password
+memset_s(password, MAX_PASSWORD_LENGTH, 0, MAX_PASSWORD_LENGTH);

cpp/ql/src/Security/CWE/CWE-014/MemsetMayBeDeleted.qhelp

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Calling <code>memset</code> or <code>bzero</code> on a buffer to clear its contents may get optimized
+away by the compiler if the buffer is not subsequently used. This is not desirable behavior if the buffer
+contains sensitive data that could somehow be retrieved by an attacker.</p>
+
+</overview>
+<recommendation>
+
+<p>Use alternative platform-supplied functions that will not get optimized away. Examples of such
+functions include <code>memset_s</code>, <code>SecureZeroMemory</code>, and <code>bzero_explicit</code>.
+Alternatively, passing the <code>-fno-builtin-memset</code> option to the GCC/Clang compiler usually
+also prevents the optimization. Finally, you can use the public-domain <code>secure_memzero</code> function
+(see references below). This function, however, is not guaranteed to work on all platforms and compilers.</p>
+
+</recommendation>
+<example>
+<p>The following program fragment uses <code>memset</code> to erase sensitive information after it is no
+longer needed:</p>
+<sample src="MemsetMayBeDeleted-bad.c" />
+<p>Because of dead store elimination, the call to <code>memset</code> may be removed by the compiler
+(since the buffer is not subsequently used), resulting in potentially sensitive data remaining in memory.
+</p>
+
+<p>The best solution to this problem is to use the <code>memset_s</code> function instead of
+<code>memset</code>:</p>
+<sample src="MemsetMayBeDeleted-good.c" />
+
+</example>
+<references>
+
+<li>
+CERT C Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/c/MSC06-C.+Beware+of+compiler+optimizations">MSC06-C. Beware of compiler optimizations</a>.
+</li>
+<li>
+USENIX: The Advanced Computing Systems Association:
+<a href="https://www.usenix.org/system/files/conference/usenixsecurity17/sec17-yang.pdf">Dead Store Elimination (Still) Considered Harmfuls</a>
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/Security/CWE/CWE-014/MemsetMayBeDeleted.ql

@@ -0,0 +1,66 @@
+/**
+ * @name Call to `memset` may be deleted
+ * @description Using the `memset` function to clear private data in a variable that has no subsequent use
+ *              can make information-leak vulnerabilities easier to exploit because the compiler can remove the call.
+ * @kind problem
+ * @id cpp/memset-may-be-deleted
+ * @problem.severity warning
+ * @precision high
+ * @tags security
+ *       external/cwe/cwe-14
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.EscapesTree
+import semmle.code.cpp.commons.Exclusions
+import semmle.code.cpp.models.interfaces.Alias
+
+class MemsetFunction extends Function {
+  MemsetFunction() {
+    this.hasGlobalOrStdOrBslName("memset")
+    or
+    this.hasGlobalOrStdName("wmemset")
+    or
+    this.hasGlobalName(["bzero", "__builtin_memset"])
+  }
+}
+
+predicate isNonEscapingArgument(Expr escaped) {
+  exists(Call call, AliasFunction aliasFunction, int i |
+    aliasFunction = call.getTarget() and
+    call.getArgument(i) = escaped.getUnconverted() and
+    (
+      aliasFunction.parameterNeverEscapes(i)
+      or
+      aliasFunction.parameterEscapesOnlyViaReturn(i) and
+      (call instanceof ExprInVoidContext or call.getConversion*() instanceof BoolConversion)
+    )
+  )
+}
+
+from FunctionCall call, LocalVariable v, MemsetFunction memset
+where
+  call.getTarget() = memset and
+  not isFromMacroDefinition(call) and
+  // `v` escapes as the argument to `memset`
+  variableAddressEscapesTree(v.getAnAccess(), call.getArgument(0).getFullyConverted()) and
+  // ... and `v` doesn't escape anywhere else.
+  forall(Expr escape | variableAddressEscapesTree(v.getAnAccess(), escape) |
+    isNonEscapingArgument(escape)
+  ) and
+  not v.isStatic() and
+  // Reference-typed variables get special treatment in `variableAddressEscapesTree` so we leave them
+  // out of this query.
+  not v.getUnspecifiedType() instanceof ReferenceType and
+  // `v` is not only just used in the call to `memset`.
+  exists(Access acc |
+    acc = v.getAnAccess() and not call.getArgument(0).getAChild*() = acc and not acc.isUnevaluated()
+  ) and
+  // There is no later use of `v`.
+  not v.getAnAccess() = call.getASuccessor*() and
+  // Not using the `-fno-builtin-memset` flag
+  exists(Compilation c |
+    c.getAFileCompiled() = call.getFile() and
+    not c.getAnArgument() = "-fno-builtin-memset"
+  )
+select call, "Call to " + memset.getName() + " may be deleted by the compiler."