Merge pull request #5767 from ihsinme/ihsinme-patch-268

CPP: Add query for CWE-1126: Declaration of Variable with Unnecessarily Wide Scope

cpp/ql/src/experimental/Security/CWE/CWE-1126/DeclarationOfVariableWithUnnecessarilyWideScope.c

@@ -0,0 +1,14 @@
+while(intIndex > 2)
+{
+  ...
+  intIndex--;
+  ...
+} // GOOD: correct cycle
+...
+while(intIndex > 2)
+{
+  ...
+  int intIndex;
+  intIndex--;
+  ...
+} // BAD: the variable used in the condition does not change.

cpp/ql/src/experimental/Security/CWE/CWE-1126/DeclarationOfVariableWithUnnecessarilyWideScope.qhelp

@@ -0,0 +1,26 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Using variables with the same name is dangerous. However, such a situation inside the while loop can create an infinite loop exhausting resources. Requires the attention of developers.</p>
+
+</overview>
+<recommendation>
+<p>We recommend not to use local variables inside a loop if their names are the same as the variables in the condition of this loop.</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates an erroneous and corrected use of a local variable within a loop.</p>
+<sample src="DeclarationOfVariableWithUnnecessarilyWideScope.c" />
+
+</example>
+<references>
+
+<li>
+  CERT C Coding Standard:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/DCL01-C.+Do+not+reuse+variable+names+in+subscopes">DCL01-C. Do not reuse variable names in subscopes</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-1126/DeclarationOfVariableWithUnnecessarilyWideScope.ql

@@ -0,0 +1,60 @@
+/**
+ * @name Errors When Using Variable Declaration Inside Loop
+ * @description Using variables with the same name is dangerous.
+ *              However, such a situation inside the while loop can create an infinite loop exhausting resources.
+ *              Requires the attention of developers.
+ * @kind problem
+ * @id cpp/errors-when-using-variable-declaration-inside-loop
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-1126
+ */
+
+import cpp
+
+/**
+ * Errors when using a variable declaration inside a loop.
+ */
+class DangerousWhileLoop extends WhileStmt {
+  Expr exp;
+  Declaration dl;
+
+  DangerousWhileLoop() {
+    this = dl.getParentScope().(BlockStmt).getParent*() and
+    exp = this.getCondition().getAChild*() and
+    not exp instanceof PointerFieldAccess and
+    not exp instanceof ValueFieldAccess and
+    exp.(VariableAccess).getTarget().getName() = dl.getName() and
+    not exp.getParent*() instanceof FunctionCall
+  }
+
+  Declaration getDeclaration() { result = dl }
+
+  /** Holds when there are changes to the variables involved in the condition. */
+  predicate isUseThisVariable() {
+    exists(Variable v |
+      this.getCondition().getAChild*().(VariableAccess).getTarget() = v and
+      (
+        exists(Assignment aexp |
+          this = aexp.getEnclosingStmt().getParentStmt*() and
+          (
+            aexp.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() = v
+            or
+            aexp.getLValue().(VariableAccess).getTarget() = v
+          )
+        )
+        or
+        exists(CrementOperation crm |
+          this = crm.getEnclosingStmt().getParentStmt*() and
+          crm.getOperand().(VariableAccess).getTarget() = v
+        )
+      )
+    )
+  }
+}
+
+from DangerousWhileLoop lp
+where not lp.isUseThisVariable()
+select lp.getDeclaration(), "A variable with this name is used in the $@ condition.", lp, "loop"

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-1126/semmle/tests/DeclarationOfVariableWithUnnecessarilyWideScope.expected

@@ -0,0 +1 @@
+| test.c:14:9:14:16 | intIndex | A variable with this name is used in the $@ condition. | test.c:11:3:16:3 | while (...) ... | loop |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-1126/semmle/tests/DeclarationOfVariableWithUnnecessarilyWideScope.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-1126/DeclarationOfVariableWithUnnecessarilyWideScope.ql

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

@@ -0,0 +1,59 @@
+void workFunction_0(char *s) {
+  int intIndex = 10;
+  int intGuard;
+  char buf[80];
+  while(intIndex > 2) // GOOD
+  {
+    buf[intIndex] = 1;
+    intIndex--;
+  }
+  intIndex = 10;
+  while(intIndex > 2)
+  {
+    buf[intIndex] = 1;
+    int intIndex; // BAD
+    intIndex--;
+  }
+  intIndex = 10;
+  intGuard = 20;
+  while(intIndex < intGuard--) // GOOD
+  {
+    buf[intIndex] = 1;
+    int intIndex;
+    intIndex--;
+  }
+  intIndex = 10;
+  intGuard = 20;
+  while(intIndex < intGuard) // GOOD
+  {
+    buf[intIndex] = 1;
+    int intIndex;
+    intIndex++;
+    intGuard--;
+  }
+  intIndex = 10;
+  intGuard = 20;
+  while(intIndex < intGuard) // GOOD
+  {
+    buf[intIndex] = 1;
+    int intIndex;
+    intIndex--;
+    intGuard -= 4;
+  }
+  intIndex = 10;
+  while(intIndex > 2) // GOOD
+  {
+    buf[intIndex] = 1;
+    intIndex -= 2;
+    int intIndex;
+    intIndex--;
+  }
+  intIndex = 10;
+  while(intIndex > 2) // GOOD
+  {
+    buf[intIndex] = 1;
+    --intIndex;
+    int intIndex;
+    intIndex--;
+  }
+}