diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index d8eea2c7d06..d6ce6af2a19 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -17,6 +17,7 @@ The CodeQL library for Go now contains a folder of simple "cookbook" queries tha | Impossible interface nil check (`go/impossible-interface-nil-check`) | correctness | Highlights code that compares an interface value that cannot be `nil` to `nil`, suggesting a logic error. Results are shown on LGTM by default. | | Incomplete URL scheme check (`go/incomplete-url-scheme-check`) | correctness, security, external/cwe/cwe-020 | Highlights checks for `javascript` URLs that do not take `data` or `vbscript` URLs into account. Results are shown on LGTM by default. | | Potentially unsafe quoting (`go/unsafe-quoting`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-089, external/cwe/cwe-094 | Highlights code that constructs a quoted string literal containing data that may itself contain quotes. Results are shown on LGTM by default. | +| Size computation for allocation may overflow (`go/allocation-size-overflow`) | correctness, security, external/cwe/cwe-190 | Highlights code that computes the size of an allocation based on the size of a potentially large object. Results are shown on LGTM by default. | ## Changes to existing queries diff --git a/ql/src/Security/CWE-190/AllocationSizeOverflow.go b/ql/src/Security/CWE-190/AllocationSizeOverflow.go new file mode 100644 index 00000000000..aa11afa816a --- /dev/null +++ b/ql/src/Security/CWE-190/AllocationSizeOverflow.go @@ -0,0 +1,14 @@ +package main + +import "encoding/json" + +func encryptValue(v interface{}) ([]byte, error) { + jsonData, err := json.Marshal(v) + if err != nil { + return nil, err + } + size := len(jsonData) + (len(jsonData) % 16) + buffer := make([]byte, size) + copy(buffer, jsonData) + return encryptBuffer(buffer) +} diff --git a/ql/src/Security/CWE-190/AllocationSizeOverflow.qhelp b/ql/src/Security/CWE-190/AllocationSizeOverflow.qhelp new file mode 100644 index 00000000000..737efe713cd --- /dev/null +++ b/ql/src/Security/CWE-190/AllocationSizeOverflow.qhelp @@ -0,0 +1,59 @@ + + + + +

+Performing calculations involving the size of potentially large strings or slices can result in an +overflow (for signed integer types) or a wraparound (for unsigned types). An overflow causes the +result of the calculation to become negative, while a wraparound results in a small (positive) +number. +

+

+This can cause further issues. If, for example, the result is then used in an allocation, it will +cause a runtime panic if it is negative, and allocate an unexpectedly small buffer otherwise. +

+
+ + +

+Always guard against overflow in arithmetic operations involving potentially large numbers by doing +one of the following: +

+ +
+ + +

+In the following example, assume that there is a function encryptBuffer that encrypts +byte slices whose length must be padded to be a multiple of 16. The function +encryptValue provides a convenience wrapper around this function: when passed an +arbitrary value, it first encodes that value as JSON, pads the resulting byte slice, and then passes +it to encryptBuffer. +

+ +

+When passed a value whose JSON encoding is close to the maximum value of type int in +length, the computation of size will overflow, producing a negative value. When that +negative value is passed to make, a runtime panic will occur. +

+

+To guard against this, the function should be improved to check the length of the JSON-encoded +value. For example, here is a version of encryptValue that ensures the value is no +larger than 64 MB, which fits comfortably within an int and avoids the overflow: +

+ +
+ + +
  • The Go Programming Language Specification: Integer overflow.
  • +
  • The Go Programming Language Specification: Making slices, maps and channels.
  • +
    +
    diff --git a/ql/src/Security/CWE-190/AllocationSizeOverflow.ql b/ql/src/Security/CWE-190/AllocationSizeOverflow.ql new file mode 100644 index 00000000000..d42d2de8407 --- /dev/null +++ b/ql/src/Security/CWE-190/AllocationSizeOverflow.ql @@ -0,0 +1,25 @@ +/** + * @name Size computation for allocation may overflow + * @description When computing the size of an allocation based on the size of a large object, + * the result may overflow and cause a runtime panic. + * @kind path-problem + * @problem.severity warning + * @precision high + * @id go/allocation-size-overflow + * @tags security + * external/cwe/cwe-190 + */ + +import go +import DataFlow::PathGraph +import semmle.go.security.AllocationSizeOverflow + +from + AllocationSizeOverflow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, + DataFlow::Node allocsz +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.", allocsz, "allocation", source, "value" diff --git a/ql/src/Security/CWE-190/AllocationSizeOverflowGood.go b/ql/src/Security/CWE-190/AllocationSizeOverflowGood.go new file mode 100644 index 00000000000..fdab927bf85 --- /dev/null +++ b/ql/src/Security/CWE-190/AllocationSizeOverflowGood.go @@ -0,0 +1,20 @@ +package main + +import ( + "encoding/json" + "errors" +) + +func encryptValueGood(v interface{}) ([]byte, error) { + jsonData, err := json.Marshal(v) + if err != nil { + return nil, err + } + if len(jsonData) > 64*1024*1024 { + return nil, errors.New("value too large") + } + size := len(jsonData) + (len(jsonData) % 16) + buffer := make([]byte, size) + copy(buffer, jsonData) + return encryptBuffer(buffer) +} diff --git a/ql/src/semmle/go/security/AllocationSizeOverflow.qll b/ql/src/semmle/go/security/AllocationSizeOverflow.qll new file mode 100644 index 00000000000..3cc98973b06 --- /dev/null +++ b/ql/src/semmle/go/security/AllocationSizeOverflow.qll @@ -0,0 +1,41 @@ +/** + * Provides a taint-tracking tracking configuration for reasoning about allocation-size overflow. + * + * Note, for performance reasons: only import this file if `AllocationSizeOverflow::Configuration` + * is needed, otherwise `AllocationSizeOverflowCustomizations` should be imported instead. + */ + +import go + +module AllocationSizeOverflow { + import AllocationSizeOverflowCustomizations::AllocationSizeOverflow + + /** + * A taint-tracking configuration for identifying allocation-size overflows. + */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "AllocationSizeOverflow" } + + override predicate isSource(DataFlow::Node nd) { nd instanceof Source } + + /** + * Holds if `nd` is at a position where overflow might occur, and its result is used to compute + * allocation size `allocsz`. + */ + predicate isSink(DataFlow::Node nd, DataFlow::Node allocsz) { + nd.(Sink).getAllocationSize() = allocsz + } + + override predicate isSink(DataFlow::Node nd) { isSink(nd, _) } + + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + additionalStep(pred, succ) + } + + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { + guard instanceof SanitizerGuard + } + + override predicate isSanitizer(DataFlow::Node nd) { nd instanceof Sanitizer } + } +} diff --git a/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll b/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll new file mode 100644 index 00000000000..731e6b87a99 --- /dev/null +++ b/ql/src/semmle/go/security/AllocationSizeOverflowCustomizations.qll @@ -0,0 +1,176 @@ +/** + * Provides default sources, sinks, and sanitizers for reasoning about allocation-size overflows, + * as well as extension points for adding your own. + */ + +import go + +module AllocationSizeOverflow { + /** + * A source of data that might cause an allocation-size overflow. + */ + abstract class Source extends DataFlow::Node { } + + /** + * A data-flow node where an overflow might occur, and whose result is used to compute + * an allocation size. + */ + abstract class Sink extends DataFlow::Node { + /** Gets the allocation size which is influenced by the result of this node. */ + abstract DataFlow::Node getAllocationSize(); + } + + /** + * A guard node that prevents allocation-size overflow. + */ + abstract class SanitizerGuard extends DataFlow::BarrierGuard { } + + /** + * A sanitizer node that prevents allocation-size overflow. + */ + abstract class Sanitizer extends DataFlow::Node { } + + /** A call to a marshaling function, considered as a source of taint. */ + class MarshalingSource extends Source { + MarshalingSource() { + exists(MarshalingFunction marshal, DataFlow::CallNode call | + call = marshal.getACall() and + // rule out cases where we can tell that the result will always be small + not forall(FunctionInput inp | inp = marshal.getAnInput() | + isSmall(inp.getNode(call).asExpr()) + ) and + this = marshal.getOutput().getNode(call) + ) + } + } + + /** + * A call to a function that reads from the file system or a stream, considered as + * a source of taint. + */ + class FileReadSource extends Source { + FileReadSource() { + exists(Function read | + read.hasQualifiedName("io/ioutil", "ReadAll") or + read.hasQualifiedName("io/ioutil", "ReadFile") + | + this = DataFlow::extractTupleElement(read.getACall(), 0) + ) + } + } + + /** + * An arithmetic operation that might overflow, and whose result is used to compute an + * allocation size. + */ + class DefaultSink extends Sink { + DataFlow::Node allocsz; + + DefaultSink() { + exists(OperatorExpr parent | isOverflowProne(parent) | + this.asExpr() = parent.getAnOperand() and + // don't flag a sink if it is an operand of another sink to avoid double reporting + not exists(OperatorExpr grandparent | parent = grandparent.getAnOperand().stripParens() | + isOverflowProne(grandparent) + ) + ) and + isAllocationSize(allocsz) and + localStep*(this, allocsz) + } + + override DataFlow::Node getAllocationSize() { result = allocsz } + } + + /** A length check, acting as a guard to prevent allocation-size overflow. */ + class LengthCheck extends SanitizerGuard, DataFlow::RelationalComparisonNode { + override predicate checks(Expr e, boolean branch) { + exists(DataFlow::CallNode lesser | this.leq(branch, lesser, _, _) | + lesser = Builtin::len().getACall() and + globalValueNumber(DataFlow::exprNode(e)) = globalValueNumber(lesser.getArgument(0)) + ) + } + } + + /** + * A conversion to a 64-bit type, acting as a sanitizer to mitigate the risk of allocation-size + * overflow. + * + * Such conversions do not really protect against overflow, but code bases dealing with strings + * or slices large enough to overflow 64 bits are likely to encounter other problems before the + * overflow happens. + */ + class WidenTo64BitSanitizer extends Sanitizer { + WidenTo64BitSanitizer() { this.getType().(NumericType).getSize() >= 64 } + } + + /** Holds if `e` is an arithmetic expression that could cause overflow. */ + private predicate isOverflowProne(ArithmeticBinaryExpr e) { + (e instanceof AddExpr or e instanceof MulExpr) and + // rule out string concatenation + e.getType() instanceof NumericType + } + + /** Holds if `nd` is a data-flow node whose result is interpreted as an allocation size. */ + private predicate isAllocationSize(DataFlow::Node nd) { + nd = Builtin::make().getACall().getArgument(0) + } + + /** Holds if `t` is a type whose values are likely to marshal to relatively small blobs. */ + private predicate isSmallType(Type t) { + t instanceof BasicType and + not t instanceof StringType + or + isSmallType(t.(NamedType).getUnderlyingType()) + or + isSmallType(t.(PointerType).getBaseType()) + or + isSmallType(t.(ArrayType).getElementType()) + or + exists(StructType st | st = t | forall(Field f | f = st.getField(_) | isSmallType(f.getType()))) + } + + /** Holds if `e` is an expression whose values are likely to marshal to relatively small blobs. */ + private predicate isSmall(Expr e) { + isSmallType(e.getType()) + or + exists(e.getExactValue()) + or + exists(KeyValueExpr kv | kv = e | + isSmall(kv.getKey()) and + isSmall(kv.getValue()) + ) + or + isSmallCompositeLit(e, 0) + } + + /** Holds if elements `n` and above of `lit` are small. */ + private predicate isSmallCompositeLit(CompositeLit lit, int n) { + n = lit.getNumElement() + or + isSmall(lit.getElement(n)) and + isSmallCompositeLit(lit, n + 1) + } + + /** + * Holds if the value of `pred` can flow into `succ` in one step, either through a call to `len` + * or through an arithmetic operation. + */ + predicate additionalStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::CallNode c | + c = Builtin::len().getACall() and + pred = c.getArgument(0) and + succ = c + ) + or + succ.asExpr().(ArithmeticExpr).getAnOperand() = pred.asExpr() + } + + /** + * Holds if the value of `pred` can flow into `succ` in one step, either by a standard taint step + * or by an additional step. + */ + private predicate localStep(DataFlow::Node pred, DataFlow::Node succ) { + TaintTracking::localTaintStep(pred, succ) or + additionalStep(pred, succ) + } +} diff --git a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected new file mode 100644 index 00000000000..27173f3ac4c --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected @@ -0,0 +1,31 @@ +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 | +| 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:22:27:34 | call to len | +| tst.go:34:2:34:30 | ... = ...[0] : slice type | tst.go:35:22:35:34 | call to len | +nodes +| AllocationSizeOverflow.go:6:2:6:33 | ... := ...[0] : slice type | semmle.label | ... := ...[0] : slice type | +| AllocationSizeOverflow.go:10:10:10:22 | call to len | semmle.label | call to len | +| tst2.go:9:2:9:37 | ... := ...[0] : slice type | semmle.label | ... := ...[0] : slice type | +| 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 | +| 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 | +| tst.go:21:22:21:34 | call to len | semmle.label | call to len | +| tst.go:26:2:26:31 | ... = ...[0] : slice type | semmle.label | ... = ...[0] : slice type | +| tst.go:27:22:27:34 | call to len | semmle.label | call to len | +| tst.go:34:2:34:30 | ... = ...[0] : slice type | semmle.label | ... = ...[0] : slice type | +| tst.go:35:22:35:34 | call to len | semmle.label | call to len | +#select +| 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 | +| 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:22:27:34 | call to len | tst.go:26:2:26:31 | ... = ...[0] : slice type | tst.go:27:22:27:34 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst.go:27:22:27:36 | ...+... | allocation | tst.go:26:2:26:31 | ... = ...[0] : slice type | value | +| tst.go:35:22:35:34 | call to len | tst.go:34:2:34:30 | ... = ...[0] : slice type | tst.go:35:22:35:34 | call to len | This operation, which is used in an $@, involves a potentially large $@ and might overflow. | tst.go:35:22:35:36 | ...+... | allocation | tst.go:34:2:34:30 | ... = ...[0] : slice type | value | diff --git a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.go b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.go new file mode 100644 index 00000000000..aa11afa816a --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.go @@ -0,0 +1,14 @@ +package main + +import "encoding/json" + +func encryptValue(v interface{}) ([]byte, error) { + jsonData, err := json.Marshal(v) + if err != nil { + return nil, err + } + size := len(jsonData) + (len(jsonData) % 16) + buffer := make([]byte, size) + copy(buffer, jsonData) + return encryptBuffer(buffer) +} diff --git a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.qlref b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.qlref new file mode 100644 index 00000000000..3f69c90d756 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.qlref @@ -0,0 +1 @@ +Security/CWE-190/AllocationSizeOverflow.ql diff --git a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflowGood.go b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflowGood.go new file mode 100644 index 00000000000..fdab927bf85 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflowGood.go @@ -0,0 +1,20 @@ +package main + +import ( + "encoding/json" + "errors" +) + +func encryptValueGood(v interface{}) ([]byte, error) { + jsonData, err := json.Marshal(v) + if err != nil { + return nil, err + } + if len(jsonData) > 64*1024*1024 { + return nil, errors.New("value too large") + } + size := len(jsonData) + (len(jsonData) % 16) + buffer := make([]byte, size) + copy(buffer, jsonData) + return encryptBuffer(buffer) +} diff --git a/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflowGood2.go b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflowGood2.go new file mode 100644 index 00000000000..2d91d679d5f --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflowGood2.go @@ -0,0 +1,16 @@ +package main + +import ( + "encoding/json" +) + +func encryptValueGood(v interface{}) ([]byte, error) { + jsonData, err := json.Marshal(v) + if err != nil { + return nil, err + } + size := uint64(len(jsonData)) + (uint64(len(jsonData)) % 16) + buffer := make([]byte, size) + copy(buffer, jsonData) + return encryptBuffer(buffer) +} diff --git a/ql/test/query-tests/Security/CWE-190/stubs.go b/ql/test/query-tests/Security/CWE-190/stubs.go new file mode 100644 index 00000000000..d619c93ff8e --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/stubs.go @@ -0,0 +1,5 @@ +package main + +func encryptBuffer(buf []byte) ([]byte, error) { + return buf, nil +} diff --git a/ql/test/query-tests/Security/CWE-190/tst.go b/ql/test/query-tests/Security/CWE-190/tst.go new file mode 100644 index 00000000000..a8e9f508566 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/tst.go @@ -0,0 +1,38 @@ +package main + +import "encoding/json" + +type header struct { + version int + key [4]byte +} + +func test(x int, s string, xs []int, ys [16]int, ss [16]string, h *header) { + jsonData, _ := json.Marshal(x) + ignore(make([]byte, len(jsonData)+1)) // OK: data is small + + jsonData, _ = json.Marshal(s) + ignore(make([]byte, len(jsonData)+1)) // NOT OK: data might be big + + jsonData, _ = json.Marshal("hi there") + ignore(make([]byte, len(jsonData)+1)) // OK: data is small + + jsonData, _ = json.Marshal(xs) + ignore(make([]byte, len(jsonData)+1)) // NOT OK: data might be big + + jsonData, _ = json.Marshal(ys) + ignore(make([]byte, len(jsonData)+1)) // OK: data is small + + jsonData, _ = json.Marshal(ss) + ignore(make([]byte, len(jsonData)+1)) // NOT OK: data might be big + + jsonData, _ = json.Marshal(h) + ignore(make([]byte, len(jsonData)+1)) // OK: data is small + + var i interface{} + i = h + jsonData, _ = json.Marshal(i) + ignore(make([]byte, len(jsonData)+1)) // NOT OK: data might be big +} + +func ignore(_ interface{}) {} diff --git a/ql/test/query-tests/Security/CWE-190/tst2.go b/ql/test/query-tests/Security/CWE-190/tst2.go new file mode 100644 index 00000000000..a0b89ac1828 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-190/tst2.go @@ -0,0 +1,16 @@ +package main + +import ( + "io" + "io/ioutil" +) + +func test2(filename string) { + data, _ := ioutil.ReadFile(filename) + ignore(make([]byte, len(data)+1)) // NOT OK +} + +func test3(r io.Reader) { + data, _ := ioutil.ReadAll(r) + ignore(make([]byte, len(data)+1)) // NOT OK +}