Merge pull request #5094 from MathiasVP/promote-UnsignedDifferenceExpressionComparedZero

Promote cpp/unsigned-difference-expression-compared-zero out of experimental

Author: geoffw0

cpp/change-notes/2020-02-04-unsigned-difference-expression-compared-zero.md

@@ -0,0 +1,2 @@
+lgtm
+* A new query (`cpp/unsigned-difference-expression-compared-zero`) has been added. The query finds unsigned subtractions used in relational comparisons with the value 0.

cpp/config/suites/cpp/correctness

@@ -10,6 +10,7 @@
 + semmlecode-cpp-queries/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql: /Correctness/Dangerous Conversions
 + semmlecode-cpp-queries/Security/CWE/CWE-253/HResultBooleanConversion.ql: /Correctness/Dangerous Conversions
 + semmlecode-cpp-queries/Likely Bugs/OO/UnsafeUseOfThis.ql: /Correctness/Dangerous Conversions
++ semmlecode-cpp-queries/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql: /Correctness/Dangerous Conversions
   # Consistent Use
 + semmlecode-cpp-queries/Critical/ReturnValueIgnored.ql: /Correctness/Consistent Use
 + semmlecode-cpp-queries/Likely Bugs/InconsistentCheckReturnNull.ql: /Correctness/Consistent Use

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.c

@@ -0,0 +1,5 @@
+unsigned limit = get_limit();
+unsigned total = 0;
+while (limit - total > 0) { // wrong: if `total` is greater than `limit` this will underflow and continue executing the loop.
+  total += get_data();
+}

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+This rule finds relational comparisons between the result of an unsigned subtraction and the value <code>0</code>.
+Such comparisons are likely to be wrong as the value of an unsigned subtraction can never be negative. So the
+relational comparison ends up checking whether the result of the subtraction is equal to <code>0</code>.
+This is probably not what the programmer intended.
+</p>
+</overview>
+<recommendation>
+
+<p>If a relational comparison is intended, consider casting the result of the subtraction to a signed type.
+   If the intention was to test for equality, consider replacing the relational comparison with an equality test.
+</p>
+
+</recommendation>
+<example>
+<sample src="UnsignedDifferenceExpressionComparedZero.c" />
+
+</example>
+<references>
+
+<li>SEI CERT C Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules">INT02-C. Understand integer conversion rules</a>.
+</li>
+
+</references>
+</qhelp>

…ignedDifferenceExpressionComparedZero.ql …ignedDifferenceExpressionComparedZero.qlcpp/ql/src/experimental/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql renamed to cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql cpp/ql/src/experimental/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql renamed to cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql

@@ -1,13 +1,12 @@
 /**
  * @name Unsigned difference expression compared to zero
- * @description It is highly probable that the condition is wrong if the difference expression has the unsigned type.
- *              The condition holds in all the cases when difference is not equal to zero. It means that we may use condition not equal.
- *              But the programmer probably wanted to compare the difference of elements.
+ * @description A subtraction with an unsigned result can never be negative. Using such an expression in a relational comparison with `0` is likely to be wrong.
  * @kind problem
  * @id cpp/unsigned-difference-expression-compared-zero
  * @problem.severity warning
  * @precision medium
  * @tags security
+ *       correctness
  *       external/cwe/cwe-191
  */
 
@@ -17,24 +16,34 @@ import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import semmle.code.cpp.controlflow.Guards
 
+/** Holds if `sub` is guarded by a condition which ensures that `left >= right`. */
+pragma[noinline]
+predicate isGuarded(SubExpr sub, Expr left, Expr right) {
+  exists(GuardCondition guard |
+    guard.controls(sub.getBasicBlock(), true) and
+    guard.ensuresLt(left, right, 0, sub.getBasicBlock(), false)
+  )
+}
+
 /** Holds if `sub` will never be negative. */
 predicate nonNegative(SubExpr sub) {
   not exprMightOverflowNegatively(sub.getFullyConverted())
   or
   // The subtraction is guarded by a check of the form `left >= right`.
-  exists(GuardCondition guard, Expr left, Expr right |
-    left = globalValueNumber(sub.getLeftOperand()).getAnExpr() and
-    right = globalValueNumber(sub.getRightOperand()).getAnExpr() and
-    guard.controls(sub.getBasicBlock(), true) and
-    guard.ensuresLt(left, right, 0, sub.getBasicBlock(), false)
+  exists(GVN left, GVN right |
+    // This is basically a poor man's version of a directional unbind operator.
+    strictcount([left, globalValueNumber(sub.getLeftOperand())]) = 1 and
+    strictcount([right, globalValueNumber(sub.getRightOperand())]) = 1 and
+    isGuarded(sub, left.getAnExpr(), right.getAnExpr())
   )
 }
 
 from RelationalOperation ro, SubExpr sub
 where
   not isFromMacroDefinition(ro) and
+  not isFromMacroDefinition(sub) and
   ro.getLesserOperand().getValue().toInt() = 0 and
   ro.getGreaterOperand() = sub and
   sub.getFullyConverted().getUnspecifiedType().(IntegralType).isUnsigned() and
   not nonNegative(sub)
-select ro, "Difference in condition is always greater than or equal to zero"
+select ro, "Unsigned subtraction can never be negative."

cpp/ql/src/experimental/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.c

@@ -0,0 +1,10 @@
+| test.cpp:6:5:6:13 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:10:8:10:24 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:15:9:15:25 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:32:12:32:20 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:39:12:39:20 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:47:5:47:13 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:55:5:55:13 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:62:5:62:13 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:69:5:69:13 | ... > ... | Unsigned subtraction can never be negative. |
+| test.cpp:75:8:75:16 | ... > ... | Unsigned subtraction can never be negative. |