Harden dataexchange against resource-exhaustion DoS#25
Merged
Conversation
Bound the memory, disk, and connection resources a single peer can commit, and fix lossy binary storage and an unvalidated filename cast. - ReadFrame: stop pre-allocating the attacker-declared frame size; grow the payload buffer incrementally as bytes arrive. Lower the default per-frame cap from 1 GiB to 64 MiB (env-overridable). - service: add a per-connection idle/read deadline (default 2m) reset before every frame to drop slowloris peers. - service: default and enforce a total-byte inbox cap (256 MiB) on every receipt, alongside the existing file-count cap. - service + filestream: add a received-files disk quota (default 2 GiB) covering completed files and retained .partial fragments, enforced on write for both the legacy TypeFile and chunked TypeFileStream paths. - service: store binary / non-UTF-8 inbox payloads as base64 with a data_encoding marker instead of corrupting them into a JSON string. - WriteFrame: reject over-long filenames before the uint16 cast so a name cannot wrap or truncate onto the wire. A negative limit value disables that limit; zero selects the default. Adds tests for each: oversized-declared frame rejected without a large allocation, read deadline fires, inbox byte cap enforced, received-files quota enforced, over-long filename rejected, binary round-trips losslessly.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
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.
Hardens the
dataexchangeservice against resource-exhaustion / DoS and fixes two correctness bugs. Each change bounds the memory, disk, or connection resources a single peer can commit.Changes
1 GiB frame cap + up-front allocation → bounded.
ReadFrameno longer pre-allocates the attacker-declared frame length; it grows the payload buffer incrementally as bytes actually arrive (readBounded, 64 KiB initial reservation, geometric growth). A peer that announces a giant in-cap frame and never sends the bytes now ties up only the small initial buffer, not the declared size. The default per-frame cap (DefaultMaxFrameSize) drops from 1 GiB to 64 MiB — large transfers belong on the chunkedTypeFileStreampath, which never buffers a whole file. Still raisable viaPILOT_DATAEXCHANGE_MAX_FRAME.Per-connection read/idle deadline.
handleConnresets a read deadline before every frame (ServiceConfig.IdleTimeout, default 2 min) via an optionalSetReadDeadlinetype-assertion — enforced on the production*driver.Conn, a no-op on test pipes. Slowloris peers are dropped instead of pinning a goroutine forever.Inbox total-byte cap.
InboxMaxBytesnow defaults to 256 MiB (was off) and is enforced on every receipt, alongside the existing file-count cap.Disk quota for received files + stream partials.
ReceivedMaxBytes(default 2 GiB) covers completed files and retained.partialfragments, enforced before every write on both the legacyTypeFilepath and the chunkedTypeFileStreampath (quota gate on INIT + per-chunk debit, so a peer that lies about its declared size still can't overrun).Lossless binary inbox storage. Binary / non-UTF-8 payloads are stored as base64 (
data_b64) with adata_encodingmarker instead of being mangled to U+FFFD inside a JSON string. UTF-8 text is unchanged;IncludeBase64still forces base64 for everything.Filename length validated before the
uint16cast.WriteFramerejects names overmaxFilenameLen(255) up front, so an over-long name can't wrap/truncate onto the wire.A negative value for
InboxMaxBytes/ReceivedMaxBytes/IdleTimeoutdisables that limit (escape hatch); zero selects the default.Tests
New
zz_harden_test.gocovers: oversized-declared frame rejected without a large allocation (asserts the max single read-buffer ask stays bounded), over-cap frame still rejected at the header, over-long filename rejected on write, binary payload round-trips losslessly, inbox byte cap enforced on receipt, received-files quota enforced (legacy + streamed INIT), read deadline fires on an idle peer, and disabled-deadline opt-out. The default-cap pin test was updated to 64 MiB.Validation
GOWORK=off go build ./...andgo build -tags=no_dataexchange ./...— cleanGOWORK=off go vet ./...— cleanGOWORK=off go test -race -parallel 4 ./...— greengofmt -l .— cleancmd/daemon,cmd/pilotctl) builds clean against this module via local replace, in both default andno_dataexchangeconfigsNotes
ServiceConfiggainsReceivedMaxBytesandIdleTimeoutfields (and the disabled-build stub is re-synced); existing web4 call sites compile unchanged.zz_*_test.gofiles lack the!no_dataexchangebuild tag and so failgo vet -tags=no_dataexchangeonmainalready. Left untouched; the CI gate runs the default-tag test build, which is green.