Go: memory leakage queries

doc/QUERIES.md

@@ -44,9 +44,11 @@
 
 | Name | Description | Severity | Precision  |
 | ---  | ----------- | :----:   | :--------: |
+|[Deferred resource release in loop](../go/src/docs/security/DeferReleaseInLoop/DeferReleaseInLoop.md)|Deferring a resource release (Close, Rollback, etc.) inside a loop delays cleanup until the enclosing function returns, causing resource leaks across iterations such as file descriptor exhaustion or connection pool starvation.|warning|medium|
 |[Invalid file permission parameter](../go/src/docs/security/FilePermsFlaws/FilePermsFlaws.md)|Finds non-octal (e.g., `755` vs `0o755`) and unsupported (e.g., `04666`) literals used as a filesystem permission parameter (`FileMode`)|error|medium|
 |[Missing MinVersion in tls.Config](../go/src/docs/security/MissingMinVersionTLS/MissingMinVersionTLS.md)|Finds uses of tls.Config where MinVersion is not set and the project's minimum Go version (from go.mod) indicates insecure defaults: Go < 1.18 for clients or Go < 1.22 for servers. Does not mark explicitly set versions (including explicitly insecure ones).|error|medium|
 |[Trim functions misuse](../go/src/docs/security/TrimMisuse/TrimMisuse.md)|Finds calls to `string.{Trim,TrimLeft,TrimRight}` with the 2nd argument not being a cutset but a continuous substring to be trimmed|error|low|
+|[Unbounded read of request body](../go/src/docs/security/UnboundedIORead/UnboundedIORead.md)|Reading an HTTP request body with `io.ReadAll` or `ioutil.ReadAll` without a size limit allows a malicious client to exhaust server memory with an arbitrarily large request.|error|high|
 
 ### Java and Kotlin
 

go/src/ext/ResourceAcquisitions.model.yml

