security: fix audit data-race crashes + blunt block-height poisoning (A6/A8/B1-B3)#521
Merged
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>
1. Concurrent map iteration crash (evm + cosmos). getDisqualifiedEndpointsResponse ranged endpointStore.endpoints WITHOUT holding endpointsMu, while writers update that map under the lock on every observation. Go turns a concurrent iterate+write into an unrecoverable `fatal error: concurrent map iteration and map write` — reachable by scraping GET /disqualified_endpoints under normal traffic. Now snapshots the map under RLock and validates outside it (basicEndpointValidation takes its own lock, so we release first to avoid lock-order inversion). 2. Unsynchronized UnifiedServicesConfig.Services (gateway). The 5-minute external-config refresh mutates/appends Services (SetServiceSyncAllowance) with no lock while every request ranges it; the append→realloc path can hand a reader a torn slice header → panic. Adds a copylock-safe *sync.RWMutex (initialized in HydrateDefaults, before any concurrency; nil-safe for tests), guards the readers, and copy-on-writes the HealthChecks pointer in the writer so a reader holding a prior copy is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
handleParallelRelayRequests fans out one goroutine per batch payload, all sharing the requestContext and all targeting the single selected endpoint. Each goroutine writes currentRPCType (RPC-type fallback), currentRelayMinerError (trackRelayMinerError), and requestErrorObservation (handleInternalError) while other goroutines read them to build observations, reputation keys, and metrics. These are unsynchronized writes/reads on shared fields — a data race, and a torn read of the *RelayMinerError pointer could crash. Reachable whenever >1 payload reaches HandleServiceRequest (multi-payload relay + health checks). Convert the three per-relay fields to sync/atomic (atomic.Int32 for the RPC-type enum, atomic.Pointer for the two observation pointers). Because all parallel relays hit the SAME endpoint, there is no cross-supplier misattribution — this is purely a memory-safety fix; behavior is unchanged. heuristicRPCType is set once before parallelization (happens-before) so it stays a plain field. Verified with `go test -race ./protocol/shannon/...`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Defense-in-depth against the block-height poisoning / traffic-hijack class (a single endpoint reporting a height far above the real tip filters out every honest endpoint). The full fix is outlier-resistant median consensus for the non-EVM QoS types; these two changes cut the worst case cheaply. A8 — plausibility guard on perceived-height updates. Add qos.IsPlausibleBlockHeight (absolute ceiling 1e12; reject jumps >1e7 above an established perceived height) and apply it at the bare-max update sites (cosmos, solana x2, noop). A report of MaxUint64 (the demonstrated attack) or any absurd value is now ignored and logged as a possible poisoning attempt, rather than becoming the perceived height. Does NOT stop small over-reports — those still require median consensus. A6 — TTL on the Redis perceived_block key. Previously the max-wins CAS SET had no expiry, so a poisoned/too-high value persisted forever: honest lower writes are dropped (max-wins), every replica re-read it on restart, and it needed a manual DEL. The SET now carries a 1h TTL, refreshed ONLY on the update path so a value no honest write can beat expires and self-heals, while a normally advancing chain keeps the key alive. qos tests pass; the Redis integration tests require a Docker Redis container (unavailable locally) and run in CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lastCloseCode/lastCloseText on websocketConnection were read and written
without synchronization:
- handleDisconnect() writes both fields and is called concurrently from
connLoop (read error) and pingLoop (ping-write failure) of the same
connection — on a dead socket both fire at once (write/write).
- GetCloseInfo() reads them from the bridge shutdown path while the peer
connection's loops may still be writing (read/write). The happens-before
established by cancelCtx only covers the connection that triggered the
shutdown, not the other one.
lastCloseText is a string (pointer+len), so a torn read is undefined
behavior, and CI's -race build flags the access. Same class as the B1/B2/B3
race fixes on this branch.
Guard both fields with a dedicated mutex, locked in handleDisconnect (write)
and GetCloseInfo (read). Adds a -race regression test that reproduces both
races before the fix and passes after.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a client Websocket upgrade succeeds but the subsequent dial to the selected upstream endpoint fails (supplier refuses/resets the connection), StartBridge closed the already-upgraded client connection with a raw gorilla Close() — no Websocket close handshake. Clients surface that as an abnormal closure (1006) or a protocol error (1002), which is misleading: the fault is an unavailable upstream, not a client protocol violation. It also gives no reconnect guidance, so clients cannot tell that simply retrying would likely draw a healthy supplier. Send a proper close frame with code 1013 (Try Again Later) and a legible reason before closing the client connection. 1013 correctly signals a transient upstream problem and invites the client to reconnect. Contained to the establishment-failure path in StartBridge, before the bridge struct exists (so the normal shutdown() close-frame path is not yet available). Adds a test that dials a client through a bridge whose upstream is dead and asserts the client receives a 1013 frame (verified to fail with a 1006 abnormal closure before the fix). Note: this does not change endpoint selection — the gateway still selects WebSocket endpoints without reputation filtering (filterByReputation=false), so it can still hand clients to suppliers with failing WS endpoints. Evicting those suppliers is tracked separately (WEBSOCKET_QOS_PLAN). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gorilla/websocket synthesizes a *CloseError{Code: 1006} when a peer drops
the TCP connection without a close handshake (verified: an abrupt
UnderlyingConn().Close() surfaces as "close 1006 (abnormal closure)").
extractCloseInfo caches that code and determineCloseCodeAndMessage forwards
it verbatim to the other peer, so PATH would put 1006 into a close frame via
FormatCloseMessage. Per RFC 6455 §7.4.1, codes 1005/1006/1015 are reserved
for internal endpoint use and MUST NOT appear in a close frame — relay miners
reject it ("websocket: bad close code 1006") and clients see a malformed
frame. (Production-observed on relay-miner logs.)
Add sanitizeCloseCode (1005/1006/1015 -> 1011 Internal Error) and apply it at
the single emit choke point in shutdown(), just before FormatCloseMessage.
This covers every propagation path while leaving the real captured code in
the debug logs. Adds a regression test that drives an abrupt endpoint TCP
close through the bridge and asserts the client receives 1011, not 1006
(verified to fail with 1006 before the fix).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The WebSocket connection duration metric (path_websocket_connection_duration_seconds) always recorded 0, so every service showed an identical, meaningless distribution (p50=0.5s, p90=0.9s — the [0,1) bucket) regardless of how long connections actually lived. Cause: buildWebsocketConnectionObservation stamped ConnectionEstablishedTimestamp to time.Now() on BOTH the established and closed events and never set ConnectionClosedTimestamp. The gateway computes duration as closedTimestamp - establishedTimestamp only when both are present, so the duration was always skipped and recorded as 0. Fix: capture the real establishment time in the bridge lifecycle goroutine and carry it through to the closure observation, which now also stamps the actual close time. The established observation carries only the establishment timestamp; the closed observation carries both, yielding a correct duration. Also add sub-second histogram buckets (0.1/0.25/0.5) so an immediate supplier reset — a connection that establishes then dies in well under a second — is visible and distinguishable from healthy long-lived subscriptions. This is what makes the reset rate (the real WebSocket supplier-quality problem) measurable. Adds a regression test asserting the closure observation carries both timestamps and a non-zero duration (verified to fail before the fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…aturation
Two per-supplier metric families dominate gateway Prometheus cardinality
and silently truncate at the 25K series guard. Bound them and make guard
saturation observable.
Hedge (was ~39% of all gateway series):
- Remove hedge_supplier_latency_seconds{supplier,role} (~12 series/tuple).
- Add hedge_supplier_outcome_total{supplier,role} counter — win-rate at
2 series/supplier, stays under the guard so it is complete (no
first-seen bias).
- Add hedge_role_latency_seconds{role} histogram — latency distribution,
supplier-less (~24 series). Same operator signal, ~89K fewer series/pod.
supplier_signal_total:
- Collapse the 8 reputation signal_type values to a 3-class severity
label (ok/slow/error), cutting the per-(supplier,service) fan 8x -> 3x.
Full error taxonomy remains per-domain on relays_total (status_code +
reputation_signal).
Cardinality guard:
- Emit a one-time WARN when a metric first saturates. Previously silent —
a capped metric looked identical to a healthy one on the wire. Pairs
with the existing path_metrics_label_dropped_total counter. Logger
wired at metrics startup.
Breaking label changes on the two metrics above; grep confirms no repo
dashboard references either old name (all hedge/error panels use
path_relays_total). No functional change.
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
Fixes the highest-severity findings from the 2026-07-15 full security audit (
SECURITY_AUDIT_2026-07-15.md): three data races that crash the pod, and the block-height poisoning / traffic-hijack class (blunted cheaply here; full median fix deferred to its own canary PR).Data races (all
go test -race-detectable, crash under normal traffic)getDisqualifiedEndpointsResponserangedendpointStore.endpointswithout holdingendpointsMuwhile writers update it on every observation →fatal error: concurrent map iteration and map write(uncatchable), reachable by scrapingGET /disqualified_endpoints. Now snapshots underRLockand validates outside it.UnifiedServicesConfig.Servicesrace. The 5-min external-config refresh mutates/appendsServiceswith no lock while every request ranges it; the append→realloc path can hand a reader a torn slice header → panic. Adds a copylock-safe*sync.RWMutex(init inHydrateDefaults, before concurrency; nil-safe for tests), guards readers, and copy-on-writes theHealthCheckspointer.requestContextrace.handleParallelRelayRequestsfans out one goroutine per batch payload, all sharingrcand writingcurrentRPCType/currentRelayMinerError/requestErrorObservationwhile others read them (torn*RelayMinerErrorread → crash). All parallel relays hit the same endpoint, so it's a pure memory-safety issue — converted the three fields tosync/atomic. Verified withgo test -race ./protocol/shannon/....Block-height poisoning — defense-in-depth (A6 + A8)
A single endpoint reporting a height far above the real tip filters out every honest endpoint (100% traffic hijack). The full fix is outlier-resistant median consensus for the non-EVM QoS types (deferred — high blast radius, own PR + canary). These two cut the worst case now:
qos.IsPlausibleBlockHeight(absolute ceiling 1e12; reject jumps >1e7 above an established perceived height) applied at the bare-max update sites (cosmos, solana×2, noop). AMaxUint64report (the demonstrated attack) is ignored + logged instead of becoming the perceived height. Does not stop small over-reports — those need the median.perceived_blockTTL. The max-wins CASSEThad no expiry, so a poisoned/too-high value persisted forever (honest lower writes dropped; re-read on restart; needed manualDEL). Now a 1h TTL refreshed only on the update path: a value no honest write can beat expires and self-heals, while a normally advancing chain keeps the key alive.Verification
go build ./...,go vet ./...cleanIsPlausibleBlockHeight, and-raceon the protocol package)Deferred (tracked in the report)
A1–A5 median-consensus port + EVM accept-band tightening (own PR + canary); mediums (pprof→localhost+auth,
/adminauth, SSRF egress filter, CI action pinning + least-privilegepermissions, endpoint_blocks TTL); CORS/CheckOrigin/heuristic-substring lows.🤖 Generated with Claude Code