From c8763174606a0df19b47c53f15cc0ae9abcff754 Mon Sep 17 00:00:00 2001 From: alan <17283637+ex0dus-0x@users.noreply.github.com> Date: Thu, 2 Apr 2026 16:56:03 -0400 Subject: [PATCH 1/3] New queries for Go memory leakage issues --- .../DeferReleaseInLoop/DeferReleaseInLoop.go | 32 ++++ .../DeferReleaseInLoop.qhelp | 42 +++++ .../DeferReleaseInLoop/DeferReleaseInLoop.ql | 48 ++++++ .../DeferReleaseInLoop/ResourceModel.qll | 71 ++++++++ .../UnboundedIORead/UnboundedIORead.go | 19 +++ .../UnboundedIORead/UnboundedIORead.qhelp | 35 ++++ .../UnboundedIORead/UnboundedIORead.ql | 85 +++++++++ .../ECDSASignatureMalleability.expected | 3 + .../ECDSASignatureMalleability.go | 78 +++++++++ .../ECDSASignatureMalleability.qlref | 1 + .../crypto/ECDSASignatureMalleability/go.mod | 3 + .../DeferReleaseInLoop.expected | 8 + .../DeferReleaseInLoop/DeferReleaseInLoop.go | 161 ++++++++++++++++++ .../DeferReleaseInLoop.qlref | 1 + .../security/DeferReleaseInLoop/go.mod | 3 + .../UnboundedIORead/UnboundedIORead.go | 61 +++++++ .../UnboundedIORead/UnboundedIORead.qlref | 1 + .../security/UnboundedIORead/go.mod | 3 + 18 files changed, 655 insertions(+) create mode 100644 go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.go create mode 100644 go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.qhelp create mode 100644 go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.ql create mode 100644 go/src/security/DeferReleaseInLoop/ResourceModel.qll create mode 100644 go/src/security/UnboundedIORead/UnboundedIORead.go create mode 100644 go/src/security/UnboundedIORead/UnboundedIORead.qhelp create mode 100644 go/src/security/UnboundedIORead/UnboundedIORead.ql create mode 100644 go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.expected create mode 100644 go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.go create mode 100644 go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.qlref create mode 100644 go/test/query-tests/crypto/ECDSASignatureMalleability/go.mod create mode 100644 go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.expected create mode 100644 go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.go create mode 100644 go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.qlref create mode 100644 go/test/query-tests/security/DeferReleaseInLoop/go.mod create mode 100644 go/test/query-tests/security/UnboundedIORead/UnboundedIORead.go create mode 100644 go/test/query-tests/security/UnboundedIORead/UnboundedIORead.qlref create mode 100644 go/test/query-tests/security/UnboundedIORead/go.mod diff --git a/go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.go b/go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.go new file mode 100644 index 0000000..68b32ac --- /dev/null +++ b/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()) + }() + } +} diff --git a/go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.qhelp b/go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.qhelp new file mode 100644 index 0000000..fd24c91 --- /dev/null +++ b/go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.qhelp @@ -0,0 +1,42 @@ + + + +

+In Go, defer schedules a function call to run when the enclosing function +returns — not when the enclosing block or loop iteration ends. Deferring a resource release +call (such as Close, Unlock, or Rollback) inside a loop +means that cleanup calls accumulate and only execute after the loop finishes and the function +returns. +

+ +

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

+ + + +

+Extract the loop body into a separate function or closure so that defer runs +at the end of each iteration: +

+ + + +

+Alternatively, call the cleanup function directly without defer at the +appropriate point in the loop body. +

