From 17b3d56195941a0e2264c83f8350a0af611b26ba Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 17 Aug 2020 14:56:42 +0100 Subject: [PATCH 1/5] Remove unnecessary string concat --- ql/src/Security/CWE-190/AllocationSizeOverflow.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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" From 35e336fe96eaa474a53b8513d34fa843186b0aac Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 18 Aug 2020 18:00:19 +0100 Subject: [PATCH 2/5] Add tests for sanitizers and sanitizer guards --- .../CWE-190/AllocationSizeOverflow.expected | 4 ++++ ql/test/query-tests/Security/CWE-190/tst3.go | 14 ++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 ql/test/query-tests/Security/CWE-190/tst3.go diff --git a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected index 3439a2ad0b8..6e27a1deb1a 100644 --- a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected +++ b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected @@ -2,6 +2,7 @@ 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 | | 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 +14,8 @@ 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 | | 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 +28,7 @@ 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 | | 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..0c1251c7ccc --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/tst3.go @@ -0,0 +1,14 @@ +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) + } +} From dbf1d24e198e2a4241d2bd0f0fa3648290c18dac Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 19 Aug 2020 07:44:49 +0100 Subject: [PATCH 3/5] Add new barrier guard for second half of path --- .../AllocationSizeOverflowCustomizations.qll | 12 ++++++- .../CWE-190/AllocationSizeOverflow.expected | 6 ++++ ql/test/query-tests/Security/CWE-190/tst3.go | 32 +++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll b/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll index 0dbc26bd70e..1b7bd4bfdb9 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, _, _) | + 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 } diff --git a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected index 6e27a1deb1a..4ba3d26de43 100644 --- a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected +++ b/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 | diff --git a/ql/test/query-tests/Security/CWE-190/tst3.go b/ql/test/query-tests/Security/CWE-190/tst3.go index 0c1251c7ccc..f0abcde34f6 100644 --- a/ql/test/query-tests/Security/CWE-190/tst3.go +++ b/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)) + } } From aed3ef4cde3fe706c8040a5a545926c7091dbda8 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 24 Aug 2020 16:13:06 +0100 Subject: [PATCH 4/5] Improve performance of new barrier guard Some projects on lgtm were taking >1 hour, and with this commit they take <10 minutes --- .../semmle/go/security/AllocationSizeOverflowCustomizations.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll b/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll index 1b7bd4bfdb9..f99851a3504 100644 --- a/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll +++ b/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll @@ -75,7 +75,7 @@ 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, _, _) | + exists(DataFlow::Node lesser | this.leq(branch, lesser, _, _) and not lesser.isConst() | globalValueNumber(DataFlow::exprNode(e)) = globalValueNumber(lesser) ) } From a669fa4aa1170dd739220d1ac53fd4eaf129ef69 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 21 Aug 2020 15:35:00 +0100 Subject: [PATCH 5/5] Do not flow taint through remainder expressions If the tainted operand is the first operand then it is being bounded above by the remainder expression. If it is the second operand then --- .../go/security/AllocationSizeOverflowCustomizations.qll | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll b/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll index f99851a3504..dffd5ac098e 100644 --- a/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll +++ b/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll @@ -175,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 | @@ -184,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 } /**