Merge pull request #4953 from ihsinme/ihsinme-patch-207

CPP: Add query for CWE-14 compiler removal of code to clear buffers.

cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c

@@ -0,0 +1,35 @@
+// BAD: the memset call will probably be removed.
+void getPassword(void) {
+  char pwd[64];
+  if (GetPassword(pwd, sizeof(pwd))) {
+    /* Checking of password, secure operations, etc. */
+  }
+  memset(pwd, 0, sizeof(pwd));
+}
+// GOOD: in this case the memset will not be removed.
+void getPassword(void) {
+  char pwd[64];
+ 
+  if (retrievePassword(pwd, sizeof(pwd))) {
+     /* Checking of password, secure operations, etc. */
+  }
+  memset_s(pwd, 0, sizeof(pwd));
+}
+// GOOD: in this case the memset will not be removed.
+void getPassword(void) {
+  char pwd[64];
+  if (retrievePassword(pwd, sizeof(pwd))) {
+    /* Checking of password, secure operations, etc. */
+  }
+  SecureZeroMemory(pwd, sizeof(pwd));
+}
+// GOOD: in this case the memset will not be removed.
+void getPassword(void) {
+  char pwd[64];
+  if (retrievePassword(pwd, sizeof(pwd))) {
+    /* Checking of password, secure operations, etc. */
+  }
+#pragma optimize("", off)
+  memset(pwd, 0, sizeof(pwd));
+#pragma optimize("", on)
+}

cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Compiler optimization will exclude the cleaning of private information.
+Using the <code>memset</code> 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 <code>memset</code> function.</p>
+
+<p>It is possible to miss detection of vulnerabilities if used to clear fields of structures or parts of a buffer.</p>
+
+</overview>
+<recommendation>
+
+<p>We recommend to use the <code>RtlSecureZeroMemory</code> or <code>memset_s</code> functions, or compilation flags that exclude optimization of <code>memset</code> calls (e.g. -fno-builtin-memset).</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates an erroneous and corrected use of the <code>memset</code> function.</p>
+<sample src="CompilerRemovalOfCodeToClearBuffers.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>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql

@@ -0,0 +1,127 @@
+/**
+ * @name Compiler Removal Of Code To Clear Buffers
+ * @description Using <code>memset</code> 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."

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.expected

@@ -0,0 +1,3 @@
+| test.c:13:9:13:13 | buff1 | This variable will not be cleared. |
+| test.c:35:9:35:13 | buff1 | This variable will not be cleared. |
+| test.c:43:9:43:13 | buff1 | This variable will not be cleared. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/test.c

@@ -0,0 +1,201 @@
+struct buffers
+{
+    unsigned char buff1[50];
+    unsigned char *buff2;
+} globalBuff1,*globalBuff2;
+
+unsigned char * globalBuff;
+void badFunc0_0(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+}
+void nobadFunc0_0(){
+	unsigned char buff1[12];
+	memset(buff1,12,12);
+}
+void nobadFunc0_1(){
+	unsigned char buff1[12];
+	int i;
+	memset(buff1,12,12);
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	free(buff1);
+}
+void nobadFunc1_0(){
+	unsigned char * buff1;
+	buff1 = (unsigned char *) malloc(12);
+	memset(buff1,12,12);
+}
+void badFunc1_0(){
+	unsigned char * buff1;
+	buff1 = (unsigned char *) malloc(12);
+	memset(buff1,12,12);
+	free(buff1);
+}
+void badFunc1_1(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;	
+	memset(buff1,12,12);
+	free(buff1);
+}
+void nobadFunc2_0_0(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	printf(buff1);
+}
+
+void nobadFunc2_0_1(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	printf(buff1+3);
+}
+
+void nobadFunc2_0_2(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	printf(*buff1);
+}
+
+void nobadFunc2_0_3(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	printf(*(buff1+3));
+}
+unsigned char * nobadFunc2_0_4(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	return buff1;
+}
+
+unsigned char * nobadFunc2_0_5(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	return buff1+3;
+}
+unsigned char nobadFunc2_0_6(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	return *buff1;
+}
+
+unsigned char nobadFunc2_0_7(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	return *(buff1+3);
+}
+void nobadFunc2_1_0(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	if(*buff1==0)
+		printf("123123");
+}
+void nobadFunc2_1_1(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	if(*(buff1+3)==0)
+		printf("123123");
+}
+void nobadFunc2_1_2(){
+	unsigned char buff1[12];
+	int i;
+	for(i=0;i<12;i++)
+		buff1[i]=13;
+	memset(buff1,12,12);
+	buff1[2]=5;
+}
+void nobadFunc3_0(unsigned char * buffAll){
+	unsigned char * buff1 = buffAll;
+	memset(buff1,12,12);
+}
+void nobadFunc3_1(unsigned char * buffAll){
+	unsigned char * buff1 = buffAll+3;
+	memset(buff1,12,12);
+}
+void nobadFunc3_2(struct buffers buffAll){
+	unsigned char * buff1 = buffAll.buff1;
+	memset(buff1,12,12);
+}
+void nobadFunc3_3(struct buffers buffAll){
+	unsigned char * buff1 = buffAll.buff2;
+	memset(buff1,12,12);
+}
+void nobadFunc3_4(struct buffers buffAll){
+	unsigned char * buff1 = buffAll.buff2+3;
+	memset(buff1,12,12);
+}
+void nobadFunc3_5(struct buffers * buffAll){
+	unsigned char * buff1 = buffAll->buff1;
+	memset(buff1,12,12);
+}
+void nobadFunc3_6(struct buffers *buffAll){
+	unsigned char * buff1 = buffAll->buff2;
+	memset(buff1,12,12);
+}
+void nobadFunc4(){
+	unsigned char * buff1 = globalBuff;
+	memset(buff1,12,12);
+}
+void nobadFunc4_0(){
+	unsigned char * buff1 = globalBuff;
+	memset(buff1,12,12);
+}
+void nobadFunc4_1(){
+	unsigned char * buff1 = globalBuff+3;
+	memset(buff1,12,12);
+}
+void nobadFunc4_2(){
+	unsigned char * buff1 = globalBuff1.buff1;
+	memset(buff1,12,12);
+}
+void nobadFunc4_3(){
+	unsigned char * buff1 = globalBuff1.buff2;
+	memset(buff1,12,12);
+}
+void nobadFunc4_4(){
+	unsigned char * buff1 = globalBuff1.buff2+3;
+	memset(buff1,12,12);
+}
+void nobadFunc4_5(){
+	unsigned char * buff1 = globalBuff2->buff1;
+	memset(buff1,12,12);
+}
+void nobadFunc4_6(){
+	unsigned char * buff1 = globalBuff2->buff2;
+	memset(buff1,12,12);
+}
+