Add new barrier guard for second half of path

Author: owen-mc

ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll

@@ -72,6 +72,15 @@ module AllocationSizeOverflow {
     }
   }
 
+  /** A check of the allocation size, acting as a guard to prevent allocation-size overflow. */
+  class AllocationSizeCheck extends DataFlow::BarrierGuard, DataFlow::RelationalComparisonNode {
+    override predicate checks(Expr e, boolean branch) {
+      exists(DataFlow::Node lesser | this.leq(branch, lesser, _, _) |
+        globalValueNumber(DataFlow::exprNode(e)) = globalValueNumber(lesser)
+      )
+    }
+  }
+
   /**
    * An arithmetic operation that might overflow, and whose result is used to compute an
    * allocation size.
@@ -81,7 +90,8 @@ module AllocationSizeOverflow {
 
     DefaultSink() {
       this instanceof OverflowProneOperand and
-      localStep*(this, allocsz)
+      localStep*(this, allocsz) and
+      not exists(AllocationSizeCheck g | allocsz = g.getAGuardedNode())
     }
 
     override DataFlow::Node getAllocationSize() { result = allocsz }

ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected

@@ -3,6 +3,8 @@ edges
 | tst2.go:9:2:9:37 | ... := ...[0] : slice type | tst2.go:10:22:10:30 | call to len |
 | tst2.go:14:2:14:29 | ... := ...[0] : slice type | tst2.go:15:22:15:30 | call to len |
 | tst3.go:6:2:6:31 | ... := ...[0] : slice type | tst3.go:7:22:7:34 | call to len |
+| tst3.go:6:2:6:31 | ... := ...[0] : slice type | tst3.go:24:16:24:28 | call to len |
+| tst3.go:6:2:6:31 | ... := ...[0] : slice type | tst3.go:32:16:32:28 | call to len |
 | tst.go:14:2:14:30 | ... = ...[0] : slice type | tst.go:15:22:15:34 | call to len |
 | tst.go:20:2:20:31 | ... = ...[0] : slice type | tst.go:21:22:21:34 | call to len |
 | tst.go:26:2:26:31 | ... = ...[0] : slice type | tst.go:27:26:27:38 | call to len |
@@ -16,6 +18,8 @@ nodes
 | tst2.go:15:22:15:30 | call to len | semmle.label | call to len |
 | tst3.go:6:2:6:31 | ... := ...[0] : slice type | semmle.label | ... := ...[0] : slice type |
 | tst3.go:7:22:7:34 | call to len | semmle.label | call to len |
+| tst3.go:24:16:24:28 | call to len | semmle.label | call to len |
+| tst3.go:32:16:32:28 | call to len | semmle.label | call to len |
 | tst.go:14:2:14:30 | ... = ...[0] : slice type | semmle.label | ... = ...[0] : slice type |
 | tst.go:15:22:15:34 | call to len | semmle.label | call to len |
 | tst.go:20:2:20:31 | ... = ...[0] : slice type | semmle.label | ... = ...[0] : slice type |
@@ -29,6 +33,8 @@ nodes
 | tst2.go:10:22:10:30 | call to len | tst2.go:9:2:9:37 | ... := ...[0] : slice type | tst2.go:10:22:10:30 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst2.go:10:22:10:32 | ...+... | allocation | tst2.go:9:2:9:37 | ... := ...[0] : slice type | value |
 | tst2.go:15:22:15:30 | call to len | tst2.go:14:2:14:29 | ... := ...[0] : slice type | tst2.go:15:22:15:30 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst2.go:15:22:15:32 | ...+... | allocation | tst2.go:14:2:14:29 | ... := ...[0] : slice type | value |
 | tst3.go:7:22:7:34 | call to len | tst3.go:6:2:6:31 | ... := ...[0] : slice type | tst3.go:7:22:7:34 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst3.go:7:22:7:36 | ...+... | allocation | tst3.go:6:2:6:31 | ... := ...[0] : slice type | value |
+| tst3.go:24:16:24:28 | call to len | tst3.go:6:2:6:31 | ... := ...[0] : slice type | tst3.go:24:16:24:28 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst3.go:27:24:27:32 | newlength | allocation | tst3.go:6:2:6:31 | ... := ...[0] : slice type | value |
+| tst3.go:32:16:32:28 | call to len | tst3.go:6:2:6:31 | ... := ...[0] : slice type | tst3.go:32:16:32:28 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst3.go:36:23:36:31 | newlength | allocation | tst3.go:6:2:6:31 | ... := ...[0] : slice type | value |
 | tst.go:15:22:15:34 | call to len | tst.go:14:2:14:30 | ... = ...[0] : slice type | tst.go:15:22:15:34 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst.go:15:22:15:36 | ...+... | allocation | tst.go:14:2:14:30 | ... = ...[0] : slice type | value |
 | tst.go:21:22:21:34 | call to len | tst.go:20:2:20:31 | ... = ...[0] : slice type | tst.go:21:22:21:34 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst.go:21:22:21:36 | ...+... | allocation | tst.go:20:2:20:31 | ... = ...[0] : slice type | value |
 | tst.go:27:26:27:38 | call to len | tst.go:26:2:26:31 | ... = ...[0] : slice type | tst.go:27:26:27:38 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst.go:27:26:27:40 | ...+... | allocation | tst.go:26:2:26:31 | ... = ...[0] : slice type | value |

ql/test/query-tests/Security/CWE-190/tst3.go

@@ -11,4 +11,36 @@ func testSanitizers(s string) {
 	if len(jsonData) < 1000 {
 		ignore(make([]byte, len(jsonData)+1)) // OK: there is an upper bound check on len(jsonData)
 	}
+
+	{
+		newlength := len(jsonData) + 2 // OK: there is an upper bound check which dominates `make`
+		_ := newlength - 1
+		if newlength < 1000 {
+			ignore(make([]byte, newlength))
+		}
+	}
+
+	{
+		newlength := len(jsonData) + 3 // NOT OK: newlength is changed after the upper bound check (even though it's made smaller)
+		if newlength < 1000 {
+			newlength = newlength - 1
+			ignore(make([]byte, newlength))
+		}
+	}
+
+	{
+		newlength := len(jsonData) + 4 // NOT OK: there is an upper bound check but it doesn't dominate `make`
+		if newlength < 1000 {
+			_ := newlength + 2
+		}
+		ignore(make([]byte, newlength))
+	}
+
+	{
+		newlength := len(jsonData) + 5 // OK: there is an upper bound check which dominates `make`
+		if newlength > 1000 {
+			return
+		}
+		ignore(make([]byte, newlength))
+	}
 }