Repository navigation
Conversation
The kGetObj handler used the client-supplied dest_address verbatim as the target for the kRespondToGetObj connection, letting an unauthenticated client make the service connect to an arbitrary host:port and send it the contents of an arbitrary local file (SSRF / file exfiltration). The response host is now always taken from getpeername() of the socket the request arrived on; only the port is read from the header, and it is validated before the request is ACKed. Add regression tests.
…n crash Expand the dest_address tests into parametrized spoofed-host and invalid- port suites, plus edge cases: rejection keeps the connection usable, an unterminated 64-byte dest_address, concurrent spoofed requests, and a public-API happy path where the requester advertises an unreachable IP. Also guard Shutdown() against RespondToGetTasks, which are registered in pending_tasks_ without a promise; shutting down while one was in flight dereferenced a nullptr. Covered by a new regression test.
Python Code Coverage Summary
Minimum allowed line rate is |
C++ Code Coverage Summary
Minimum allowed line rate is |
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
The
kGetObjhandler inTransferServiceused the client-supplieddest_addressfrom theObjInfoHeaderdirectly as the target for thekRespondToGetObjconnection. An unauthenticated client could therefore make the serviceconnect()to an arbitrary host:port and stream the contents of an arbitrary local file (source_obj_id) to it — a Server-Side Request Forgery with file exfiltration.This change pins the response host to the peer that actually sent the request:
HandleGetObjRequestnow resolves the callback address via a new file-local helper,ResolveGetCallbackAddress(), which takes the host fromgetpeername(client_fd)and only the port fromheader.dest_address.absl::SimpleAtoiand range-checked (1..65535); a malformeddest_addressis rejected with akErrorresponse before the ACK is sent, so the service never attempts a connection for a bad request.AsyncGet/AsyncPutAPI, or Python changes. For legitimate peers (which always advertise their ownlocal_address_), behaviour is unchanged.Also fixed: shutdown crash with an in-flight
RespondToGetTaskWhile stress-testing, a pre-existing crash surfaced:
HandleGetObjRequestregistersRespondToGetTasks inpending_tasks_with anullptrpromise, andShutdown()unconditionally calledpromise->set_exception(...)on every pending task. Shutting down while aRespondToGetTaskwas still connecting back segfaulted.Shutdown()now skips promise-less entries (2 lines). Covered byShutdownWithInFlightRespondToGetTaskIsSafe, which was confirmed to segfault without the guard.Tests
New tests in
transfer_service_p2p_test.cpp(raw-socket helpers drive the service directly):SpoofedDestHostTest.GetResponseGoesBackToRequester(×8)203.0.113.1,10.0.0.1,169.254.169.254,0.0.0.0,255.255.255.255,not-a-host,127.0.0.1, empty) are ignored; thekRespondToGetObjarrives at the requester's real address. Fails without the fix.InvalidDestAddressTest.GetRequestIsRejected(×10)kError, no ACK, no connection attempt.GetRequestRejectionKeepsConnectionUsableGetRequestWithUnterminatedDestAddressIsRejecteddest_addressare handled safely and rejected; service stays responsive.ConcurrentSpoofedGetRequestsAllReturnToRequesterGetSucceedsWhenRequesterAdvertisesUnreachableIpShutdownWithInFlightRespondToGetTaskIsSafeFlake check
New tests: 10× shuffled → 220/220 pass; concurrent test 50× → 50/50.
Full
transfer_service_testbinary (excludingMLFLogSinkTest, which has a pre-existing in-process-onlyabsl SetTimeZoneabort and passes under ctest): 5× shuffled → 470/470 pass.pytest tests/replication/test_transer_service.py→ 6/6 pass against the rebuilt extension.Tests pass
Appropriate changes to documentation are included in the PR (N/A — no user-facing behaviour change)