Merge pull request #105 from geoffw0/samate-crement

CPP: Support crement operations in CWE-190

Author: jbj

change-notes/1.18/analysis-cpp.md

@@ -26,7 +26,11 @@
 | [Variable used in its own initializer] | Fewer false positive results | Results where a macro is used to indicate deliberate uninitialization are now excluded |
 | [Assignment where comparison was intended] | Fewer false positive results | Results are no longer reported if the variable is not yet defined. |
 | [Comparison where assignment was intended] | More correct results | "This query now includes results where an overloaded `operator==` is used in the wrong context. |
-
+| [User-controlled data in arithmetic expression] | More correct results | Increment / decrement / addition assignment / subtraction assignment operations are now understood as arithmetic operations in this query. |
+| [Uncontrolled data in arithmetic expression] | More correct results | Increment / decrement / addition assignment / subtraction assignment operations are now understood as arithmetic operations in this query. |
+| [Use of extreme values in arithmetic expression] | More correct results | Increment / decrement / addition assignment / subtraction assignment operations are now understood as arithmetic operations in this query. |
+| [Use of extreme values in arithmetic expression] | Fewer false positives | The query now considers whether a particular expression might cause an overflow of minimum or maximum values only. |
+ 
 ## Changes to QL libraries
 
 * *Series of bullet points*

cpp/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql

@@ -21,7 +21,7 @@ predicate taintedVarAccess(Expr origin, VariableAccess va) {
   tainted(origin, va)
 }
 
-from Expr origin, BinaryArithmeticOperation op, VariableAccess va, string effect
+from Expr origin, Operation op, VariableAccess va, string effect
 where taintedVarAccess(origin, va)
   and op.getAnOperand() = va
   and

cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql

@@ -46,7 +46,7 @@ predicate guardedByAssignDiv(Expr origin) {
          tainted(origin, va) and div.getLValue() = va)
 }
 
-from Expr origin, BinaryArithmeticOperation op, VariableAccess va, string effect
+from Expr origin, Operation op, VariableAccess va, string effect
 where taintedVarAccess(origin, va)
   and op.getAnOperand() = va
   and

cpp/ql/src/Security/CWE/CWE-190/ArithmeticWithExtremeValues.ql

@@ -40,18 +40,29 @@ class SecurityOptionsArith extends SecurityOptions {
   }
 }
 
-predicate taintedVarAccess(Expr origin, VariableAccess va) {
-  isUserInput(origin, _) and
+predicate taintedVarAccess(Expr origin, VariableAccess va, string cause) {
+  isUserInput(origin, cause) and
   tainted(origin, va)
 }
 
-from Expr origin, BinaryArithmeticOperation op, VariableAccess va, string effect
-where taintedVarAccess(origin, va)
+predicate causeEffectCorrespond(string cause, string effect) {
+  (
+    cause = "max value" and
+    effect = "overflow"
+  ) or (
+    cause = "min value" and
+    effect = "underflow"
+  )
+}
+
+from Expr origin, Operation op, VariableAccess va, string cause, string effect
+where taintedVarAccess(origin, va, cause)
   and op.getAnOperand() = va
   and
   (
     (missingGuardAgainstUnderflow(op, va) and effect = "underflow") or
     (missingGuardAgainstOverflow(op, va) and effect = "overflow")
-  )
+  ) and
+  causeEffectCorrespond(cause, effect)
 select va, "$@ flows to here and is used in arithmetic, potentially causing an " + effect + ".",
   origin, "Extreme value"

cpp/ql/src/semmle/code/cpp/security/Overflow.qll

@@ -4,7 +4,7 @@ import semmle.code.cpp.controlflow.Dominance
 /* Guarding */
 
 /** is the size of this use guarded using 'abs'? */
-predicate guardedAbs(BinaryArithmeticOperation e, Expr use) {
+predicate guardedAbs(Operation e, Expr use) {
   exists(FunctionCall fc |
     fc.getTarget().getName() = "abs" |
     fc.getArgument(0).getAChild*() = use
@@ -13,7 +13,7 @@ predicate guardedAbs(BinaryArithmeticOperation e, Expr use) {
 }
 
 /** is the size of this use guarded to be less than something? */
-predicate guardedLesser(BinaryArithmeticOperation e, Expr use) {
+predicate guardedLesser(Operation e, Expr use) {
   exists(IfStmt c, RelationalOperation guard |
     use = guard.getLesserOperand().getAChild*() and
     guard = c.getControllingExpr().getAChild*() and
@@ -33,7 +33,7 @@ predicate guardedLesser(BinaryArithmeticOperation e, Expr use) {
 }
 
 /** is the size of this use guarded to be greater than something? */
-predicate guardedGreater(BinaryArithmeticOperation e, Expr use) {
+predicate guardedGreater(Operation e, Expr use) {
   exists(IfStmt c, RelationalOperation guard |
     use = guard.getGreaterOperand().getAChild*() and
     guard = c.getControllingExpr().getAChild*() and
@@ -58,24 +58,28 @@ VariableAccess varUse(LocalScopeVariable v) {
 }
 
 /** is e not guarded against overflow by use? */
-predicate missingGuardAgainstOverflow(BinaryArithmeticOperation e, VariableAccess use) {
+predicate missingGuardAgainstOverflow(Operation e, VariableAccess use) {
   use = e.getAnOperand() and
   exists(LocalScopeVariable v | use.getTarget() = v |
     // overflow possible if large
     (e instanceof AddExpr and not guardedLesser(e, varUse(v))) or
+    (e instanceof AssignAddExpr and not guardedLesser(e, varUse(v))) or
+    (e instanceof IncrementOperation and not guardedLesser(e, varUse(v)) and v.getType().getUnspecifiedType() instanceof IntegralType) or     
     // overflow possible if large or small
     (e instanceof MulExpr and
       not (guardedLesser(e, varUse(v)) and guardedGreater(e, varUse(v))))
   )
 }
 
 /** is e not guarded against underflow by use? */
-predicate missingGuardAgainstUnderflow(BinaryArithmeticOperation e, VariableAccess use) {
+predicate missingGuardAgainstUnderflow(Operation e, VariableAccess use) {
   use = e.getAnOperand() and
   exists(LocalScopeVariable v | use.getTarget() = v |
     // underflow possible if use is left operand and small
-    (e instanceof SubExpr and
-      (use = e.getLeftOperand() and not guardedGreater(e, varUse(v)))) or
+    (use = e.(SubExpr).getLeftOperand() and not guardedGreater(e, varUse(v))) or
+    (use = e.(AssignSubExpr).getLValue() and not guardedGreater(e, varUse(v))) or
+    // underflow possible if small
+    (e instanceof DecrementOperation and not guardedGreater(e, varUse(v)) and v.getType().getUnspecifiedType() instanceof IntegralType) or
     // underflow possible if large or small
     (e instanceof MulExpr and
       not (guardedLesser(e, varUse(v)) and guardedGreater(e, varUse(v))))