Merge pull request github#5326 from ihsinme/ihsinme-patch-244

CPP: Add query for CWE-20 Improper Input Validation

Author: geoffw0

cpp/ql/src/experimental/Security/CWE/CWE-020/LateCheckOfFunctionArgument.c

@@ -0,0 +1,7 @@
+if(len<0) return 1;
+memset(dest, source, len); // GOOD: variable `len` checked before call
+
+...
+
+memset(dest, source, len); // BAD: variable `len` checked after call
+if(len<0) return 1;

cpp/ql/src/experimental/Security/CWE/CWE-020/LateCheckOfFunctionArgument.qhelp

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Checking the function argument after calling the function itself. This situation looks suspicious and requires the attention of the developer. It may be necessary to add validation before calling the function</p>
+
+
+</overview>
+<recommendation>
+
+<p>We recommend checking before calling the function.</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates an erroneous and fixed use of function argument validation.</p>
+<sample src="LateCheckOfFunctionArgument.c" />
+
+</example>
+<references>
+
+<li>
+  CWE Common Weakness Enumeration:
+  <a href="https://cwe.mitre.org/data/definitions/20.html"> CWE-20: Improper Input Validation</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-020/LateCheckOfFunctionArgument.ql

@@ -0,0 +1,66 @@
+/**
+ * @name Late Check Of Function Argument
+ * @description --Checking the function argument after calling the function itself.
+ *              --This situation looks suspicious and requires the attention of the developer.
+ *              --It may be necessary to add validation before calling the function.
+ * @kind problem
+ * @id cpp/late-check-of-function-argument
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-20
+ */
+
+import cpp
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+/** Holds for a function `f` that has an argument at index `apos` used for positioning in a buffer. */
+predicate numberArgument(Function f, int apos) {
+  f.hasGlobalOrStdName("write") and apos = 2
+  or
+  f.hasGlobalOrStdName("read") and apos = 2
+  or
+  f.hasGlobalOrStdName("lseek") and apos = 1
+  or
+  f.hasGlobalOrStdName("memmove") and apos = 2
+  or
+  f.hasGlobalOrStdName("memset") and apos = 2
+  or
+  f.hasGlobalOrStdName("memcpy") and apos = 2
+  or
+  f.hasGlobalOrStdName("memcmp") and apos = 2
+  or
+  f.hasGlobalOrStdName("strncat") and apos = 2
+  or
+  f.hasGlobalOrStdName("strncpy") and apos = 2
+  or
+  f.hasGlobalOrStdName("strncmp") and apos = 2
+  or
+  f.hasGlobalOrStdName("snprintf") and apos = 1
+  or
+  f.hasGlobalOrStdName("strndup") and apos = 2
+}
+
+class IfCompareWithZero extends IfStmt {
+  IfCompareWithZero() { this.getCondition().(RelationalOperation).getAChild().getValue() = "0" }
+
+  Expr noZerroOperand() {
+    if this.getCondition().(RelationalOperation).getGreaterOperand().getValue() = "0"
+    then result = this.getCondition().(RelationalOperation).getLesserOperand()
+    else result = this.getCondition().(RelationalOperation).getGreaterOperand()
+  }
+}
+
+from FunctionCall fc, IfCompareWithZero ifc, int na
+where
+  numberArgument(fc.getTarget(), na) and
+  globalValueNumber(fc.getArgument(na)) = globalValueNumber(ifc.noZerroOperand()) and
+  dominates(fc, ifc) and
+  not exists(IfStmt ifc1 |
+    dominates(ifc1, fc) and
+    globalValueNumber(fc.getArgument(na)) = globalValueNumber(ifc1.getCondition().getAChild*())
+  )
+select fc,
+  "The value of argument '$@' appears to be checked after the call, rather than before it.",
+  fc.getArgument(na), fc.getArgument(na).toString()

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-020/semmle/tests/LateCheckOfFunctionArgument.expected

@@ -0,0 +1 @@
+| test.c:6:3:6:8 | call to memset | The value of argument '$@' appears to be checked after the call, rather than before it. | test.c:6:17:6:20 | len1 | len1 |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-020/semmle/tests/LateCheckOfFunctionArgument.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-020/LateCheckOfFunctionArgument.ql

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

@@ -0,0 +1,8 @@
+void workFunction_0(char *s) {
+  int len = 5, len1;
+  char buf[80], buf1[8];
+  if(len<0) return;
+  memset(buf,0,len); //GOOD
+  memset(buf1,0,len1); //BAD
+  if(len1<0) return;
+}