Skip to content

fix(security): replace pickle deserialization with JSON in config_server WebSocket registration (GH-1563) - #1568

Open
Ashfaqbs wants to merge 1 commit into
ModelTC:mainfrom
Ashfaqbs:security/fix-config-server-pickle-rce
Open

fix(security): replace pickle deserialization with JSON in config_server WebSocket registration (GH-1563)#1568
Ashfaqbs wants to merge 1 commit into
ModelTC:mainfrom
Ashfaqbs:security/fix-config-server-pickle-rce

Conversation

@Ashfaqbs

Copy link
Copy Markdown
Contributor

Fixes #1563.

Vulnerability

/visual_register and /pd_master_register in lightllm/server/config_server/api_http.py both accept an unauthenticated WebSocket connection and pass the first frame straight into pickle.loads():

registered_visual_server_obj: VIT_Obj = pickle.loads(await websocket.receive_bytes())

pickle.loads() on untrusted network input allows arbitrary code execution via __reduce__ (CWE-502) — #1563 has a full PoC for /visual_register demonstrating file-write via exec in the Config Server process.

Fix

Both registered objects (VIT_Obj, PD_Master_Obj) are plain dataclasses with only int/str fields, fully representable as JSON — no functional need for pickle here. This switches the wire format: the two legitimate client call sites (visual_only_manager.py, register_loop.py) now send json.dumps(dataclasses.asdict(obj)) as text, and the server parses it through a small validator that checks the payload is a dict with the expected field names and types before constructing the dataclass:

def _parse_vit_obj(raw: dict) -> VIT_Obj:
    if not isinstance(raw, dict):
        raise ValueError("registration payload must be a JSON object")
    node_id = raw.get("node_id")
    host_ip = raw.get("host_ip")
    port = raw.get("port")
    if not isinstance(node_id, int) or not isinstance(host_ip, str) or not isinstance(port, int):
        raise ValueError("invalid VIT_Obj registration payload")
    return VIT_Obj(node_id=node_id, host_ip=host_ip, port=port)

Malformed input closes the connection (code 1008) with a logged reason instead of being deserialized.

Scope note

/pd_master_register has the identical vulnerable pattern in the same file and wasn't in scope of #1563 (which only reports /visual_register), but leaving it unpatched right next to the endpoint this PR fixes didn't seem right, so it's fixed here too rather than filed as a separate report.

/registered_objects and /registered_visual_objects still use pickle.dumps() to serialize server-computed registry state for their HTTP GET responses — left unchanged, since pickle.dumps() of trusted local data isn't the deserialization sink this issue is about.

Verification

No local checkout — Windows NTFS rejects this repo's Triton autotune JSON filenames (: in path segments) even with sparse-checkout, so this was built via the Contents/Git Data API against the three affected files directly. Verified:

  • All three changed files pass python -m py_compile.
  • Manually round-tripped both dataclasses through the exact _parse_* validators standalone (dataclasses.asdictjson.dumpsjson.loads → validator → equality check against the original object) — passes for both VIT_Obj and PD_Master_Obj.
  • Confirmed the validators reject a non-dict payload and a dict with a wrong-typed field, both with a ValueError rather than constructing the dataclass.

…ver WebSocket registration

/visual_register and /pd_master_register both accept an unauthenticated
WebSocket connection and pass the first frame straight into pickle.loads(),
allowing arbitrary code execution via a crafted payload (CWE-502). Fixes ModelTC#1563.

Both registered objects (VIT_Obj, PD_Master_Obj) are plain dataclasses with
only int/str fields, so this switches the wire format to JSON: clients send
dataclasses.asdict(obj) as JSON text, and the server parses it through a
small validator (_parse_vit_obj / _parse_pd_master_obj) that checks the
payload is a dict with the expected field names and types before
constructing the dataclass. Malformed input closes the connection (code
1008) instead of being deserialized.

/pd_master_register has the identical vulnerable pattern in the same file
and wasn't in scope of ModelTC#1563, but leaving it unpatched next to the fixed
/visual_register endpoint didn't seem right, so it's fixed here too.

/registered_objects and /registered_visual_objects still use
pickle.dumps() to serialize server-computed data for their HTTP responses,
which is unrelated to this vulnerability class (pickle.dumps of trusted
local data isn't a deserialization sink) and is left unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]Unauthenticated Code Execution via Unsafe Pickle Deserialization in LightLLM Config Server /visual_register

1 participant