Merge pull request #843 from geoffw0/strtoul

CPP: Improve ArithmeticTainted.ql

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

@@ -16,18 +16,18 @@ import semmle.code.cpp.security.Overflow
 import semmle.code.cpp.security.Security
 import semmle.code.cpp.security.TaintTracking
 
-predicate taintedVarAccess(Expr origin, VariableAccess va) {
-  isUserInput(origin, _) and
-  tainted(origin, va)
-}
-
-from Expr origin, Operation op, VariableAccess va, string effect
-where taintedVarAccess(origin, va)
-  and op.getAnOperand() = va
+from Expr origin, Operation op, Expr e, string effect
+where isUserInput(origin, _)
+  and tainted(origin, e)
+  and op.getAnOperand() = e
   and
   (
-    (missingGuardAgainstUnderflow(op, va) and effect = "underflow") or
-    (missingGuardAgainstOverflow(op, va) and effect = "overflow")
+    (missingGuardAgainstUnderflow(op, e) and effect = "underflow") or
+    (missingGuardAgainstOverflow(op, e) and effect = "overflow") or
+    (not e instanceof VariableAccess and effect = "overflow")
+  ) and (
+    op instanceof UnaryArithmeticOperation or
+    op instanceof BinaryArithmeticOperation
   )
-select va, "$@ flows to here and is used in arithmetic, potentially causing an " + effect + ".",
+select e, "$@ flows to here and is used in arithmetic, potentially causing an " + effect + ".",
   origin, "User-provided value"

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected

@@ -1,6 +1,9 @@
 | test3.c:15:10:15:10 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test3.c:11:15:11:18 | argv | User-provided value |
 | test3.c:15:14:15:14 | y | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test3.c:11:15:11:18 | argv | User-provided value |
 | test3.c:15:18:15:18 | z | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test3.c:11:15:11:18 | argv | User-provided value |
+| test5.cpp:17:6:17:18 | call to getTaintedInt | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test5.cpp:9:7:9:9 | buf | User-provided value |
+| test5.cpp:19:6:19:6 | y | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test5.cpp:9:7:9:9 | buf | User-provided value |
+| test5.cpp:19:6:19:6 | y | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test5.cpp:9:7:9:9 | buf | User-provided value |
 | test.c:14:15:14:28 | maxConnections | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:11:29:11:32 | argv | User-provided value |
 | test.c:14:15:14:28 | maxConnections | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:11:29:11:32 | argv | User-provided value |
 | test.c:44:7:44:10 | len2 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:41:17:41:20 | argv | User-provided value |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected

@@ -1,3 +1,4 @@
 | test3.c:12:31:12:34 | * ... | $@ flows to here and is used in an expression which might overflow negatively. | test3.c:11:15:11:18 | argv | User-provided value |
 | test3.c:13:16:13:19 | * ... | $@ flows to here and is used in an expression which might overflow negatively. | test3.c:11:15:11:18 | argv | User-provided value |
 | test4.cpp:13:17:13:20 | access to array | $@ flows to here and is used in an expression which might overflow negatively. | test4.cpp:9:13:9:16 | argv | User-provided value |
+| test5.cpp:10:9:10:15 | call to strtoul | $@ flows to here and is used in an expression which might overflow. | test5.cpp:9:7:9:9 | buf | User-provided value |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/test5.cpp

@@ -0,0 +1,20 @@
+
+char *gets(char *s);
+unsigned long int strtoul( const char * nptr, char ** endptr, int base);
+
+int getTaintedInt()
+{
+	char buf[128];
+
+	gets(buf);
+	return strtoul(buf, 0, 10);
+}
+
+void useTaintedInt()
+{
+	int x, y;
+
+	x = getTaintedInt() * 1024; // BAD: arithmetic on a tainted value
+	y = getTaintedInt();
+	y = y * 1024; // BAD: arithmetic on a tainted value
+}