Merge pull request #55 from max-schaefer/tainted-arithmetic

Add new query `AllocationSizeOverflow`.

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
 

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

ql/src/Security/CWE-190/AllocationSizeOverflow.qhelp

@@ -0,0 +1,59 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+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.
+</p>
+<p>
+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.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Always guard against overflow in arithmetic operations involving potentially large numbers by doing
+one of the following:
+</p>
+<ul>
+<li>Validate the size of the data from which the numbers are computed.</li>
+<li>Define a guard on the arithmetic expression, so that the operation is performed only if the
+result can be known to be less than, or equal to, the maximum value for the type.</li>
+<li>Use a wider type (such as <code>uint64</code> instead of <code>int</code>), so that larger input
+values do not cause overflow.</li>
+</ul>
+</recommendation>
+
+<example>
+<p>
+In the following example, assume that there is a function <code>encryptBuffer</code> that encrypts
+byte slices whose length must be padded to be a multiple of 16. The function
+<code>encryptValue</code> 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 <code>encryptBuffer</code>.
+</p>
+<sample src="AllocationSizeOverflow.go"/>
+<p>
+When passed a value whose JSON encoding is close to the maximum value of type <code>int</code> in
+length, the computation of <code>size</code> will overflow, producing a negative value. When that
+negative value is passed to <code>make</code>, a runtime panic will occur.
+</p>
+<p>
+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 <code>encryptValue</code> that ensures the value is no
+larger than 64 MB, which fits comfortably within an <code>int</code> and avoids the overflow:
+</p>
+<sample src="AllocationSizeOverflowGood.go"/>
+</example>
+
+<references>
+<li>The Go Programming Language Specification: <a href="https://golang.org/ref/spec#Integer_overflow">Integer overflow</a>.</li>
+<li>The Go Programming Language Specification: <a href="https://golang.org/ref/spec#Making_slices_maps_and_channels">Making slices, maps and channels</a>.</li>
+</references>
+</qhelp>

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"

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

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

@@ -0,0 +1,41 @@
+/**
+ * Provides a taint-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 }
+  }
+}

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

@@ -0,0 +1,195 @@
+/**
+ * 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 data-flow node that is an operand to an operation that may overflow.
+   */
+  abstract class OverflowProneOperand extends DataFlow::Node { }
+
+  /**
+   * A data-flow node that represents the size argument of an allocation, such as the `n` in
+   * `make([]byte, n)`.
+   */
+  abstract class AllocationSize 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 {
+    AllocationSize allocsz;
+
+    DefaultSink() {
+      this instanceof OverflowProneOperand 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 and floating-point operations
+    e.getType() instanceof IntegerType
+  }
+
+  /** An operand of an arithmetic expression that could cause overflow. */
+  private class DefaultOverflowProneOperand extends OverflowProneOperand {
+    DefaultOverflowProneOperand() {
+      exists(OperatorExpr parent | isOverflowProne(parent) |
+        this.asExpr() = parent.getAnOperand() and
+        // only consider outermost operands to avoid double reporting
+        not exists(OperatorExpr grandparent | parent = grandparent.getAnOperand().stripParens() |
+          isOverflowProne(grandparent)
+        )
+      )
+    }
+  }
+
+  /**
+   * The first or second (non-type) argument to a call to `make`, considered as an allocation size.
+   */
+  private class DefaultAllocationSize extends AllocationSize {
+    DefaultAllocationSize() { this = Builtin::make().getACall().getArgument([0 .. 1]) }
+  }
+
+  /** 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
+    e.isConst()
+    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)
+  }
+}

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:26:27:38 | 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:26:27:38 | 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: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 |
+| 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 |

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

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

@@ -0,0 +1 @@
+Security/CWE-190/AllocationSizeOverflow.ql

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

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

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

@@ -0,0 +1,5 @@
+package main
+
+func encryptBuffer(buf []byte) ([]byte, error) {
+	return buf, nil
+}

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, 10, 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{}) {}

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
+}