@@ -0,0 +1,72 @@
+# Models-as-Data definitions of functions that acquire a resource which must
+# later be released (Close, etc.). Consumed by DeferReleaseInLoop.ql via
+# `sourceNode(node, "tob-resource-acq")`.
+#
+# The `output` column selects which acquired value to mark:
+#   - "ReturnValue"    -> the first return value (the common `(resource, error)` shape).
+#   - "ReturnValue[n]" -> the n-th return value, for functions that return more
+#                         than one resource (e.g. `os.Pipe` returns a read and a
+#                         write `*os.File`, so both ends are modeled below).
+#
+# Row format (sourceModel):
+#   [package, type, subtypes, name, signature, ext, output, kind, provenance]
+extensions:
+  - addsTo:
+      pack: codeql/go-all
+      extensible: sourceModel
+    data:
+      # os
+      - ["os", "", false, "Open", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["os", "", false, "OpenFile", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["os", "", false, "Create", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["os", "", false, "CreateTemp", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["os", "", false, "NewFile", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # os.Pipe returns (r, w *File, err): model both the read end and write end.
+      - ["os", "", false, "Pipe", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["os", "", false, "Pipe", "", "", "ReturnValue[1]", "tob-resource-acq", "manual"]
+      # net (package-level)
+      - ["net", "", false, "Dial", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net", "", false, "DialTimeout", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net", "", false, "Listen", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net", "", false, "ListenPacket", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net", "", false, "FileConn", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net", "", false, "FileListener", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net", "", false, "FilePacketConn", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # net.Dialer (method)
+      - ["net", "Dialer", true, "Dial", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net", "Dialer", true, "DialContext", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # net/http (package-level)
+      - ["net/http", "", false, "Get", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net/http", "", false, "Post", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net/http", "", false, "PostForm", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net/http", "", false, "Head", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # net/http.Client (method)
+      - ["net/http", "Client", true, "Do", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net/http", "Client", true, "Get", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net/http", "Client", true, "Post", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net/http", "Client", true, "PostForm", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["net/http", "Client", true, "Head", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # crypto/tls (package-level)
+      - ["crypto/tls", "", false, "Dial", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["crypto/tls", "", false, "DialWithDialer", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["crypto/tls", "", false, "Client", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["crypto/tls", "", false, "Server", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # crypto/tls.Dialer (method)
+      - ["crypto/tls", "Dialer", true, "DialContext", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # compress/gzip
+      - ["compress/gzip", "", false, "NewReader", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["compress/gzip", "", false, "NewWriter", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["compress/gzip", "", false, "NewWriterLevel", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # compress/zlib
+      - ["compress/zlib", "", false, "NewReader", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["compress/zlib", "", false, "NewWriter", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["compress/zlib", "", false, "NewWriterLevel", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["compress/zlib", "", false, "NewWriterLevelDict", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # compress/flate
+      - ["compress/flate", "", false, "NewReader", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["compress/flate", "", false, "NewWriter", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # compress/lzw
+      - ["compress/lzw", "", false, "NewReader", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      - ["compress/lzw", "", false, "NewWriter", "", "", "ReturnValue", "tob-resource-acq", "manual"]
+      # archive/zip
+      - ["archive/zip", "", false, "OpenReader", "", "", "ReturnValue", "tob-resource-acq", "manual"]

go/src/qlpack.yml

@@ -2,11 +2,13 @@
 name: trailofbits/go-queries
 description: CodeQL queries for Go developed by Trail of Bits
 authors: Trail of Bits
-version: 0.3.0
+version: 0.3.1
 license: AGPL
 library: false
 extractor: go
 dependencies:
   codeql/go-all: "*"
+dataExtensions:
+  - ext/**/*.model.yml
 suites: codeql-suites
 defaultSuiteFile: codeql-suites/tob-go-code-scanning.qls

go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.go

@@ -0,0 +1,32 @@
+package main
+
+import (
+	"fmt"
+	"os"
+)
+
+// BAD: defer Close inside a loop leaks file descriptors
+func badReadFiles(paths []string) {
+	for _, path := range paths {
+		f, err := os.Open(path)
+		if err != nil {
+			continue
+		}
+		defer f.Close() // not closed until function returns
+		fmt.Println(f.Name())
+	}
+}
+
+// GOOD: extract into a function so defer runs per iteration
+func goodReadFiles(paths []string) {
+	for _, path := range paths {
+		func() {
+			f, err := os.Open(path)
+			if err != nil {
+				return
+			}
+			defer f.Close() // closed at end of this closure
+			fmt.Println(f.Name())
+		}()
+	}
+}

go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+In Go, <code>defer</code> schedules a function call to run when the <em>enclosing function</em>
+returns — not when the enclosing block or loop iteration ends. Deferring a resource release
+call (such as <code>Close</code>, <code>Unlock</code>, or <code>Rollback</code>) inside a loop
+means that cleanup calls accumulate and only execute after the loop finishes and the function
+returns.
+</p>
+
+<p>
+This can lead to resource exhaustion: file descriptors pile up, database connections are held
+open, locks are held longer than intended, or transactions remain open across iterations.
+</p>
+
+</overview>
+<recommendation>
+<p>
+Extract the loop body into a separate function or closure so that <code>defer</code> runs
+at the end of each iteration:
+</p>
+
+<sample src="DeferReleaseInLoop.go" />
+
+<p>
+Alternatively, call the cleanup function directly without <code>defer</code> at the
+appropriate point in the loop body.
+</p>
+
+</recommendation>
+<references>
+<li>
+    <a href="https://go.dev/ref/spec#Defer_statements">Go Language Specification — Defer statements</a>
+</li>
+<li>
+    <a href="https://blog.learngoprogramming.com/gotchas-of-defer-in-go-1-8d070894cb01">Gotchas of Defer in Go</a>
+</li>
+</references>
+</qhelp>

go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.ql

@@ -0,0 +1,68 @@
+/**
+ * @name Deferred resource release in loop
+ * @id tob/go/defer-release-in-loop
+ * @description Deferring a resource release (Close, Rollback, etc.) inside a loop delays cleanup until the enclosing function returns, causing resource leaks across iterations such as file descriptor exhaustion or connection pool starvation.
+ * @kind problem
+ * @tags security
+ * @problem.severity warning
+ * @precision medium
+ * @security-severity 3.0
+ * @group security
+ */
+
+import go
+import semmle.go.dataflow.ExternalFlow
+
+/**
+ * Holds if `inner` is a (transitive) child of `outer` without crossing
+ * a function literal boundary.
+ */
+predicate parentWithoutFuncLit(AstNode inner, AstNode outer) {
+  inner.getParent() = outer and not inner instanceof FuncLit
+  or
+  exists(AstNode mid |
+    parentWithoutFuncLit(inner, mid) and
+    parentWithoutFuncLit(mid, outer)
+  )
+}
+
+/** Holds if `node` is inside the body of `loop`, not crossing closures. */
+predicate inLoopBody(AstNode node, LoopStmt loop) {
+  parentWithoutFuncLit(node, loop.(ForStmt).getBody())
+  or
+  parentWithoutFuncLit(node, loop.(RangeStmt).getBody())
+}
+
+/**
+ * Gets the root identifier of an identifier or (possibly nested) selector
+ * expression: `f` for `f`, `resp` for `resp.Body`, `a` for `a.b.c`.
+ */
+Ident selectorRoot(Expr e) {
+  result = e
+  or
+  result = selectorRoot(e.(SelectorExpr).getBase())
+}
+
+/**
+ * Gets the data-flow node for the root variable of a `defer x.Close()`
+ * receiver chain. For `defer f.Close()`, this is `f`; for
+ * `defer resp.Body.Close()`, this is `resp`; for `defer a.b.c.Close()`,
+ * this is `a`.
+ */
+DataFlow::Node deferCloseReceiverBase(DeferStmt d) {
+  d.getCall().getTarget().getName() = "Close" and
+  result.asExpr() = selectorRoot(d.getCall().getCalleeExpr().(SelectorExpr).getBase())
+}
+
+from DeferStmt deferStmt, DataFlow::CallNode acquisition, DataFlow::Node acquired, LoopStmt loop
+where
+  // Match any return value modeled as a resource acquisition, not just the
+  // first — e.g. `os.Pipe` returns both a read and a write `*os.File`.
+  acquired = acquisition.getResult(_) and
+  sourceNode(acquired, "tob-resource-acq") and
+  inLoopBody(acquisition.asExpr(), loop) and
+  inLoopBody(deferStmt, loop) and
+  DataFlow::localFlow(acquired, deferCloseReceiverBase(deferStmt))
+select deferStmt,
+  "Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations.",
+  acquisition, acquisition.getTarget().getName() + "()"

go/src/security/UnboundedIORead/UnboundedIORead.go

@@ -0,0 +1,19 @@
+package main
+
+import (
+	"io"
+	"net/http"
+)
+
+// BAD: unbounded read of request body
+func badHandler(w http.ResponseWriter, r *http.Request) {
+	body, _ := io.ReadAll(r.Body) // no size limit — OOM on large request
+	w.Write(body)
+}
+
+// GOOD: limit body size before reading
+func goodHandler(w http.ResponseWriter, r *http.Request) {
+	r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1 MB limit
+	body, _ := io.ReadAll(r.Body)
+	w.Write(body)
+}

go/src/security/UnboundedIORead/UnboundedIORead.qhelp

@@ -0,0 +1,35 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Reading an HTTP request body with <code>io.ReadAll</code> (or the deprecated
+<code>ioutil.ReadAll</code>) allocates the entire body into memory with no upper bound.
+A malicious client can send an arbitrarily large request body to exhaust server memory,
+causing a denial-of-service condition.
+</p>
+
+</overview>
+<recommendation>
+<p>
+Wrap the request body with a size-limiting reader before reading it:
+</p>
+
+<sample src="UnboundedIORead.go" />
+
+<p>
+Prefer <code>http.MaxBytesReader</code> which also sets the appropriate error on the
+response, or <code>io.LimitReader</code> for non-HTTP contexts.
+</p>
+
+</recommendation>
+<references>
+<li>
+    <a href="https://pkg.go.dev/net/http#MaxBytesReader">http.MaxBytesReader documentation</a>
+</li>
+<li>
+    <a href="https://pkg.go.dev/io#LimitReader">io.LimitReader documentation</a>
+</li>
+</references>
+</qhelp>