hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-01-30 06:42:57 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #296 from owen-mc/allocation-size-overflow-improve-sanitizers-easy

Browse Source 
Add new sanitizer guard to Allocation size overflow query
This commit is contained in:
Max Schaefer
2020-08-28 07:44:45 +01:00
committed by GitHub
parent b4550f244b a669fa4aa1
commit 031a48ecd3
4 changed files with 71 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
ql/src/Security/CWE-190/AllocationSizeOverflow.ql
View File

@@ -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"

17
ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll
View File

@@ -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
}
/**

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

@@ -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 |

46
ql/test/query-tests/Security/CWE-190/tst3.go Normal file
View File

@@ -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))
}
}