+ + + +
  • + Go Language Specification — Defer statements +
    • +
  • + Gotchas of Defer in Go +
    • +     +     diff --git a/go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.ql b/go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.ql new file mode 100644 index 0000000..e0e9826 --- /dev/null +++ b/go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.ql @@ -0,0 +1,48 @@ +/** + * @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 ResourceModel + +/** + * Holds if `inner` is a (transitive) child of `outer` without crossing + * a function literal boundary. This ensures we don't catch defers inside + * closures that are invoked per-iteration. + */ +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) + ) +} + +/** + * A `defer` statement that defers a resource close/release call. + */ +class DeferredResourceClose extends DeferStmt { + ResourceCloseMethod closeMethod; + + DeferredResourceClose() { closeMethod = this.getCall().getTarget() } + + string getMethodName() { result = closeMethod.getName() } +} + +from DeferredResourceClose deferStmt, LoopStmt loop +where + parentWithoutFuncLit(deferStmt, loop.(ForStmt).getBody()) + or + parentWithoutFuncLit(deferStmt, loop.(RangeStmt).getBody()) +select deferStmt, + "Deferred " + deferStmt.getMethodName() + + "() in a loop will not execute until the function returns, leaking resources across iterations." diff --git a/go/src/security/DeferReleaseInLoop/ResourceModel.qll b/go/src/security/DeferReleaseInLoop/ResourceModel.qll new file mode 100644 index 0000000..8886f26 --- /dev/null +++ b/go/src/security/DeferReleaseInLoop/ResourceModel.qll @@ -0,0 +1,71 @@ +/** + * Models for file, network, and database resource acquisition and release in Go. + * + * These models identify: + * - "Source" functions that return closeable resources (files, connections, etc.) + * - "Sink" methods that release/close those resources + */ + +import go + +/** + * A function that opens or acquires a file-based resource. No common trait used + */ +class FileResourceAcquisition extends Function { + FileResourceAcquisition() { + // os package: file open/create + this.hasQualifiedName("os", ["Open", "OpenFile", "Create", "CreateTemp", "NewFile", "Pipe"]) + or + // net package: connections and listeners + this.hasQualifiedName("net", ["Dial", "DialTimeout", "Listen", "ListenPacket"]) + or + this.hasQualifiedName("net", ["FileConn", "FileListener", "FilePacketConn"]) + or + this.(Method).hasQualifiedName("net", "Dialer", ["Dial", "DialContext"]) + or + // net/http: client requests returning response bodies + this.hasQualifiedName("net/http", ["Get", "Post", "PostForm", "Head"]) + or + this.(Method).hasQualifiedName("net/http", "Client", ["Do", "Get", "Post", "PostForm", "Head"]) + or + // crypto/tls + this.hasQualifiedName("crypto/tls", ["Dial", "DialWithDialer", "Client", "Server"]) + or + this.(Method).hasQualifiedName("crypto/tls", "Dialer", "DialContext") + or + // compress readers/writers (implement io.Closer) + this.hasQualifiedName("compress/gzip", ["NewReader", "NewWriter", "NewWriterLevel"]) + or + this.hasQualifiedName("compress/zlib", + ["NewReader", "NewWriter", "NewWriterLevel", "NewWriterLevelDict"]) + or + this.hasQualifiedName("compress/flate", ["NewReader", "NewWriter"]) + or + this.hasQualifiedName("compress/lzw", ["NewReader", "NewWriter"]) + or + // archive readers + this.hasQualifiedName("archive/zip", "OpenReader") + } +} + +/** + * A method that closes or releases a file-based resource that shouldn't be leaked. + * Matching on those that implement `io.Closer` for now. + */ +class ResourceCloseMethod extends Method { + ResourceCloseMethod() { + this.getName() = "Close" and + ( + this.getReceiverType().implements("io", "Closer") + or + exists(Type base | + base = this.getReceiverType().(PointerType).getBaseType() + or + not this.getReceiverType() instanceof PointerType and + base = this.getReceiverType() + | + base.implements("io", "Closer") + ) + ) + } +} diff --git a/go/src/security/UnboundedIORead/UnboundedIORead.go b/go/src/security/UnboundedIORead/UnboundedIORead.go new file mode 100644 index 0000000..aeccad8 --- /dev/null +++ b/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) +} diff --git a/go/src/security/UnboundedIORead/UnboundedIORead.qhelp b/go/src/security/UnboundedIORead/UnboundedIORead.qhelp new file mode 100644 index 0000000..4278e7b --- /dev/null +++ b/go/src/security/UnboundedIORead/UnboundedIORead.qhelp @@ -0,0 +1,35 @@ + + + +

    +Reading an HTTP request body with io.ReadAll (or the deprecated +ioutil.ReadAll) 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. +

    + +     + +

    +Wrap the request body with a size-limiting reader before reading it: +

    + + + +

    +Prefer http.MaxBytesReader which also sets the appropriate error on the +response, or io.LimitReader for non-HTTP contexts. +

    + +     + +
  • + http.MaxBytesReader documentation +
    • +
  • + io.LimitReader documentation +
    • +     +     diff --git a/go/src/security/UnboundedIORead/UnboundedIORead.ql b/go/src/security/UnboundedIORead/UnboundedIORead.ql new file mode 100644 index 0000000..0f02a20 --- /dev/null +++ b/go/src/security/UnboundedIORead/UnboundedIORead.ql @@ -0,0 +1,85 @@ +/** + * @name Unbounded read of request body + * @id tob/go/unbounded-io-read + * @description 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. + * @kind path-problem + * @tags security + * @problem.severity error + * @precision high + * @security-severity 7.5 + * @group security + */ + +import go + +/** + * An `io.ReadAll` or `ioutil.ReadAll` call — functions that read an entire + * reader into memory with no size bound. + */ +class UnboundedReadCall extends DataFlow::CallNode { + UnboundedReadCall() { + this.getTarget().hasQualifiedName("io", "ReadAll") + or + this.getTarget().hasQualifiedName("io/ioutil", "ReadAll") // deprecated but still used + or + this.getTarget().hasQualifiedName("ioutil", "ReadAll") + } +} + +/** + * A source node: an HTTP request body that can be controlled by an attacker. + * + * Matches: + * - `r.Body` where `r` is an `*http.Request` + * - `r.GetBody()` calls + */ +class HTTPRequestBodySource extends DataFlow::Node { + HTTPRequestBodySource() { + // r.Body field read + exists(Field f, SelectorExpr sel | + f.hasQualifiedName("net/http", "Request", "Body") and + sel.getSelector().refersTo(f) and + this.asExpr() = sel + ) + } +} + +/** + * A call that wraps a reader with a size limit, acting as a sanitizer. + * + * - `http.MaxBytesReader(w, r, n)` + * - `io.LimitReader(r, n)` + * - `io.LimitedReader{R: r, N: n}` + */ +class SizeLimitingCall extends DataFlow::CallNode { + SizeLimitingCall() { + this.getTarget().hasQualifiedName("net/http", "MaxBytesReader") + or + this.getTarget().hasQualifiedName("io", "LimitReader") + } +} + +module UnboundedReadConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof HTTPRequestBodySource } + + predicate isSink(DataFlow::Node sink) { + exists(UnboundedReadCall readAll | sink = readAll.getArgument(0)) + } + + predicate isBarrier(DataFlow::Node node) { + // If the body passes through a size-limiting wrapper, it is safe + node = any(SizeLimitingCall c).getResult(0) + or + node = any(SizeLimitingCall c).getResult() + } +} + +module UnboundedReadFlow = DataFlow::Global; + +import UnboundedReadFlow::PathGraph + +from UnboundedReadFlow::PathNode source, UnboundedReadFlow::PathNode sink +where UnboundedReadFlow::flowPath(source, sink) +select sink.getNode(), source, sink, + "Unbounded read of HTTP request $@ with no size limit allows denial of service via memory exhaustion.", + source.getNode(), "body" diff --git a/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.expected b/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.expected new file mode 100644 index 0000000..d6fe915 --- /dev/null +++ b/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.expected @@ -0,0 +1,3 @@ +| ECDSASignatureMalleability.go:18:28:18:30 | sig | ECDSA signature used as hex encoding (likely identifier). Signatures are malleable: (r, s) and (r, -s mod n) are both valid, so they cannot serve as unique identifiers. | +| ECDSASignatureMalleability.go:34:21:34:23 | sig | ECDSA signature used as byte comparison. Signatures are malleable: (r, s) and (r, -s mod n) are both valid, so they cannot serve as unique identifiers. | +| ECDSASignatureMalleability.go:48:28:48:30 | sig | ECDSA signature used as hex encoding (likely identifier). Signatures are malleable: (r, s) and (r, -s mod n) are both valid, so they cannot serve as unique identifiers. | diff --git a/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.go b/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.go new file mode 100644 index 0000000..951931f --- /dev/null +++ b/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.go @@ -0,0 +1,78 @@ +package main + +import ( + "bytes" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/sha256" + "database/sql" + "encoding/hex" + "fmt" +) + +// finding: signature used as hex-encoded identifier +func test_sig_as_hex_id(key *ecdsa.PrivateKey, msg []byte) string { + hash := sha256.Sum256(msg) + sig, _ := ecdsa.SignASN1(rand.Reader, key, hash[:]) + return hex.EncodeToString(sig) +} + +// TODO: not yet detected — r.String() concatenation breaks dataflow from big.Int +func test_sig_as_map_key(key *ecdsa.PrivateKey, msg []byte) { + hash := sha256.Sum256(msg) + r, s, _ := ecdsa.Sign(rand.Reader, key, hash[:]) + seen := make(map[string]bool) + sigKey := r.String() + ":" + s.String() + seen[sigKey] = true +} + +// finding: signature compared with bytes.Equal (replay check) +func test_sig_bytes_equal(key *ecdsa.PrivateKey, msg []byte, prevSig []byte) bool { + hash := sha256.Sum256(msg) + sig, _ := ecdsa.SignASN1(rand.Reader, key, hash[:]) + return bytes.Equal(sig, prevSig) +} + +// TODO: not yet detected — variadic argument matching needs refinement +func test_sig_in_db(db *sql.DB, key *ecdsa.PrivateKey, msg []byte) { + hash := sha256.Sum256(msg) + sig, _ := ecdsa.SignASN1(rand.Reader, key, hash[:]) + db.Exec("INSERT INTO sigs (sig) VALUES (?)", sig) +} + +// finding: PrivateKey.Sign method result used as identifier +func test_method_sign_hex(key *ecdsa.PrivateKey, msg []byte) string { + hash := sha256.Sum256(msg) + sig, _ := key.Sign(rand.Reader, hash[:], nil) + return hex.EncodeToString(sig) +} + +/* + * False positives that should NOT be flagged + */ + +// ok: signature verified but not used as identifier +func test_fp_verify_only(key *ecdsa.PrivateKey, msg []byte) bool { + hash := sha256.Sum256(msg) + sig, _ := ecdsa.SignASN1(rand.Reader, key, hash[:]) + return ecdsa.VerifyASN1(&key.PublicKey, hash[:], sig) +} + +// ok: message hash used as identifier (not signature) +func test_fp_hash_as_id(msg []byte) string { + hash := sha256.Sum256(msg) + return hex.EncodeToString(hash[:]) +} + +func main() { + key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + msg := []byte("test message") + fmt.Println(test_sig_as_hex_id(key, msg)) + test_sig_as_map_key(key, msg) + fmt.Println(test_sig_bytes_equal(key, msg, nil)) + test_sig_in_db(nil, key, msg) + fmt.Println(test_method_sign_hex(key, msg)) + fmt.Println(test_fp_verify_only(key, msg)) + fmt.Println(test_fp_hash_as_id(msg)) +} diff --git a/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.qlref b/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.qlref new file mode 100644 index 0000000..58cc4b4 --- /dev/null +++ b/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.qlref @@ -0,0 +1 @@ +crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.ql diff --git a/go/test/query-tests/crypto/ECDSASignatureMalleability/go.mod b/go/test/query-tests/crypto/ECDSASignatureMalleability/go.mod new file mode 100644 index 0000000..e854f91 --- /dev/null +++ b/go/test/query-tests/crypto/ECDSASignatureMalleability/go.mod @@ -0,0 +1,3 @@ +module codeql-go-tests/query/ECDSASignatureMalleability + +go 1.18 diff --git a/go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.expected b/go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.expected new file mode 100644 index 0000000..5014c42 --- /dev/null +++ b/go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.expected @@ -0,0 +1,8 @@ +| DeferReleaseInLoop.go:19:3:19:17 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. | +| DeferReleaseInLoop.go:31:3:31:17 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. | +| DeferReleaseInLoop.go:43:3:43:25 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. | +| DeferReleaseInLoop.go:55:3:55:20 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. | +| DeferReleaseInLoop.go:67:3:67:18 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. | +| DeferReleaseInLoop.go:84:3:84:18 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. | +| DeferReleaseInLoop.go:85:3:85:17 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. | +| DeferReleaseInLoop.go:98:4:98:18 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. | diff --git a/go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.go b/go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.go new file mode 100644 index 0000000..fb3159f --- /dev/null +++ b/go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.go @@ -0,0 +1,161 @@ +package main + +import ( + "compress/gzip" + "fmt" + "net" + "net/http" + "os" + "sync" +) + +// finding: defer os.File.Close in range loop +func test_file_range_close(paths []string) { + for _, path := range paths { + f, err := os.Open(path) + if err != nil { + continue + } + defer f.Close() + fmt.Println(f.Name()) + } +} + +// finding: defer os.File.Close in for loop (via os.Create) +func test_file_for_close() { + for i := 0; i < 100; i++ { + f, err := os.Create(fmt.Sprintf("/tmp/file%d", i)) + if err != nil { + continue + } + defer f.Close() + fmt.Println(f.Name()) + } +} + +// finding: defer resp.Body.Close() in loop (io.ReadCloser) +func test_http_body(urls []string) { + for _, url := range urls { + resp, err := http.Get(url) + if err != nil { + continue + } + defer resp.Body.Close() + fmt.Println(resp.Status) + } +} + +// finding: defer net.Conn.Close in loop +func test_net_dial(addrs []string) { + for _, addr := range addrs { + conn, err := net.Dial("tcp", addr) + if err != nil { + continue + } + defer conn.Close() + fmt.Fprintln(conn, "hello") + } +} + +// finding: defer net.Listener.Close in loop +func test_net_listen(ports []string) { + for _, port := range ports { + ln, err := net.Listen("tcp", ":"+port) + if err != nil { + continue + } + defer ln.Close() + _ = ln + } +} + +// finding: defer gzip.Reader.Close in loop +func test_gzip_close(paths []string) { + for _, path := range paths { + f, err := os.Open(path) + if err != nil { + continue + } + gz, err := gzip.NewReader(f) + if err != nil { + f.Close() + continue + } + defer gz.Close() + defer f.Close() + _ = gz + } +} + +// finding: defer os.File.Close in nested loop +func test_nested_loop(paths []string) { + for i := 0; i < 3; i++ { + for _, path := range paths { + f, err := os.Open(path) + if err != nil { + continue + } + defer f.Close() + fmt.Println(f.Name()) + } + } +} + +/* + * False positives that should NOT be flagged + */ + +// ok: defer Close in closure inside loop — runs per iteration +func test_fp_closure(paths []string) { + for _, path := range paths { + func() { + f, err := os.Open(path) + if err != nil { + return + } + defer f.Close() + fmt.Println(f.Name()) + }() + } +} + +// ok: defer Close outside any loop +func test_fp_no_loop() { + f, err := os.Open("/tmp/test") + if err != nil { + return + } + defer f.Close() + fmt.Println(f.Name()) +} + +// ok: defer of non-resource call in loop (fmt.Println is not a release) +func test_fp_arbitrary_defer(items []string) { + for _, item := range items { + defer fmt.Println(item) + } +} + +// ok: defer Unlock in loop (sync.Mutex is not file/IO/socket) +func test_fp_mutex(items []string) { + var mu sync.Mutex + for _, item := range items { + mu.Lock() + defer mu.Unlock() + fmt.Println(item) + } +} + +func main() { + test_file_range_close(os.Args[1:]) + test_file_for_close() + test_http_body(os.Args[1:]) + test_net_dial(os.Args[1:]) + test_net_listen(os.Args[1:]) + test_gzip_close(os.Args[1:]) + test_nested_loop(os.Args[1:]) + test_fp_closure(os.Args[1:]) + test_fp_no_loop() + test_fp_arbitrary_defer(os.Args[1:]) + test_fp_mutex(os.Args[1:]) +} diff --git a/go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.qlref b/go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.qlref new file mode 100644 index 0000000..4dab5be --- /dev/null +++ b/go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.qlref @@ -0,0 +1 @@ +security/DeferReleaseInLoop/DeferReleaseInLoop.ql diff --git a/go/test/query-tests/security/DeferReleaseInLoop/go.mod b/go/test/query-tests/security/DeferReleaseInLoop/go.mod new file mode 100644 index 0000000..ce16a2c --- /dev/null +++ b/go/test/query-tests/security/DeferReleaseInLoop/go.mod @@ -0,0 +1,3 @@ +module codeql-go-tests/query/DeferReleaseInLoop + +go 1.18 diff --git a/go/test/query-tests/security/UnboundedIORead/UnboundedIORead.go b/go/test/query-tests/security/UnboundedIORead/UnboundedIORead.go new file mode 100644 index 0000000..83450cd --- /dev/null +++ b/go/test/query-tests/security/UnboundedIORead/UnboundedIORead.go @@ -0,0 +1,61 @@ +package main + +import ( + "fmt" + "io" + "io/ioutil" + "net/http" +) + +// finding: io.ReadAll on raw request body +func test_readall_raw(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + w.Write(body) +} + +// finding: ioutil.ReadAll on raw request body (deprecated but common) +func test_ioutil_readall_raw(w http.ResponseWriter, r *http.Request) { + body, _ := ioutil.ReadAll(r.Body) + w.Write(body) +} + +// finding: body passed through variable +func test_readall_via_var(w http.ResponseWriter, r *http.Request) { + reader := r.Body + body, _ := io.ReadAll(reader) + w.Write(body) +} + +/* + * False positives that should NOT be flagged + */ + +// ok: body wrapped with MaxBytesReader +func test_fp_maxbytes(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, 1<<20) + body, _ := io.ReadAll(r.Body) + w.Write(body) +} + +// ok: body wrapped with LimitReader +func test_fp_limitreader(w http.ResponseWriter, r *http.Request) { + limited := io.LimitReader(r.Body, 1<<20) + body, _ := io.ReadAll(limited) + w.Write(body) +} + +// ok: reading from a non-HTTP source +func test_fp_non_http() { + resp, _ := http.Get("http://example.com") + body, _ := io.ReadAll(resp.Body) + fmt.Println(string(body)) +} + +func main() { + http.HandleFunc("/raw", test_readall_raw) + http.HandleFunc("/ioutil", test_ioutil_readall_raw) + http.HandleFunc("/var", test_readall_via_var) + http.HandleFunc("/maxbytes", test_fp_maxbytes) + http.HandleFunc("/limit", test_fp_limitreader) + test_fp_non_http() +} diff --git a/go/test/query-tests/security/UnboundedIORead/UnboundedIORead.qlref b/go/test/query-tests/security/UnboundedIORead/UnboundedIORead.qlref new file mode 100644 index 0000000..4c47823 --- /dev/null +++ b/go/test/query-tests/security/UnboundedIORead/UnboundedIORead.qlref @@ -0,0 +1 @@ +security/UnboundedIORead/UnboundedIORead.ql diff --git a/go/test/query-tests/security/UnboundedIORead/go.mod b/go/test/query-tests/security/UnboundedIORead/go.mod new file mode 100644 index 0000000..ecb8819 --- /dev/null +++ b/go/test/query-tests/security/UnboundedIORead/go.mod @@ -0,0 +1,3 @@ +module codeql-go-tests/query/UnboundedIORead + +go 1.18 From 0bb1b873f36e17edf9d9cc8a59958a115e61a548 Mon Sep 17 00:00:00 2001 From: alan <17283637+ex0dus-0x@users.noreply.github.com> Date: Fri, 3 Apr 2026 13:43:48 -0400 Subject: [PATCH 2/3] Remove unnecessary test --- .../ECDSASignatureMalleability.expected | 3 - .../ECDSASignatureMalleability.go | 78 ------------------- .../ECDSASignatureMalleability.qlref | 1 - .../crypto/ECDSASignatureMalleability/go.mod | 3 - 4 files changed, 85 deletions(-) delete mode 100644 go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.expected delete mode 100644 go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.go delete mode 100644 go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.qlref delete mode 100644 go/test/query-tests/crypto/ECDSASignatureMalleability/go.mod diff --git a/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.expected b/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.expected deleted file mode 100644 index d6fe915..0000000 --- a/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.expected +++ /dev/null @@ -1,3 +0,0 @@ -| ECDSASignatureMalleability.go:18:28:18:30 | sig | ECDSA signature used as hex encoding (likely identifier). Signatures are malleable: (r, s) and (r, -s mod n) are both valid, so they cannot serve as unique identifiers. | -| ECDSASignatureMalleability.go:34:21:34:23 | sig | ECDSA signature used as byte comparison. Signatures are malleable: (r, s) and (r, -s mod n) are both valid, so they cannot serve as unique identifiers. | -| ECDSASignatureMalleability.go:48:28:48:30 | sig | ECDSA signature used as hex encoding (likely identifier). Signatures are malleable: (r, s) and (r, -s mod n) are both valid, so they cannot serve as unique identifiers. | diff --git a/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.go b/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.go deleted file mode 100644 index 951931f..0000000 --- a/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.go +++ /dev/null @@ -1,78 +0,0 @@ -package main - -import ( - "bytes" - "crypto/ecdsa" - "crypto/elliptic" - "crypto/rand" - "crypto/sha256" - "database/sql" - "encoding/hex" - "fmt" -) - -// finding: signature used as hex-encoded identifier -func test_sig_as_hex_id(key *ecdsa.PrivateKey, msg []byte) string { - hash := sha256.Sum256(msg) - sig, _ := ecdsa.SignASN1(rand.Reader, key, hash[:]) - return hex.EncodeToString(sig) -} - -// TODO: not yet detected — r.String() concatenation breaks dataflow from big.Int -func test_sig_as_map_key(key *ecdsa.PrivateKey, msg []byte) { - hash := sha256.Sum256(msg) - r, s, _ := ecdsa.Sign(rand.Reader, key, hash[:]) - seen := make(map[string]bool) - sigKey := r.String() + ":" + s.String() - seen[sigKey] = true -} - -// finding: signature compared with bytes.Equal (replay check) -func test_sig_bytes_equal(key *ecdsa.PrivateKey, msg []byte, prevSig []byte) bool { - hash := sha256.Sum256(msg) - sig, _ := ecdsa.SignASN1(rand.Reader, key, hash[:]) - return bytes.Equal(sig, prevSig) -} - -// TODO: not yet detected — variadic argument matching needs refinement -func test_sig_in_db(db *sql.DB, key *ecdsa.PrivateKey, msg []byte) { - hash := sha256.Sum256(msg) - sig, _ := ecdsa.SignASN1(rand.Reader, key, hash[:]) - db.Exec("INSERT INTO sigs (sig) VALUES (?)", sig) -} - -// finding: PrivateKey.Sign method result used as identifier -func test_method_sign_hex(key *ecdsa.PrivateKey, msg []byte) string { - hash := sha256.Sum256(msg) - sig, _ := key.Sign(rand.Reader, hash[:], nil) - return hex.EncodeToString(sig) -} - -/* - * False positives that should NOT be flagged - */ - -// ok: signature verified but not used as identifier -func test_fp_verify_only(key *ecdsa.PrivateKey, msg []byte) bool { - hash := sha256.Sum256(msg) - sig, _ := ecdsa.SignASN1(rand.Reader, key, hash[:]) - return ecdsa.VerifyASN1(&key.PublicKey, hash[:], sig) -} - -// ok: message hash used as identifier (not signature) -func test_fp_hash_as_id(msg []byte) string { - hash := sha256.Sum256(msg) - return hex.EncodeToString(hash[:]) -} - -func main() { - key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) - msg := []byte("test message") - fmt.Println(test_sig_as_hex_id(key, msg)) - test_sig_as_map_key(key, msg) - fmt.Println(test_sig_bytes_equal(key, msg, nil)) - test_sig_in_db(nil, key, msg) - fmt.Println(test_method_sign_hex(key, msg)) - fmt.Println(test_fp_verify_only(key, msg)) - fmt.Println(test_fp_hash_as_id(msg)) -} diff --git a/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.qlref b/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.qlref deleted file mode 100644 index 58cc4b4..0000000 --- a/go/test/query-tests/crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.qlref +++ /dev/null @@ -1 +0,0 @@ -crypto/ECDSASignatureMalleability/ECDSASignatureMalleability.ql diff --git a/go/test/query-tests/crypto/ECDSASignatureMalleability/go.mod b/go/test/query-tests/crypto/ECDSASignatureMalleability/go.mod deleted file mode 100644 index e854f91..0000000 --- a/go/test/query-tests/crypto/ECDSASignatureMalleability/go.mod +++ /dev/null @@ -1,3 +0,0 @@ -module codeql-go-tests/query/ECDSASignatureMalleability - -go 1.18 From 80faff698e0545146aa3fceab53b6e8c1a2f6608 Mon Sep 17 00:00:00 2001 From: alan <17283637+ex0dus-0x@users.noreply.github.com> Date: Fri, 3 Apr 2026 14:15:20 -0400 Subject: [PATCH 3/3] add better barrier, and fix up test --- .../UnboundedIORead/UnboundedIORead.ql | 38 +++++++++++++------ .../UnboundedIORead/UnboundedIORead.expected | 12 ++++++ 2 files changed, 39 insertions(+), 11 deletions(-) create mode 100644 go/test/query-tests/security/UnboundedIORead/UnboundedIORead.expected diff --git a/go/src/security/UnboundedIORead/UnboundedIORead.ql b/go/src/security/UnboundedIORead/UnboundedIORead.ql index 0f02a20..c15047f 100644 --- a/go/src/security/UnboundedIORead/UnboundedIORead.ql +++ b/go/src/security/UnboundedIORead/UnboundedIORead.ql @@ -20,7 +20,7 @@ class UnboundedReadCall extends DataFlow::CallNode { UnboundedReadCall() { this.getTarget().hasQualifiedName("io", "ReadAll") or - this.getTarget().hasQualifiedName("io/ioutil", "ReadAll") // deprecated but still used + this.getTarget().hasQualifiedName("io/ioutil", "ReadAll") or this.getTarget().hasQualifiedName("ioutil", "ReadAll") } @@ -29,13 +29,10 @@ class UnboundedReadCall extends DataFlow::CallNode { /** * A source node: an HTTP request body that can be controlled by an attacker. * - * Matches: - * - `r.Body` where `r` is an `*http.Request` - * - `r.GetBody()` calls + * Matches `r.Body` where `r` is an `*http.Request`. */ class HTTPRequestBodySource extends DataFlow::Node { HTTPRequestBodySource() { - // r.Body field read exists(Field f, SelectorExpr sel | f.hasQualifiedName("net/http", "Request", "Body") and sel.getSelector().refersTo(f) and @@ -46,10 +43,6 @@ class HTTPRequestBodySource extends DataFlow::Node { /** * A call that wraps a reader with a size limit, acting as a sanitizer. - * - * - `http.MaxBytesReader(w, r, n)` - * - `io.LimitReader(r, n)` - * - `io.LimitedReader{R: r, N: n}` */ class SizeLimitingCall extends DataFlow::CallNode { SizeLimitingCall() { @@ -59,15 +52,38 @@ class SizeLimitingCall extends DataFlow::CallNode { } } +/** + * Holds if `r.Body` is reassigned from a size-limiting call in the same function + * that `bodyRead` is in, on the same request variable `r`. + */ +predicate bodyLimitedByReassignment(SelectorExpr bodyRead) { + exists(SizeLimitingCall limiter, Assignment assign, SelectorExpr lhs, Variable v | + // The assignment target is `v.Body` + assign.getLhs() = lhs and + lhs.getSelector().getName() = "Body" and + lhs.getBase().(Ident).refersTo(v) and + // The RHS is a MaxBytesReader/LimitReader call + assign.getRhs() = limiter.asExpr() and + // The body read is on the same variable: `v.Body` + bodyRead.getBase().(Ident).refersTo(v) and + // Both in the same function + assign.getEnclosingFunction() = bodyRead.getEnclosingFunction() + ) +} + module UnboundedReadConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { source instanceof HTTPRequestBodySource } + predicate isSource(DataFlow::Node source) { + source instanceof HTTPRequestBodySource and + // Exclude r.Body reads where r.Body was reassigned from a size limiter + not bodyLimitedByReassignment(source.asExpr()) + } predicate isSink(DataFlow::Node sink) { exists(UnboundedReadCall readAll | sink = readAll.getArgument(0)) } predicate isBarrier(DataFlow::Node node) { - // If the body passes through a size-limiting wrapper, it is safe + // If the body passes through a size-limiting wrapper (io.LimitReader pattern), it is safe node = any(SizeLimitingCall c).getResult(0) or node = any(SizeLimitingCall c).getResult() diff --git a/go/test/query-tests/security/UnboundedIORead/UnboundedIORead.expected b/go/test/query-tests/security/UnboundedIORead/UnboundedIORead.expected new file mode 100644 index 0000000..27a0ed4 --- /dev/null +++ b/go/test/query-tests/security/UnboundedIORead/UnboundedIORead.expected @@ -0,0 +1,12 @@ +edges +| UnboundedIORead.go:24:12:24:17 | selection of Body | UnboundedIORead.go:25:24:25:29 | reader | provenance | Src:MaD:1919 | +nodes +| UnboundedIORead.go:12:24:12:29 | selection of Body | semmle.label | selection of Body | +| UnboundedIORead.go:18:28:18:33 | selection of Body | semmle.label | selection of Body | +| UnboundedIORead.go:24:12:24:17 | selection of Body | semmle.label | selection of Body | +| UnboundedIORead.go:25:24:25:29 | reader | semmle.label | reader | +subpaths +#select +| UnboundedIORead.go:12:24:12:29 | selection of Body | UnboundedIORead.go:12:24:12:29 | selection of Body | UnboundedIORead.go:12:24:12:29 | selection of Body | Unbounded read of HTTP request $@ with no size limit allows denial of service via memory exhaustion. | UnboundedIORead.go:12:24:12:29 | selection of Body | body | +| UnboundedIORead.go:18:28:18:33 | selection of Body | UnboundedIORead.go:18:28:18:33 | selection of Body | UnboundedIORead.go:18:28:18:33 | selection of Body | Unbounded read of HTTP request $@ with no size limit allows denial of service via memory exhaustion. | UnboundedIORead.go:18:28:18:33 | selection of Body | body | +| UnboundedIORead.go:25:24:25:29 | reader | UnboundedIORead.go:24:12:24:17 | selection of Body | UnboundedIORead.go:25:24:25:29 | reader | Unbounded read of HTTP request $@ with no size limit allows denial of service via memory exhaustion. | UnboundedIORead.go:24:12:24:17 | selection of Body | body |