Skip to content

fix(disagg): use JSON instead of pickle for P2P ZMQ requests (#4804) - #4812

Open
Anai-Guo wants to merge 2 commits into
InternLM:mainfrom
Anai-Guo:fix-disagg-zmq-pickle-rce-4804
Open

fix(disagg): use JSON instead of pickle for P2P ZMQ requests (#4804)#4812
Anai-Guo wants to merge 2 commits into
InternLM:mainfrom
Anai-Guo:fix-disagg-zmq-pickle-rce-4804

Conversation

@Anai-Guo

@Anai-Guo Anai-Guo commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Motivation

Fixes #4804.

The disaggregated-serving P2P connector deserializes peer-supplied bytes with pickle:

# lmdeploy/pytorch/disagg/conn/engine_conn.py
req = await self.p2p_receiver[remote_engine_id].recv_pyobj()   # = pickle.loads()
if isinstance(req, DistServeCacheFreeRequest):                 # check runs *after* loads

recv_pyobj() calls pickle.loads() on the raw frame, so the isinstance guard runs only after any __reduce__ payload has already executed. As analysed in the issue, the reachable path is not the bound PUSH socket but the HTTP endpoint: an unauthenticated POST /distserve/p2p_connect supplies zmq_address, the engine's PULL socket connect()s to that caller-chosen address, and handle_zmq_recv then pickle.loads() whatever that address sends → remote code execution on the engine node (AuthenticationMiddleware is only installed when api_keys is set, and the /distserve/* routes carry no auth dependency).

Modification

The connector's only wire message is DistServeCacheFreeRequest, a pydantic model with two scalar fields (remote_engine_id: str, remote_session_id: int). Both send and receive live in the same class and upgrade together, so switching the codec has no protocol-mismatch risk:

  • zmq_send: send_pyobj(...)send_json(req.model_dump())
  • handle_zmq_recv: recv_pyobj()recv_json() + DistServeCacheFreeRequest.model_validate(...)

recv_json() decodes with json.loads (no code execution) and model_validate enforces the schema before the payload is used, making the old isinstance check redundant. Malformed or off-schema messages are now logged and skipped rather than raising out of the bare while True in the detached task (which previously surfaced only as a task-destroyed warning).

No wire-format compatibility break beyond the coordinated both-ends change here.

Checklist

  • Pre-commit / code style aligned with the existing file
  • Reproduced the deserialization path against main
  • Change is confined to engine_conn.py

🤖 Generated with Claude Code

…M#4804)

recv_pyobj()/send_pyobj() call pickle.loads() on peer-supplied bytes in
the disaggregated-serving P2P connector, and the reachable path is an
unauthenticated POST to /distserve/p2p_connect that makes the engine's
PULL socket connect to a caller-chosen address. A crafted pickle payload
on that address is arbitrary code execution on the engine node.

Switch the connector's only wire message (DistServeCacheFreeRequest) to
send_json()/recv_json() and validate it with model_validate before use,
so decoding is json.loads (no code execution) and the schema check runs
before the payload is acted on. Malformed/off-schema messages are logged
and skipped instead of tearing down the detached receive loop.
@lvhan028
lvhan028 requested review from caikun-pjlab and a lite review from Copilot August 4, 2026 07:53
@lvhan028 lvhan028 added the Bug:P1 label Aug 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR mitigates a remote code execution risk in the disaggregated-serving P2P (ZMQ) connector by removing pickle-based deserialization and replacing it with JSON + schema validation in EngineP2PConnection.

Changes:

  • Switch P2P ZMQ message sending from send_pyobj() (pickle) to send_json() using DistServeCacheFreeRequest.model_dump().
  • Switch receiving from recv_pyobj() (pickle.loads) to recv_json() followed by DistServeCacheFreeRequest.model_validate(...).
  • Add error handling to log and skip malformed/off-schema messages rather than crashing the receive loop.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lmdeploy/pytorch/disagg/conn/engine_conn.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@caikun-pjlab caikun-pjlab left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Unauthenticated pickle deserialization via ZMQ in disaggregated serving → RCE

4 participants