fix(security): replace pickle deserialization with JSON in config_server WebSocket registration (GH-1563) - #1568
Open
Ashfaqbs wants to merge 1 commit into
Open
Conversation
…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.
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.
Fixes #1563.
Vulnerability
/visual_registerand/pd_master_registerinlightllm/server/config_server/api_http.pyboth accept an unauthenticated WebSocket connection and pass the first frame straight intopickle.loads():pickle.loads()on untrusted network input allows arbitrary code execution via__reduce__(CWE-502) — #1563 has a full PoC for/visual_registerdemonstrating file-write viaexecin the Config Server process.Fix
Both registered objects (
VIT_Obj,PD_Master_Obj) are plain dataclasses with onlyint/strfields, 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 sendjson.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:Malformed input closes the connection (code 1008) with a logged reason instead of being deserialized.
Scope note
/pd_master_registerhas 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_objectsand/registered_visual_objectsstill usepickle.dumps()to serialize server-computed registry state for their HTTP GET responses — left unchanged, sincepickle.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:python -m py_compile._parse_*validators standalone (dataclasses.asdict→json.dumps→json.loads→ validator → equality check against the original object) — passes for bothVIT_ObjandPD_Master_Obj.ValueErrorrather than constructing the dataclass.