Merge pull request github#5766 from ihsinme/ihsinme-patch-267

CPP: Add query for CWE-415 Double Free

Author: MathiasVP

cpp/ql/src/experimental/Security/CWE/CWE-415/DoubleFree.c

@@ -0,0 +1,14 @@
+...
+  buf = malloc(intSize);
+...
+  free(buf); 
+  buf = NULL;
+  if(buf) free(buf); // GOOD
+...
+
+...
+  buf = malloc(intSize);
+...
+  free(buf); 
+  if(buf) free(buf); // BAD: the cleanup function does not zero out the pointer
+...

cpp/ql/src/experimental/Security/CWE/CWE-415/DoubleFree.qhelp

@@ -0,0 +1,26 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Freeing a previously allocated resource twice can lead to various vulnerabilities in the program.</p>
+
+</overview>
+<recommendation>
+<p>We recommend that you exclude situations of possible double release. For example, use the assignment NULL to a freed variable.</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates an erroneous and corrected use of freeing a pointer.</p>
+<sample src="DoubleFree.c" />
+
+</example>
+<references>
+
+<li>
+  CERT C Coding Standard:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/MEM30-C.+Do+not+access+freed+memory">MEM30-C. Do not access freed memory</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-415/DoubleFree.ql

@@ -0,0 +1,43 @@
+/**
+ * @name Errors When Double Free
+ * @description Freeing a previously allocated resource twice can lead to various vulnerabilities in the program.
+ * @kind problem
+ * @id cpp/double-free
+ * @problem.severity warning
+ * @precision medium
+ * @tags security
+ *       external/cwe/cwe-415
+ */
+
+import cpp
+
+from FunctionCall fc, FunctionCall fc2, LocalScopeVariable v
+where
+  freeCall(fc, v.getAnAccess()) and
+  freeCall(fc2, v.getAnAccess()) and
+  fc != fc2 and
+  fc.getASuccessor*() = fc2 and
+  not exists(Expr exptmp |
+    (exptmp = v.getAnAssignedValue() or exptmp.(AddressOfExpr).getOperand() = v.getAnAccess()) and
+    exptmp = fc.getASuccessor*() and
+    exptmp = fc2.getAPredecessor*()
+  ) and
+  not exists(FunctionCall fctmp |
+    not fctmp instanceof DeallocationExpr and
+    fctmp = fc.getASuccessor*() and
+    fctmp = fc2.getAPredecessor*() and
+    fctmp.getAnArgument().(VariableAccess).getTarget() = v
+  ) and
+  (
+    fc.getTarget().hasGlobalOrStdName("realloc") and
+    (
+      not fc.getParent*() instanceof IfStmt and
+      not exists(IfStmt iftmp |
+        iftmp.getCondition().getAChild*().(VariableAccess).getTarget().getAnAssignedValue() = fc
+      )
+    )
+    or
+    not fc.getTarget().hasGlobalOrStdName("realloc")
+  )
+select fc2.getArgument(0),
+  "This pointer may have already been cleared in the line " + fc.getLocation().getStartLine() + "."

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-415/semmle/tests/DoubleFree.expected

@@ -0,0 +1,4 @@
+| test.c:11:16:11:18 | buf | This pointer may have already been cleared in the line 10. |
+| test.c:18:8:18:10 | buf | This pointer may have already been cleared in the line 17. |
+| test.c:57:8:57:10 | buf | This pointer may have already been cleared in the line 55. |
+| test.c:78:8:78:10 | buf | This pointer may have already been cleared in the line 77. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-415/semmle/tests/DoubleFree.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-415/DoubleFree.ql

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

@@ -0,0 +1,96 @@
+typedef unsigned long size_t;
+void *malloc(size_t size);
+void free(void *ptr);
+#define NULL 0
+
+void workFunction_0(char *s) {
+  int intSize = 10;
+  char *buf;
+  buf = (char *) malloc(intSize);
+  free(buf); // GOOD
+  if(buf) free(buf); // BAD
+}
+void workFunction_1(char *s) {
+  int intSize = 10;
+  char *buf;
+  buf = (char *) malloc(intSize);
+  free(buf); // GOOD
+  free(buf); // BAD
+}
+void workFunction_2(char *s) {
+  int intSize = 10;
+  char *buf;
+  buf = (char *) malloc(intSize);
+  free(buf); // GOOD
+  buf = NULL;
+  free(buf); // GOOD
+}
+void workFunction_3(char *s) {
+  int intSize = 10;
+  char *buf;
+  int intFlag;
+  buf = (char *) malloc(intSize);
+  if(buf[1]%5) {
+    free(buf); // GOOD
+    buf = NULL;
+  }
+  free(buf); // GOOD
+}
+void workFunction_4(char *s) {
+  int intSize = 10;
+  char *buf;
+  char *tmpbuf;
+  tmpbuf = (char *) malloc(intSize);
+  buf = (char *) malloc(intSize);
+  free(buf); // GOOD
+  buf = tmpbuf;
+  free(buf); // GOOD
+}
+void workFunction_5(char *s, int intFlag) {
+  int intSize = 10;
+  char *buf;
+
+  buf = (char *) malloc(intSize);
+  if(intFlag) {
+    free(buf); // GOOD
+  }
+  free(buf); // BAD
+}
+void workFunction_6(char *s, int intFlag) {
+  int intSize = 10;
+  char *buf;
+  char *tmpbuf;
+
+  tmpbuf = (char *) malloc(intSize);
+  buf = (char *) malloc(intSize);
+  if(intFlag) {
+    free(buf); // GOOD
+    buf = tmpbuf;
+  }
+  free(buf); // GOOD
+}
+void workFunction_7(char *s) {
+  int intSize = 10;
+  char *buf;
+  char *buf1;
+  buf = (char *) malloc(intSize);
+  buf1 = (char *) realloc(buf,intSize*4);
+  free(buf); // BAD
+}
+void workFunction_8(char *s) {
+  int intSize = 10;
+  char *buf;
+  char *buf1;
+  buf = (char *) malloc(intSize);
+  buf1 = (char *) realloc(buf,intSize*4);
+  if(!buf1)
+  free(buf); // GOOD
+}
+void workFunction_9(char *s) {
+  int intSize = 10;
+  char *buf;
+  char *buf1;
+  buf = (char *) malloc(intSize);
+  if(!(buf1 = (char *) realloc(buf,intSize*4)))
+  free(buf); // GOOD
+}