Add query for CWE-758: Reliance on Undefined, Unspecified, or Implementation-Defined Behavior

cpp/ql/src/experimental/Security/CWE/CWE-758/UndefinedOrImplementationDefinedBehavior.c

@@ -0,0 +1,12 @@
+intA = ++intA + 1; // BAD: undefined behavior when changing variable `intA`
+...
+intA++;
+intA = intA + 1; // GOOD: correct design
+...
+char * buff;
+...
+if(funcAdd(buff)+fucDel(buff)>0) return 1; // BAD: undefined behavior when calling functions to change the `buff` variable
+...
+intA = funcAdd(buff);
+intB = funcDel(buff);
+if(intA+intB>0) return 1; // GOOD: correct design

cpp/ql/src/experimental/Security/CWE/CWE-758/UndefinedOrImplementationDefinedBehavior.qhelp

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>In some situations, the code constructs used may be executed in the wrong order in which the developer designed them. For example, if you call multiple functions as part of a single expression, and the functions have the ability to modify a shared resource, then the sequence in which the resource is changed can be unpredictable. These code snippets look suspicious and require the developer's attention.</p>
+
+
+</overview>
+<recommendation>
+
+<p>We recommend that you use more guaranteed, in terms of sequence of execution, coding techniques.</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates sections of code with insufficient execution sequence definition.</p>
+<sample src="UndefinedOrImplementationDefinedBehavior.c" />
+
+</example>
+<references>
+
+<li>
+  CWE Common Weakness Enumeration:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/EXP10-C.+Do+not+depend+on+the+order+of+evaluation+of+subexpressions+or+the+order+in+which+side+effects+take+place"> EXP10-C. Do not depend on the order of evaluation of subexpressions or the order in which side effects take place</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-758/UndefinedOrImplementationDefinedBehavior.ql

