security: WebSocket DoS hardening + audit code fixes (C1/C2/C6/C7/H6)#520
Open
oten91 wants to merge 14 commits into
Open
security: WebSocket DoS hardening + audit code fixes (C1/C2/C6/C7/H6)#520oten91 wants to merge 14 commits into
oten91 wants to merge 14 commits into
Conversation
…bump errors to `critical` - Updated all `eth_chainId` health checks to include chain ID assertions. - Guarded endpoints against incorrect chain routing (e.g., tron/Base incidents). - Adjusted `reputation_signal` from `major_error` to `critical_error` for stricter enforcement. - Added initial rules for previously unchecked services (e.g., Manta, CosmosHub, Provenance, etc.), improving coverage. - Included chain-specific tuning to prevent false positives by tailoring sync allowances and intervals.
…rs to `critical` - Modified `eth_chainId` health check for Sei mainnet to validate against chain ID `0x531`, ensuring correctness. - Upgraded `reputation_signal` from `major_error` to `critical_error` for stricter reliability enforcement.
…speaks-first connections The WebSocket bridge starts reading before the gateway assigns its protocol context: BuildWebsocketRequestContextForEndpoint starts the bridge internally (go b.start()) and only returns the protocol context afterward, which the gateway then assigns. A frame arriving in that window — e.g. an endpoint-speaks-first backend pushing a frame on connect — reached the message processor while protocolCtx was still nil and was rejected with errWebsocketProtocolContextNotReady. The bridge treats any processor error as fatal, so the whole connection was torn down with close code 1011 (the trailing "use of closed network connection" log line is just the aftermath of writing to the closed socket). The existing protocolCtxMu mutex only fixed the older nil-receiver panic; it converted the panic into a connection-fatal error but left the underlying race live. Gate both message processors on a ready channel that is closed once protocolCtx is assigned, instead of rejecting a nil context. A setup-window frame now waits for setup to complete rather than killing the connection. The wait is bounded by the connection context, which is canceled on every failure path, so there is no deadlock; the channel is closed only on the single success path, so there is no double-close. The nil check is retained as a should-never-happen safety net. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…alid responses and session exhaustion - Updated `invalidResponseTimeout` comments to clarify the relationship with session length and endpoint recovery behavior. - Revised `sessionExhaustionTTL` comments for improved readability and precision regarding memory growth and session bounding.
…ening) Two robustness gaps in the same class as the setup-window race, both unbounded-resource DoS on long-lived WS connections: 1. No read size limit. connLoop called ReadMessage() with no SetReadLimit, and gorilla's default is unlimited — a single oversized frame (from a malicious client OR a compromised/malicious endpoint) is buffered whole and can OOM the process. Set SetReadLimit(maxMessageBytes) on both connections in newConnection. 15MB matches go-ethereum's WS wsMessageSizeLimit: large enough for legitimate EVM responses over a subscription, but bounded. On exceed, ReadMessage errors and the connection is torn down cleanly via the existing handleDisconnect path. 2. No write deadline. bridge WriteMessage calls had no SetWriteDeadline, and WriteMessage does not observe the context. A client that stops reading (its TCP receive buffer fills) blocks the bridge's single processing goroutine indefinitely — it can never reach its ctx.Done() select, so pingLoop canceling the context cannot free it, wedging the connection and its resources until the OS TCP timeout. Add writeMessage(), which sets a writeWaitDuration deadline before each data write so a stalled reader fails fast. Control writes (ping/close) already pass their own deadline. Adds Test_Connection_ReadLimit covering the oversized-frame teardown. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…utines Completes the WebSocket DoS-hardening pass (audit H4 + a goroutine-growth sibling) alongside the read-limit/write-deadline fixes. 1. Concurrent-connection cap (H4). Each live WS connection costs ~5-7 goroutines + 2 sockets + buffers, and nothing inside PATH bounded the number of live connections — a flood of upgrades or a slow drain of never-closing connections could exhaust goroutines/FDs. Envoy rate-limits connection *attempts* at the edge, but this is the missing internal backstop. Adds WebsocketConnectionLimiter (nil-safe, lock-free atomic counter) wired through RouterConfig.MaxConcurrentWebsocketConnections (default 10000; negative disables). A slot is reserved at upgrade and released either immediately on setup failure or when the connection terminates (via websocketCtx), guaranteeing exactly one release per acquire. Over-cap upgrades are rejected with 503. 2. Bounded observation processing. BroadcastMessageObservations spawned a goroutine per endpoint message; a high-frequency subscription (e.g. a Polygon logs/newHeads firehose) could spawn unbounded goroutines racing on shared observation state. It now runs inline in the per-connection listener goroutine, which already reads the observation channel sequentially. Bursts are absorbed by the buffered channel and the bridge's existing non-blocking drop-on-full send. Adds unit tests for the limiter (cap, release, nil-disabled, concurrency) and updates RouterConfig default expectations + schema. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
io.ReadAll(httpReq.Body) ran with no size limit on the raw client body (the observation-capture path reads and buffers the whole body before any downstream limit applies), so a single request with a multi-GB body could OOM the process. Wrap the request body with http.MaxBytesReader at the router's single HTTP choke point (handleServiceRequest), before any handler reads it, so every downstream read (RPC-type detection, observation capture, relay) draws from a capped source and a read past the limit fails instead of allocating without bound. Websocket upgrades are unaffected (they do not read req.Body). Configurable via RouterConfig.MaxRequestBodyBytes (default 10MB — well above any legitimate JSON-RPC payload including large batches; negative disables). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…udit C6, C7) C6: The metrics server served http.DefaultServeMux (ListenAndServe(addr, nil)). Importing net/http/pprof (in metrics/pprof.go) registers the /debug/pprof/* handlers on DefaultServeMux via init(), so profiling endpoints (heap dumps, goroutine stacks, CPU profiles) were reachable on the public metrics port. Give the metrics server its own ServeMux so it exposes only /metrics; pprof stays on its dedicated ServePprof port. C7: Pin rymndhng/release-on-push-action from the mutable @master branch to the immutable commit SHA aebba2bbce07a9474bf95e8710e5ee8a9e922fe2 (v0.28.0). A moving branch ref means a compromised upstream would get arbitrary code execution in CI with GITHUB_TOKEN. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oroutines (audit C2) handleBatchRelayRequest spawned one goroutine per batch payload with no pre-spawn cap — the max_batch_payloads limit was only enforced deep inside each spawned goroutine. Combined with per-payload retry/hedge fan-out, a single oversized batch request could spawn tens of thousands of goroutines before any rejection. - Enforce max_batch_payloads BEFORE the spawn loop; oversized batches are rejected up front (per-service override honored, global default otherwise). - Bound concurrent payload goroutines with a semaphore acquired BEFORE each spawn (sized to max_concurrent_relays), so fan-out stays bounded even if the batch cap is disabled. Result channel is fully buffered, so workers always drain and release — no deadlock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oop) The limiter's Acquire used an optimistic add-then-rollback, so the counter could transiently read cap+1 during a concurrent rejection's rollback window — grants stayed correctly capped, but a concurrent Active() read could momentarily exceed max (flaked the concurrency test). Switch to a lock-free compare-and-swap loop that only increments when strictly below the cap, so the counter never overshoots even transiently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rflow (audit H6) perceivedBlock - syncAllowance is a uint64 subtraction computed at five endpoint-selection sites (evm x3, cosmos, noop). When the sync allowance exceeds the perceived block height — early in a chain's life, low heights, or a misconfigured allowance — it underflows to ~MaxUint64, making the "endpoint block >= minAllowed" check fail for every endpoint and rejecting the entire endpoint set. Add qos.MinAllowedBlockNumber(perceivedBlock, syncAllowance), which saturates at 0 (allowance larger than height => no floor => all endpoints pass), and route all five sites through it. Includes unit tests covering the underflow and MaxUint64 cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
More headroom for the largest legitimate payloads (big batches, large eth_getLogs/debug traces, contract deploys). Still bounds a single request's allocation to prevent multi-GB OOM (audit C1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
request_bytes_received_total is a counter, so it only yields totals and averages — there was no way to see p99/max request sizes (e.g. to validate max_request_body_bytes against real traffic). Closes the existing TODO in http_request_context.go. Adds path_request_body_size_bytes (HistogramVec) recorded alongside the existing counter in RecordRequestSize, only for non-zero bodies. Cardinality is bounded by design: it reuses the same fixed rpc_type/service_id labels as the counter (no new label dimensions) and a fixed 11-bucket set (256B..128MB). The Observe runs on the per-request observation-publish path that already executes per request, so the added hot-path cost is a single sub-microsecond histogram Observe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One branch closing a set of correctness/DoS findings from the codebase security audit, split into WebSocket hardening and general (non-WS) fixes. Every change is a guard that fires only on abusive/edge inputs or a latent-bug fix — normal relay and WebSocket traffic behaves identically (see Behavior impact below).
WebSocket hardening
protocolCtxwas assigned (endpoint-speaks-first backends) tore the whole connection down (close 1011). Message processors now gate on areadychannel instead of rejecting a nil context.ReadMessage()had noSetReadLimit; a single oversized frame (from a client or a compromised endpoint) buffered whole → OOM. Capped at 15MB on both connections (matches go-ethereum/borwsMessageSizeLimit, so Polygon — the primary WS service — can't trip it).WriteMessagehad no deadline; a stalled reader wedged the bridge's single goroutine indefinitely. Now bounded bywriteWaitDuration.WebsocketConnectionLimiter(nil-safe, lock-free CAS), configurable viamax_concurrent_websocket_connections(default 10000; negative disables).BroadcastMessageObservationsspawned a goroutine per message (unbounded under a subscription firehose); now runs inline in the per-connection listener.General audit fixes
io.ReadAll(httpReq.Body)was unbounded. Wrapped withhttp.MaxBytesReaderat the router choke point; configurablemax_request_body_bytes(default 75MB; negative disables).handleBatchRelayRequestspawned one goroutine per payload with the cap checked inside each. Now rejects oversized batches before the spawn loop and bounds concurrent payload goroutines with a semaphore.DefaultServeMux, whichnet/http/pprof'sinit()pollutes. Given it its own mux; pprof stays on its dedicated port.rymndhng/release-on-push-action@masterpinned to an immutable SHA (v0.28.0).perceivedBlock - syncAllowance(uint64) underflowed to ~MaxUint64 when the allowance exceeded the height, rejecting every endpoint. Addedqos.MinAllowedBlockNumber(saturating) across all five sites (evm×3, cosmos, noop).Observability
path_request_body_size_byteshistogram — the existing counter only yields averages; this gives p99/max request-size visibility (closes an existing TODO). Bounded labels (reusesrpc_type/service_id), fixed buckets, one Observe on the per-request publish path.Behavior impact
Verified against production traffic (24h): average request body ~1.5KB, largest per-service average ~9KB — so the 75MB body limit is ~50,000× the average, and the 15MB WS limit is above bor's own send limit. The only change that could reject legitimate traffic is the 10000 WS connection cap — raise or disable it if a pod holds more than that concurrently.
Verification
go build ./...,go vet ./...cleanMinAllowedBlockNumberunderflow)Out of scope (separate effort)
CheckOrigin(low risk under PATH's header-auth model)🤖 Generated with Claude Code