Merge pull request #329 from geoffw0/overflowdest

CPP: Improve Overflowdest.ql

Author: rdmarsh2

cpp/ql/src/Critical/OverflowDestination.cpp

@@ -1,13 +1,13 @@
 
 int main(int argc, char* argv[]) {
-	char param[SIZE];
+	char param[20];
+	char *arg1;
 
-	char arg1[10];
-	char arg2[20];
+	arg1 = argv[1];
 
 	//wrong: only uses the size of the source (argv[1]) when using strncpy
-	strncpy(param, argv[1], strlen(arg1));
+	strncpy(param, arg1, strlen(arg1));
 
 	//correct: uses the size of the destination array as well
-	strncpy(param, argv[1], min(strlen(arg1, sizeof(param) -1)));
+	strncpy(param, arg1, min(strlen(arg1), sizeof(param) -1));
 }

cpp/ql/src/Critical/OverflowDestination.ql

@@ -5,75 +5,46 @@
  * @kind problem
  * @id cpp/overflow-destination
  * @problem.severity warning
+ * @precision low
  * @tags reliability
  *       security
  *       external/cwe/cwe-119
  *       external/cwe/cwe-131
  */
 import cpp
-import semmle.code.cpp.pointsto.PointsTo
+import semmle.code.cpp.security.TaintTracking
 
-predicate sourceSized(FunctionCall fc)
+/**
+ * Holds if `fc` is a call to a copy operation where the size argument contains
+ * a reference to the source argument.  For example:
+ * ```
+ *   memcpy(dest, src, sizeof(src));
+ * ```
+ */
+predicate sourceSized(FunctionCall fc, Expr src)
 {
   exists(string name |
     (name = "strncpy" or name = "strncat" or name = "memcpy" or name = "memmove") and
     fc.getTarget().hasQualifiedName(name))
   and
-  exists(Expr dest, Expr src, Expr size, Variable v |
+  exists(Expr dest, Expr size, Variable v |
     fc.getArgument(0) = dest and fc.getArgument(1) = src and fc.getArgument(2) = size and
     src = v.getAnAccess() and size.getAChild+() = v.getAnAccess() and
+
+    // exception: `dest` is also referenced in the size argument
     not exists(Variable other |
       dest = other.getAnAccess() and size.getAChild+() = other.getAnAccess())
     and
+
+    // exception: `src` and `dest` are both arrays of the same type and size
     not exists(ArrayType srctype, ArrayType desttype |
       dest.getType().getUnderlyingType() = desttype and
       src.getType().getUnderlyingType() = srctype and
       desttype.getBaseType().getUnderlyingType() = srctype.getBaseType().getUnderlyingType() and
       desttype.getArraySize() = srctype.getArraySize()))
 }
 
-class VulnerableArgument extends PointsToExpr
-{
-  VulnerableArgument() { sourceSized(this.getParent()) }
-  override predicate interesting() { sourceSized(this.getParent()) }
-}
-
-predicate taintingFunction(Function f, int buf)
-{
-  (f.hasQualifiedName("read") and buf = 1) or
-  (f.hasQualifiedName("fgets") and buf = 0) or
-  (f.hasQualifiedName("fread") and buf = 0)
-}
-
-// Taint `argv[i]`, for all i, but also `*argv`, etc.
-predicate commandLineArg(Expr e)
-{
-  exists(Function f, Parameter argv, VariableAccess access |
-    f.hasQualifiedName("main") and f.getParameter(1) = argv and
-    argv.getAnAccess() = access and access.isRValue() and
-    pointer(access, e))
-}
-
-predicate tainted(Expr e)
-{
-  exists(FunctionCall fc, int arg |
-    taintingFunction(fc.getTarget(), arg) and
-    e = fc.getArgument(arg))
-  or
-  e.(FunctionCall).getTarget().hasQualifiedName("getenv")
-  or
-  commandLineArg(e)
-}
-
-class TaintedArgument extends PointsToExpr
-{
-  TaintedArgument() { tainted(this) }
-  override predicate interesting() { tainted(this) }
-}
-
-from FunctionCall fc, VulnerableArgument vuln, TaintedArgument tainted
-where sourceSized(fc)
-  and fc.getArgument(1) = vuln
-  and vuln.pointsTo() = tainted.pointsTo()
-  and vuln.confidence() > 0.01
+from FunctionCall fc, Expr vuln, Expr taintSource
+where sourceSized(fc, vuln)
+  and tainted(taintSource, vuln)
 select fc, "To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size."