[CPP-435] Calls to memset and ZeroMemory may be deleted by the compiler

cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted-bad.c

@@ -0,0 +1,4 @@
+char * password = malloc(PASSWORD_SIZE);
+// ... read and check password
+memset(password, 0, PASSWORD_SIZE);
+free(password);

cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted-good.c

@@ -0,0 +1,4 @@
+char * password = malloc(PASSWORD_SIZE);
+// ... read and check password
+memset_s(password, PASSWORD_SIZE, 0, PASSWORD_SIZE);
+free(password);

cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.qhelp

@@ -0,0 +1,52 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Calling <code>memset</code>, <code>bzero</code>, <code>FillMemory</code> or
+<code>ZeroMemory</code> on a buffer in order 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 be somehow retrieved by an attacker.</p>
+
+</overview>
+<recommendation>
+<p>The best workaround is to use alternate platform-supplied functions that
+ will not get optimized away: <code>memset_s</code>, <code>SecureZeroMemory</code>,
+ <code>bzero_explicit</code>, and the like (if they are available).
+ Otherwise, passing the <code>-fno-builtin-memset</code> flag to the 
+ GCC/Clang compiler is usually effective.  Finally, the public-domain 
+ <code>secure_memzero</code> routine (see below) can be helpful.</p>
+
+</recommendation>
+<example>
+<p>The following program fragment uses <code>memset</code> to scrub memory after
+ it is no longer used:</p>
+<sample src="MemsetMayBeDeleted-bad.c" />
+<p>Because of dead store elimination (DSE), the <code>memset</code> may be removed
+ by the compiler (since the data being scrubbed 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>MITRE:
+  <a href="https://cwe.mitre.org/data/definitions/14.html">
+  CWE-14</a>.</li>
+<li>Public domain:
+  <a href="https://compsec.sysnet.ucsd.edu/secure_memzero.h">
+  secure_memzero</a> implementation.</li>
+<li>Blog:
+  <a href="https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_sC">
+  Zero'ing memory, compiler optimizations and memset_s</a>.</li>
+<li>USENIX 2017 Paper:
+  <a href="https://www.usenix.org/system/files/conference/usenixsecurity17/sec17-yang.pdf">
+  Dead Store Elimination (Still) Considered Harmful</a>.</li>
+
+</references>
+</qhelp>

cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.ql

@@ -0,0 +1,41 @@
+/**
+ * @name Call to `memset` may be deleted
+ * @description Calling `memset` or `bzero` on a buffer in order 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 be exploited by an attacker.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision high
+ * @id cpp/memset-may-be-deleted
+ * @tags security
+ *       external/cwe/cwe-14
+ */
+
+import cpp
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.ValueNumbering
+import semmle.code.cpp.models.implementations.Memset
+
+class MemsetCallInstruction extends CallInstruction {
+  MemsetCallInstruction() { this.getStaticCallTarget() instanceof MemsetFunction }
+}
+
+Instruction getAUseInstruction(Instruction insn) { result = insn.getAUse().getUse() }
+
+predicate pointsIntoStack(Instruction instr) {
+  instr.(VariableAddressInstruction).getIRVariable() instanceof IRAutomaticVariable
+  or
+  pointsIntoStack(instr.(CopyInstruction).getSourceValue())
+  or
+  pointsIntoStack(instr.(ConvertInstruction).getUnary())
+}
+
+from MemsetCallInstruction memset, SizedBufferMustWriteSideEffectInstruction sei
+where
+  sei.getPrimaryInstruction() = memset and
+  // The first argument to memset must reside on the stack
+  pointsIntoStack(valueNumber(memset.getPositionalArgument(0)).getAnInstruction()) and
+  // The result of memset may not be subsequently used
+  forall(Instruction use | use = getAUseInstruction+(sei) | use instanceof ChiInstruction)
+select memset,
+  "Call to " + memset.getStaticCallTarget().getName() + " may be deleted by the compiler."

cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.c

@@ -0,0 +1,140 @@
+typedef unsigned long long size_t;
+void *memset(void *s, int c, unsigned long n);
+void *__builtin_memset(void *s, int c, unsigned long n);
+typedef int errno_t;
+typedef unsigned int rsize_t;
+errno_t memset_s(void *dest, rsize_t destsz, int ch, rsize_t count);
+char *strcpy(char *dest, const char *src);
+
+extern void use_pw(char *pw);
+
+#define PW_SIZE 32
+
+// x86-64 gcc 9.2: deleted
+// x86-64 clang 9.0.0: deleted
+// x64 msvc v19.14 (WINE): deleted
+int func1(void) { 
+	char pw1[PW_SIZE];
+	use_pw(pw1);
+	memset(pw1, 0, PW_SIZE); // BAD
+	return 0;
+}
+
+// x86-64 gcc 9.2: deleted
+// x86-64 clang 9.0.0: deleted
+// x64 msvc v19.14 (WINE): not deleted
+int func1a(void) {  
+	char pw1a[PW_SIZE];
+	use_pw(pw1a);
+	__builtin_memset(pw1a, 0, PW_SIZE); // BAD
+	return 0;
+}
+
+// x86-64 gcc 9.2: deleted
+// x86-64 clang 9.0.0: deleted
+// x64 msvc v19.14 (WINE): deleted
+char *func1b(void) {  
+	char pw1b[PW_SIZE];
+	use_pw(pw1b);
+	memset(pw1b, 0, PW_SIZE); // BAD
+	pw1b[0] = pw1b[3] = 'a';
+	return 0;
+}
+
+// x86-64 gcc 9.2: not deleted
+// x86-64 clang 9.0.0: not deleted
+// x64 msvc v19.14 (WINE): not deleted
+int func1c(char pw1c[PW_SIZE]) {  
+	use_pw(pw1c);
+	memset(pw1c, 0, PW_SIZE); // GOOD
+	return 0;
+}
+
+// x86-64 gcc 9.2: not deleted
+// x86-64 clang 9.0.0: not deleted
+// x64 msvc v19.14 (WINE): not deleted
+char pw1d[PW_SIZE];
+int func1d() {  
+	use_pw(pw1d);
+	memset(pw1d, 0, PW_SIZE); // GOOD
+	return 0;
+}
+// x86-64 gcc 9.2: not deleted
+// x86-64 clang 9.0.0: not deleted
+// x64 msvc v19.14 (WINE): not deleted
+char *func2(void) {  
+	char pw2[PW_SIZE];
+	use_pw(pw2);
+	memset(pw2, 1, PW_SIZE); // BAD
+	return pw2;
+}
+
+// x86-64 gcc 9.2: deleted
+// x86-64 clang 9.0.0: deleted
+// x64 msvc v19.14 (WINE): partially deleted
+int func3(void) { 
+	char pw3[PW_SIZE];
+	use_pw(pw3);
+	memset(pw3, 4, PW_SIZE); // BAD [NOT DETECTED]
+	return pw3[2];
+}
+
+// x86-64 gcc 9.2: deleted
+// x86-64 clang 9.0.0: deleted
+// x64 msvc v19.14 (WINE): not deleted
+int func4(void) { 
+	char pw1a[PW_SIZE];
+	use_pw(pw1a);
+	__builtin_memset(pw1a + 3, 0, PW_SIZE - 3); // BAD [NOT DETECTED]
+	return 0;
+}
+
+// x86-64 gcc 9.2: deleted
+// x86-64 clang 9.0.0: deleted
+// x64 msvc v19.14 (WINE): not deleted
+int func6(void) { 
+	char pw1a[PW_SIZE];
+	use_pw(pw1a);
+	__builtin_memset(&pw1a[3], 0, PW_SIZE - 3); // BAD [NOT DETECTED]
+	return pw1a[2];
+}
+
+// x86-64 gcc 9.2: deleted
+// x86-64 clang 9.0.0: deleted
+// x64 msvc v19.14 (WINE): not deleted
+int func5(void) { 
+	char pw1a[PW_SIZE];
+	use_pw(pw1a);
+	__builtin_memset(pw1a + 3, 0, PW_SIZE - 4); // GOOD
+	return pw1a[4];
+}
+
+// x86-64 gcc 9.2: deleted
+// x86-64 clang 9.0.0: deleted
+// x64 msvc v19.14 (WINE): not deleted
+int func7(void) { 
+	char pw1a[PW_SIZE];
+	use_pw(pw1a);
+	__builtin_memset(&pw1a[3], 0, PW_SIZE - 5); // BAD [NOT DETECTED]
+	return 0;
+}
+
+// x86-64 gcc 9.2: not deleted
+// x86-64 clang 9.0.0: not deleted
+// x64 msvc v19.14 (WINE): not deleted
+int func8(void) { 
+	char pw1a[PW_SIZE];
+	use_pw(pw1a);
+	__builtin_memset(pw1a + pw1a[3], 0, PW_SIZE - 4); // GOOD
+	return pw1a[4];
+}
+
+// x86-64 gcc 9.2: not deleted
+// x86-64 clang 9.0.0: not deleted
+// x64 msvc v19.14 (WINE): not deleted
+char *func9(void) {
+	char pw1[PW_SIZE];
+	use_pw(pw1);
+	memset(pw1, 0, PW_SIZE); // BAD
+	return 0;
+}