@@ -0,0 +1,166 @@
+/**
+ * @name Errors Of Undefined Program Behavior
+ * @description --In some situations, the code constructs used may be executed in the wrong order in which the developer designed them.
+ *              --For example, if you call multiple functions as part of a single expression, and the functions have the ability to modify a shared resource, then the sequence in which the resource is changed can be unpredictable.
+ *              --These code snippets look suspicious and require the developer's attention.
+ * @kind problem
+ * @id cpp/errors-of-undefined-program-behavior
+ * @problem.severity warning
+ * @precision medium
+ * @tags security
+ *       external/cwe/cwe-758
+ */
+
+import cpp
+import semmle.code.cpp.valuenumbering.HashCons
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+/**
+ * Threatening expressions of undefined behavior.
+ */
+class ExpressionsOfTheSameLevel extends Expr {
+  Expr exp2;
+
+  ExpressionsOfTheSameLevel() {
+    this != exp2 and
+    this.getParent() = exp2.getParent()
+  }
+
+  /** Holds if the underlying expression is a function call. */
+  predicate expressionCall() {
+    this instanceof FunctionCall and
+    exp2.getAChild*() instanceof FunctionCall and
+    not this.getParent() instanceof Operator and
+    not this.(FunctionCall).hasQualifier()
+  }
+
+  /** Holds if the underlying expression is a call to a function to free resources. */
+  predicate existsCloseOrFreeCall() {
+    (
+      globalValueNumber(this.(FunctionCall).getAnArgument()) =
+        globalValueNumber(exp2.getAChild*().(FunctionCall).getAnArgument()) or
+      hashCons(this.(FunctionCall).getAnArgument()) =
+        hashCons(exp2.getAChild*().(FunctionCall).getAnArgument())
+    ) and
+    (
+      this.(FunctionCall).getTarget().hasGlobalOrStdName("close") or
+      this.(FunctionCall).getTarget().hasGlobalOrStdName("free") or
+      this.(FunctionCall).getTarget().hasGlobalOrStdName("fclose")
+    )
+  }
+
+  /** Holds if the arguments in the function can be changed. */
+  predicate generalArgumentDerivedType() {
+    exists(Parameter prt1, Parameter prt2, AssignExpr aet1, AssignExpr aet2, int i, int j |
+      not this.(FunctionCall).getArgument(i).isConstant() and
+      hashCons(this.(FunctionCall).getArgument(i)) =
+        hashCons(exp2.getAChild*().(FunctionCall).getArgument(j)) and
+      prt1 = this.(FunctionCall).getTarget().getParameter(i) and
+      prt2 = exp2.getAChild*().(FunctionCall).getTarget().getParameter(j) and
+      prt1.getType() instanceof DerivedType and
+      (
+        aet1 = this.(FunctionCall).getTarget().getEntryPoint().getASuccessor*() and
+        (
+          aet1.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() =
+            prt1.getAnAccess().getTarget() or
+          aet1.getLValue().(VariableAccess).getTarget() = prt1.getAnAccess().getTarget()
+        )
+        or
+        exists(FunctionCall fc1 |
+          fc1.getTarget().hasGlobalName("memcpy") and
+          fc1.getArgument(0).(VariableAccess).getTarget() = prt1.getAnAccess().getTarget() and
+          fc1 = this.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
+        )
+      ) and
+      (
+        aet2 = exp2.getAChild*().(FunctionCall).getTarget().getEntryPoint().getASuccessor*() and
+        (
+          aet2.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() =
+            prt2.getAnAccess().getTarget() or
+          aet2.getLValue().(VariableAccess).getTarget() = prt2.getAnAccess().getTarget()
+        )
+        or
+        exists(FunctionCall fc1 |
+          fc1.getTarget().hasGlobalName("memcpy") and
+          fc1.getArgument(0).(VariableAccess).getTarget() = prt2.getAnAccess().getTarget() and
+          fc1 = exp2.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
+        )
+      )
+    )
+  }
+
+  /** Holds if functions have a common global argument. */
+  predicate generalGlobalArgument() {
+    exists(Declaration dl, AssignExpr aet1, AssignExpr aet2 |
+      dl instanceof GlobalVariable and
+      (
+        (
+          aet1.getLValue().(Access).getTarget() = dl or
+          aet1.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() = dl
+        ) and
+        aet1 = this.(FunctionCall).getTarget().getEntryPoint().getASuccessor*() and
+        not aet1.getRValue().isConstant()
+        or
+        exists(FunctionCall fc1 |
+          fc1.getTarget().hasGlobalName("memcpy") and
+          fc1.getArgument(0).(VariableAccess).getTarget() = dl and
+          fc1 = this.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
+        )
+      ) and
+      (
+        (
+          aet2.getLValue().(Access).getTarget() = dl or
+          aet2.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() = dl
+        ) and
+        aet2 = exp2.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
+        or
+        exists(FunctionCall fc1 |
+          fc1.getTarget().hasGlobalName("memcpy") and
+          fc1.getArgument(0).(VariableAccess).getTarget() = dl and
+          fc1 = exp2.(FunctionCall).getTarget().getEntryPoint().getASuccessor*()
+        )
+      )
+    )
+  }
+
+  /** Holds if sequence point is not present in expression. */
+  predicate orderOfActionExpressions() {
+    not this.getParent() instanceof BinaryLogicalOperation and
+    not this.getParent() instanceof ConditionalExpr and
+    not this.getParent() instanceof Loop and
+    not this.getParent() instanceof CommaExpr
+  }
+
+  /** Holds if expression is crement. */
+  predicate dangerousCrementChanges() {
+    hashCons(this.(CrementOperation).getOperand()) = hashCons(exp2.(CrementOperation).getOperand())
+    or
+    hashCons(this.(CrementOperation).getOperand()) = hashCons(exp2)
+    or
+    hashCons(this.(CrementOperation).getOperand()) = hashCons(exp2.(ArrayExpr).getArrayOffset())
+    or
+    hashCons(this.(Assignment).getLValue()) = hashCons(exp2.(Assignment).getLValue())
+    or
+    not this.getAChild*() instanceof Call and
+    (
+      hashCons(this.getAChild*().(CrementOperation).getOperand()) = hashCons(exp2) or
+      hashCons(this.getAChild*().(CrementOperation).getOperand()) =
+        hashCons(exp2.(Assignment).getLValue())
+    )
+  }
+}
+
+from ExpressionsOfTheSameLevel eots
+where
+  eots.orderOfActionExpressions() and
+  (
+    eots.expressionCall() and
+    (
+      eots.generalArgumentDerivedType() or
+      eots.generalGlobalArgument() or
+      eots.existsCloseOrFreeCall()
+    )
+    or
+    eots.dangerousCrementChanges()
+  )
+select eots, "This expression may have undefined behavior."

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-758/semmle/tests/UndefinedOrImplementationDefinedBehavior.expected

@@ -0,0 +1,3 @@
+| test.c:13:10:13:21 | call to tmpFunction1 | This expression may have undefined behavior. |
+| test.c:13:30:13:41 | call to tmpFunction2 | This expression may have undefined behavior. |
+| test.c:16:15:16:20 | ... ++ | This expression may have undefined behavior. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-758/semmle/tests/UndefinedOrImplementationDefinedBehavior.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-758/UndefinedOrImplementationDefinedBehavior.ql

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

@@ -0,0 +1,19 @@
+char tmpFunction1(char * buf)
+{
+  buf[1]=buf[1] + buf[2] + buf[3];
+  return buf[1];
+}
+char tmpFunction2(char * buf)
+{
+  buf[2]=buf[1] + buf[2] + buf[3];
+  return buf[2];
+}
+void workFunction_0(char *s, char * buf) {
+  int intA;
+  intA = tmpFunction1(buf) + tmpFunction2(buf);  // BAD
+  intA = tmpFunction1(buf);  //GOOD
+  intA += tmpFunction2(buf);  // GOOD
+  buf[intA] = intA++; // BAD
+  intA++;
+  buf[intA] = intA; // GOOD
+}