Unbounded per-IP counter map growth from retained zero-count entries

[
  {
    "finding_title": "Unbounded per-IP counter map growth from retained zero-count entries",
    "verdict": "INVALID",
    "reasoning": "The report claims that `perIPConnCounter`'s map `m` grows unbounded because `Unregister` clamps the count to zero but never deletes the entry, and that an attacker can trigger this via the public API. However, the provided code for `peripconn.go` consists entirely of unexported (lowercase) types and functions: `perIPConnCounter`, `Register`, `Unregister`, `perIPConn`, `perIPTLSConn`, `acquirePerIPConn`, `getUint32IP`, etc. In Go, unexported identifiers are not part of the public API and cannot be accessed by external callers or other packages. The threat model for a library explicitly states that internal helpers are only reachable through the public surface and that missing validation in non-public functions is not a vulnerability unless directly reachable from outside. Since there is no public exported function in this file that exposes the `perIPConnCounter` map to external input, and the code is intended for internal use (likely by `client.go` within the same package), an external attacker cannot directly instantiate this counter or manipulate the map growth. The 'attacker' described (remote IPv4 addresses) influences the *value* passed to `Register` (via `getUint32IP`), but the mechanism for map growth (retaining zero-count entries) is an internal implementation detail. Even if an attacker could cause many unique IPs to be registered, the memory growth is bounded by the number of unique IPv4 addresses seen, which is a potential resource exhaustion issue only if the public API (not shown here, e.g., `Client`) allows an attacker to force dialing to arbitrary many distinct IPs without bound. However, standard HTTP clients limit connections or use connection pooling that prevents infinite unique IP churn from a single remote endpoint in a way that bypasses higher-level limits. More critically, the specific bug reported—retaining map entries—is a code quality/efficiency issue in an internal component, not a direct security vulnerability reachable by an external attacker through the public API surface defined in the threat model. The public API would need to expose this counter or allow unbounded unique IP creation to make this a security issue, but as an internal helper, it's not directly exploitable in the manner described. The absence of a public entry point for this specific component makes the finding INVALID under the library threat model.\n\nCRUX: The perIPConnCounter type and its Register/Unregister methods are unexported (lowercase) in Go, meaning they are not part of the public API and cannot be directly accessed or triggered by external attackers as described.\nGREP: func (cc \\*perIPConnCounter)",
    "elapsed": 37.0,
    "tokens": 3332,
    "tokens_in": 2765,
    "tokens_out": 567,
    "cost_usd": 0.0028623,
    "file": "peripconn.go",
    "round": 1,
    "grep_used": true,
    "grep_results": "GREP `func (cc *perIPConnCounter) (simplified to: perIPConnCounter)`:\n```\nperipconn_test.go:28:\tvar cc perIPConnCounter\nperipconn.go:10:type perIPConnCounter struct {\nperipconn.go:17:func (cc *perIPConnCounter) Register(ip uint32) int {\nperipconn.go:28:func (cc *perIPConnCounter) Unregister(ip uint32) {\nperipconn.go:33:\t\tpanic(\"BUG: perIPConnCounter.Register() wasn't called\")\nperipconn.go:42:\tperIPConnCounter *perIPConnCounter\nperipconn.go:51:\tperIPConnCounter *perIPConnCounter\nperipconn.go:57:func acquirePerIPConn(conn net.Conn, ip uint32, counter *perIPConnCounter) net.Conn {\nperipconn.go:62:\t\t\t\tperIPConnCounter: counter,\nperipconn.go:76:\t\t\tperIPConnCounter: counter,\nperipconn.go:98:\tc.perIPConnCounter.Unregister(c.ip)\nperipconn.go:99:\tc.perIPConnCounter.perIPConnPool.Put(c)\nperipconn.go:114:\tc.perIPConnCounter.Unregister(c.ip)\nperipconn.go:115:\tc.perIPConnCounter.perIPTLSConnPool.Put(c)\nserver.go:152:\tperIPConnCounter perIPConnCounter\nserver.go:2110:\tn := s.perIPConnCounter.Register(ip)\nserver.go:2112:\t\ts.perIPConnCounter.Unregister(ip)\nserver.go:2117:\treturn acquirePerIPConn(c, ip, &s.perIPConnCounter)\n```"
  },
  {
    "finding_title": "Unbounded per-IP counter map growth from retained zero-count entries",
    "verdict": "VALID",
    "reasoning": "Reviewer 1's reasoning is fundamentally flawed because it ignores that the threat model for a library includes the data passed through its public API. The `perIPConnCounter` is embedded in `fasthttp.Server` (an exported/public struct) in `server.go`. The counter is used by the server to track connections from remote clients. The `Register` and `Unregister` methods are called from `server.go` (specifically around line 2110-2112 and 2117), which is part of the request handling path. \n\nThe bug pattern is real: `Unregister` decrements the count but never deletes the key from the map `m` when the count reaches zero. \n\nCan an attacker trigger it? The `ip` is derived from `getConnIP4(c)`, which reads `c.RemoteAddr()`. In a server context, `c.RemoteAddr()` is the address of the remote peer. An attacker can connect from many different source IPs (or use a proxy protocol that exposes many client IPs, or simply rotate source IPs). Each new IP causes a new entry in `m`. When connections close, `Unregister` is called, but the entry remains with value 0. Since IPv4 addresses are 32-bit, there are ~4 billion possible keys. If an attacker (or a DDoS source) uses a large number of unique source IPs, the map `m` will grow to accommodate all of them, leading to unbounded memory growth (well, bounded by 2^32, but that's effectively unbounded for practical DoS purposes) and potential denial of service. \n\nThe threat model explicitly states: \"resource bounds (unbounded allocation ... on crafted input)\" are vulnerabilities. It also states that public APIs (like `fasthttp.Server.Serve`) must handle adversarial payloads. Since the map growth is a direct result of processing incoming connections (untrusted input: source IP), and it leads to resource exhaustion, this is a valid security issue. The fact that the counter is internal does not protect it if it is reachable via the public `Server` API. The lack of deletion in `Unregister` is a missing defense against this resource exhaustion.\n\nCRUX: Whether the map `m` in `perIPConnCounter` is reachable via untrusted input (source IPs) through the public `Server` API and causes unbounded memory growth.\nGREP: func (s \\*Server) handle\\|perIPConnCounter.Register",
    "elapsed": 142.2,
    "tokens": 4410,
    "tokens_in": 3872,
    "tokens_out": 538,
    "cost_usd": 0.00324024,
    "file": "peripconn.go",
    "round": 2,
    "grep_used": true,
    "grep_results": "GREP `func (s *Server) handle\\ (simplified to: Server)`:\n```\nheader_timing_test.go:44:\t\t\t\t\t\"Server: aaa 1/2.3\\r\\n\" +\nheader_timing_test.go:100:\t\t\t\t\t\"Server: aaa 1/2.3\\r\\n\" +\nheader_timing_test.go:144:\t\th.SetServer(\"aaa 1/2.3\")\nheader_timing_test.go:213:\tsrc := []byte(\"User-Agent-Host-Content-Type-Content-Length-Server\")\nfasthttpadaptor/adaptor.go:58:\t\t\tctx.Error(\"Internal Server Error\", fasthttp.StatusInternalServerError)\nfasthttpadaptor/request.go:14:// forServer should be set to true when the http.Request is going to be passed to a http.Handler.\nfasthttpadaptor/request.go:18:func ConvertRequest(ctx *fasthttp.RequestCtx, r *http.Request, forServer bool) error {\nfasthttpadaptor/request.go:42:\tif forServer {\nfasthttpadaptor/adaptor_test.go:233:\ts := &fasthttp.Server{\nfasthttpadaptor/adaptor_test.go:302:\ts := &fasthttp.Server{\nfasthttpadaptor/adaptor_test.go:385:\ts := &fasthttp.Server{\nfasthttpadaptor/adaptor_test.go:473:\ts := &fasthttp.Server{\nfasthttpadaptor/adaptor_test.go:525:\ts := &fasthttp.Server{\nfasthttpadaptor/adaptor_test.go:582:\t\t\t\t\thttp.Error(w, \"Flusher not supported\", http.StatusInternalServerError)\nfasthttpadaptor/adaptor_test.go:602:\t\t\t\t\thttp.Error(w, \"Hijacker not supported\", http.StatusInternalServerError)\nfasthttpadaptor/adaptor_test.go:609:\ts := &fasthttp.Server{\nREADME.md:21:[Server Benchmarks](#http-server-performance-comparison-with-nethttp)\nREADME.md:53:$ GOMAXPROCS=1 go test -bench=NetHTTPServerGet -benchmem -benchtime=10s\nREADME.md:55:BenchmarkNetHTTPServerGet1ReqPerConn                      722565             15327 ns/op            3258 B/op         36 allocs/op\nREADME.md:56:BenchmarkNetHTTPServerGet2ReqPerConn                      990067             11533 ns/op            2817 B/op         28 allocs/op\nREADME.md:57:BenchmarkNetHTTPServerGet10ReqPerConn                    1376821              8734 ns/op            2483 B/op         23 allocs/op\nREADME.md:58:BenchmarkNetHTTPServerGet10KReqPerConn                   1691265              7151 ns/op            2385 B/op         21 allocs/op\nREADME.md:59:BenchmarkNetHTTPServerGet1ReqPerConn10KClients            643940             17152 ns/op            3529 B/op         36 allocs/op\nREADME.md:60:BenchmarkNetHTTPServerGet2ReqPerConn10KClients            868576             14010 ns/op            2826 B/op         28 allocs/op\nREADME.md:61:BenchmarkNetHTTPServerGet10ReqPerConn10KClients          1297398              9329 ns/op            2611 B/op         23 allocs/op\nREADME.md:62:BenchmarkNetHTTPServerGet100ReqPerConn10KClients         1467963              7902 ns/op            2450 B/op         21 allocs/op\nREADME.md:68:$ GOMAXPROCS=1 go test -bench=kServerGet -benchmem -benchtime=10s\nREADME.md:70:BenchmarkServerGet1ReqPerConn                    4304683              2733 ns/op               0 B/op          0 allocs/op\nREADME.md:71:BenchmarkServerGet2ReqPerConn                    5685157              2140 ns/op               0 B/op          0 allocs/op\nREADME.md:72:BenchmarkServerGet10ReqPerConn                   7659729              1550 ns/op               0 B/op          0 allocs/op\n```\n\nGREP `perIPConnCounter.Register`:\n```\nperipconn.go:33:\t\tpanic(\"BUG: perIPConnCounter.Register() wasn't called\")\nserver.go:2110:\tn := s.perIPConnCounter.Register(ip)\n```"
  },
  {
    "finding_title": "Unbounded per-IP counter map growth from retained zero-count entries",
    "verdict": "VALID",
    "reasoning": "The reported bug is that `perIPConnCounter.m` grows unbounded because `Unregister` does not delete entries with zero count. I verified the code in `peripconn.go`: `Unregister` indeed clamps to 0 but does not remove the key from the map. \n\nHowever, I must verify if this code path is reachable and if it applies to the Server or Client. The context states: 'It sits in the client-side connection lifecycle... to track how many connections have been issued per remote IPv4 address.' This suggests it tracks outbound connections initiated by the `fasthttp.Client`, not inbound connections to a `fasthttp.Server`. \n\nLet's check the caller. `server.go:2110` calls `s.perIPConnCounter.Register(ip)`. This implies the Server *does* use this counter. Let's look closer at `server.go`. If the Server uses this counter to limit inbound connections per IP, then an attacker can indeed create many unique source IPs. \n\nCrucially, is the map growth 'unbounded' in a way that causes DoS? The number of unique IPv4 addresses is $2^{32} \\approx 4.2 \\times 10^9$. A map entry in Go for a `uint32` key and `int` value takes roughly 32 bytes (key+value) + overhead. In Go's map implementation, the memory overhead per entry is significant, perhaps ~50-100 bytes. \n$4 \\times 10^9$ entries * ~100 bytes = ~400 GB. This is certainly unbounded for any practical server process (which typically runs in GBs of RAM). \n\nCan an attacker trigger this? An attacker must connect from many different source IPs. In a DDoS scenario or via a botnet, this is feasible. Even without a botnet, if the server is behind a load balancer or NAT that preserves client IPs, or if the attacker uses a range of IPs they control, they can populate the map. \n\nIs there a defense? The threat model says 'resource bounds (unbounded allocation ... on crafted input)' are vulnerabilities. The input is the source IP address. The map retains entries for every unique IP ever seen. There is no eviction mechanism. \n\nReviewer 2 argued this is VALID because it's reachable via the public Server API. Reviewer 1 argued INVALID because it's internal. The code is internal, but the *effect* is reachable via the public API (`Server.Serve`). The map is a component of the `Server` struct. \n\nHowever, let's look at the specific usage in `server.go`. Why does the server need a per-IP connection counter? Usually, servers have `MaxConnsPerIP` or similar. If `MaxConnsPerIP` is set, `Register` might reject new connections when the limit is reached. But `Unregister` still leaves the key. So even if connections are rejected, the key stays. If an attacker connects and gets rejected (or accepts), the key is added. \n\nWait, if `MaxConnsPerIP` is NOT set (default 0, which usually means unlimited or a high default), does `Register` still add to the map? Yes, `Register` always adds. So even without a limit, the map grows. \n\nIs this a security vulnerability? It allows a remote attacker to exhaust memory by sending connections from many unique IPs. This is a classic resource exhaustion DoS. The fact that it requires many unique IPs doesn't make it invalid; it just defines the threshold. Since the memory usage scales linearly with unique IPs, and there is no bound on unique IPs (except the IP space size, which is too large for memory), it is a valid DoS vector. \n\nOne counter-argument: The number of concurrent connections is usually limited by OS file descriptors or `ListenBacklog`. However, the map entries persist *after* the connection closes. So the attacker can open/close connections serially from different IPs to grow the map without holding many open connections simultaneously. This makes it much easier to trigger than a standard connection exhaustion attack. \n\nTherefore, the bug is real, reachable via public API, and causes unbounded resource consumption (memory) by an untrusted actor. This fits the definition of a Valid vulnerability under the threat model (resource exhaustion/DoS).\n\nCRUX: The map `m` in `perIPConnCounter` retains entries for every unique source IP ever seen, even after the connection closes, allowing an attacker to exhaust server memory by rotating through a large number of unique source IPs.\nGREP: s.perIPConnCounter.Register",
    "elapsed": 84.8,
    "tokens": 6163,
    "tokens_in": 5154,
    "tokens_out": 1009,
    "cost_usd": 0.005191680000000001,
    "file": "peripconn.go",
    "round": 3,
    "grep_used": true,
    "grep_results": "GREP `s.perIPConnCounter.Register`:\n```\nserver.go:2110:\tn := s.perIPConnCounter.Register(ip)\n```"
  },
  {
    "finding_title": "Unbounded per-IP counter map growth from retained zero-count entries",
    "verdict": "VALID",
    "reasoning": "The report claims that the `perIPConnCounter` map grows unbounded because `Unregister` does not delete entries with zero count, allowing a remote attacker to cause memory exhaustion via many unique source IPs. I must verify if this code path is reachable from the public API with untrusted input that an attacker can control. \n\n1. **Code Analysis**: The `Unregister` function in `peripconn.go` indeed decrements the count and clamps it to 0 (`n := max(cc.m[ip]-1, 0)`) but does not remove the key from the map `cc.m`. This confirms the \"retained zero-count entry\" bug pattern exists.\n\n2. **Reachability & Trust Boundary**: The threat model defines the attacker as code calling the public API and the data it passes through. The entry point is the public API of the library. The context states this code is in `peripconn.go`. I need to determine if this is used by the `Server` (inbound, attacker-controlled source IP) or `Client` (outbound, developer-controlled destination IP). \n   - Reviewer 2 and 3 argued it is used in `server.go` to track inbound connections from remote clients. If true, the source IP is attacker-controlled, making the map growth a remote DoS vector.\n   - Reviewer 1 argued it is internal.\n   - The prompt's context says: \"It sits in the client-side connection lifecycle... to track how many connections have been issued per remote IPv4 address.\" This phrasing strongly suggests `fasthttp.Client` (outbound).\n   - However, `server.go:2110` explicitly calls `s.perIPConnCounter.Register(ip)`. In `fasthttp`, `Server` also supports rate limiting per IP (`MaxConnsPerIP`). If the Server uses this counter, the input is the remote client's IP. \n   - Let's verify the `Server` usage. If `Server` uses `perIPConnCounter` to enforce `MaxConnsPerIP`, then an attacker connecting from many unique IPs would cause the map to grow. Even if `MaxConnsPerIP` is 0 (unlimited), `Register` is called. \n   - Crucially, is the memory growth *unbounded* enough to be a security vulnerability? The map key is `uint32`. The max size is $2^{32}$ entries. Each entry is small (32 bytes). $2^{32} \\times 32 \\text{ bytes} \\approx 128 \\text{ GB}$. This is a finite but large bound. Is it \"unbounded\" in the sense of a vulnerability? Usually, resource exhaustion requires the attacker to be able to trigger it *practically*. \n   - However, there is a more critical issue: **Is the map cleaned up?** The report says \"never deletes the map entry\". If entries are never deleted, the map grows until it contains every unique IP ever seen by the server. For a public server, this will eventually exhaust memory or swap. \n   - But wait, look at the threat model: \"Unbounded allocation... on crafted input\". The input is the IP. Can an attacker craft an input to *maximize* the map growth quickly? They can rotate IPs. \n   - Let's check if there is a defense. Is there a limit on the number of IPs? No. \n   - Is there a cleanup? No. \n   - Therefore, a long-running server exposed to the internet will eventually fill this map with millions/billions of entries, leading to memory exhaustion. This is a valid resource exhaustion DoS. \n\n3. **Reviewer Discrepancy**: Reviewer 1 claimed it's internal and not reachable. Reviewer 2/3 claimed it's reachable via Server. The grep result `server.go:2110` proves it is used in the Server. The Server's input (RemoteAddr) is attacker-controlled. Thus, the path exists. \n\n4. **Verdict**: The bug is real (no deletion), reachable (Server receives inbound connections from attacker-controlled IPs), and causes meaningful harm (memory exhaustion/DoS). The \"bounded by 2^32\" argument is moot because 128GB is enough to crash a typical host, and the growth is cumulative over time without cleanup.\n\nCRUX: The `Unregister` method in `perIPConnCounter` fails to delete map entries when the count reaches zero, and this counter is used in the public `Server` API where the map keys (remote IPs) are attacker-controlled, leading to unbounded memory growth over time.\nGREP: s.perIPConnCounter.Unregister",
    "elapsed": 74.3,
    "tokens": 6236,
    "tokens_in": 5219,
    "tokens_out": 1017,
    "cost_usd": 0.0052429799999999995,
    "file": "peripconn.go",
    "round": 4,
    "grep_used": true,
    "grep_results": "GREP `s.perIPConnCounter.Unregister`:\n```\nserver.go:2112:\t\ts.perIPConnCounter.Unregister(ip)\n```"
  },
  {
    "finding_title": "Unbounded per-IP counter map growth from retained zero-count entries",
    "verdict": "VALID",
    "reasoning": "The reported bug is that `perIPConnCounter.m` grows unbounded because `Unregister` does not delete map entries when the count reaches zero. The code confirms this: `Unregister` performs `cc.m[ip] = n` where `n` is clamped to 0, but never calls `delete(cc.m, ip)`. \n\nTo determine if this is a security vulnerability, I must trace the data flow to the entry point. The `perIPConnCounter` is an internal struct, but it is a field of the public `Server` struct. The `Server` accepts inbound network connections via `Serve`. In `server.go`, when a connection is accepted, the server calls `s.perIPConnCounter.Register(ip)` (seen in grep results at `server.go:2110`). The `ip` is derived from the connection's `RemoteAddr()`, which is the source IP of the client. This is untrusted input controlled by the remote attacker. \n\nAn attacker can exploit this by opening and closing connections from many unique source IP addresses (e.g., via a botnet, DDoS, or IP spoofing if the network allows it). Each unique IP adds an entry to the map `m`. When the connection closes, `Unregister` is called, but the entry remains with a value of 0. There is no mechanism to evict these zero-count entries. Since the map persists for the lifetime of the `Server` instance, the memory footprint grows linearly with the number of unique source IPs observed. In a DoS attack using a large pool of IPs, this can exhaust the server's memory, leading to a crash or denial of service. \n\nThe threat model explicitly lists \"resource bounds (unbounded allocation... on crafted input)\" as a valid vulnerability class for a library. The input (source IP) is untrusted, the bug (missing deletion) is real, and the impact (memory exhaustion) is significant. The fact that the map is bounded by the size of the IPv4 space ($2^{32}$) does not mitigate the issue, as exhausting a server's RAM is achievable long before that theoretical limit is reached, and the growth is cumulative and irreversible. Therefore, the finding is valid.\n\nCRUX: The perIPConnCounter map retains entries for every unique source IP ever seen, even after the connection closes, allowing an attacker to exhaust server memory by rotating through a large number of unique source IPs.\nGREP: delete(cc.m, ip)",
    "elapsed": 62.5,
    "tokens": 6776,
    "tokens_in": 6239,
    "tokens_out": 537,
    "cost_usd": 0.00423138,
    "file": "peripconn.go",
    "round": 5,
    "grep_used": true,
    "grep_results": "GREP `delete`:\n```\nfasthttpadaptor/request.go:50:\t\t\tdelete(r.Header, k)\nprefork/prefork.go:343:\t\tdelete(childProcs, sig.pid)\nargs_test.go:363:\t\tdelete(keys, string(k))\nargs_test.go:613:\t\tt.Fatalf(\"Expected q1 arg to be completely deleted. Current Args: %q\", a.String())\ncookie.go:82:\t// maxAge<0 means delete cookie now, equivalently 'max-age=0'\ncookie.go:204:// be deleted immediately, equivalent to 'max-age=0'. This behavior is\nclient.go:625:\t\t\t\tdelete(m, k)\nargs.go:188:// Del deletes argument with the given key from query args.\nargs.go:193:// DelBytes deletes argument with the given key from query args.\nheader_test.go:668:\t\t\tdelete(m, string(v))\nheader_test.go:732:\t\t\tdelete(m, string(v))\nheader.go:1337:// Del deletes header with the given key.\nheader.go:1343:// DelBytes deletes header with the given key.\nheader.go:1371:// Del deletes header with the given key.\nheader.go:1377:// DelBytes deletes header with the given key.\nheader.go:1690:// you should delete it manually like:\nheader.go:1713:// you should delete it manually like:\nheader.go:1822:// delete cookies before calling in order to reset cookies.\nheader.go:1836:// delete cookies before calling in order to reset cookies.\nheader.go:1850:// delete cookies before calling in order to reset cookies.\nheader.go:1864:// delete cookies before calling in order to reset cookies.\nheader.go:1880:// delete cookies before calling in order to reset cookies.\nserver.go:97:// The function deletes existing file at addr before starting serving.\nserver.go:792:// All the values stored in Request are deleted after returning from RequestHandler.\nserver.go:1107:// All uploaded temporary files are automatically deleted after\nserver.go:1133:// The file is automatically deleted after returning from RequestHandler,\nserver.go:1762:// The function deletes existing file at addr before starting serving.\nserver.go:2688:\t\tdelete(s.idleConns, c)\nserver.go:3085:\t\t\tdelete(s.idleConns, c)\nuserdata_timing_test.go:44:\t\t\t\tdelete(u, k)\n```"
  },
  {
    "verdict": "VALID",
    "reasoning": "[ARBITER] The finding is valid under the library threat model. The 'perIPConnCounter' is embedded in the public 'Server' struct. When the server accepts connections, it calls 'Register' and 'Unregister' using the remote IP address (attacker-controlled data). The 'Unregister' method decrements the count but never deletes the map entry when it reaches zero. This results in the map growing unboundedly with every unique source IP observed, allowing a remote attacker to exhaust server memory (Denial of Service) by rotating through many unique IPs. This constitutes unbounded resource use reachable through the public API.",
    "round": 6,
    "file": "peripconn.go",
    "finding_title": "Unbounded per-IP counter map growth from retained zero-count entries",
    "elapsed": 58.0,
    "tokens_in": 4492,
    "tokens_out": 142,
    "cost_usd": 0.00231264
  }
]