diff --git a/ql/src/Security/CWE-190/AllocationSizeOverflow.ql b/ql/src/Security/CWE-190/AllocationSizeOverflow.ql index 122fb5f7d8e..e6d2ec0a5cc 100644 --- a/ql/src/Security/CWE-190/AllocationSizeOverflow.ql +++ b/ql/src/Security/CWE-190/AllocationSizeOverflow.ql @@ -21,5 +21,5 @@ where cfg.hasFlowPath(source, sink) and cfg.isSink(sink.getNode(), allocsz) select sink, source, sink, - "This operation, which is used in an $@, involves a potentially large $@ " + "and might overflow.", + "This operation, which is used in an $@, involves a potentially large $@ and might overflow.", allocsz, "allocation", source, "value" diff --git a/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll b/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll index 0dbc26bd70e..dffd5ac098e 100644 --- a/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll +++ b/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, _, _) and not lesser.isConst() | + 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 } @@ -165,7 +175,7 @@ module AllocationSizeOverflow { /** * Holds if the value of `pred` can flow into `succ` in one step, either through a call to `len` - * or through an arithmetic operation. + * or through an arithmetic operation (other than remainder). */ predicate additionalStep(DataFlow::Node pred, DataFlow::Node succ) { exists(DataFlow::CallNode c | @@ -174,7 +184,8 @@ module AllocationSizeOverflow { succ = c ) or - succ.asExpr().(ArithmeticExpr).getAnOperand() = pred.asExpr() + succ.asExpr().(ArithmeticExpr).getAnOperand() = pred.asExpr() and + not succ.asExpr() instanceof RemExpr } /** diff --git a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected index 3439a2ad0b8..4ba3d26de43 100644 --- a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected +++ b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected @@ -2,6 +2,9 @@ edges | AllocationSizeOverflow.go:6:2:6:33 | ... := ...[0] : slice type | AllocationSizeOverflow.go:10:10:10:22 | call to len | | 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 | @@ -13,6 +16,10 @@ nodes | tst2.go:10:22:10:30 | call to len | semmle.label | call to len | | tst2.go:14:2:14:29 | ... := ...[0] : slice type | semmle.label | ... := ...[0] : slice type | | 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 | @@ -25,6 +32,9 @@ nodes | AllocationSizeOverflow.go:10:10:10:22 | call to len | AllocationSizeOverflow.go:6:2:6:33 | ... := ...[0] : slice type | AllocationSizeOverflow.go:10:10:10:22 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | AllocationSizeOverflow.go:11:25:11:28 | size | allocation | AllocationSizeOverflow.go:6:2:6:33 | ... := ...[0] : slice type | value | | 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 | diff --git a/ql/test/query-tests/Security/CWE-190/tst3.go b/ql/test/query-tests/Security/CWE-190/tst3.go new file mode 100644 index 00000000000..f0abcde34f6 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/tst3.go @@ -0,0 +1,46 @@ +package main + +import "encoding/json" + +func testSanitizers(s string) { + jsonData, _ := json.Marshal(s) + ignore(make([]byte, len(jsonData)+1)) // NOT OK: data might be big + + ignore(make([]byte, int64(len(jsonData))+1)) // OK: sanitized by widening to 64 bits + + 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)) + } +}