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..c15047f --- /dev/null +++ b/go/src/security/UnboundedIORead/UnboundedIORead.ql @@ -0,0 +1,101 @@ +/** + * @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") + 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`. + */ +class HTTPRequestBodySource extends DataFlow::Node { + HTTPRequestBodySource() { + 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. + */ +class SizeLimitingCall extends DataFlow::CallNode { + SizeLimitingCall() { + this.getTarget().hasQualifiedName("net/http", "MaxBytesReader") + or + this.getTarget().hasQualifiedName("io", "LimitReader") + } +} + +/** + * 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 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 (io.LimitReader pattern), 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/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.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 | 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