Repository navigation
fix(core): reset client-supplied is_builtin on custom model registration - #5530
Conversation
CustomLLMFamilyV2, CustomEmbeddingModelFamilyV2 and CustomRerankModelFamilyV2 declare is_builtin with Config.extra = "allow", so parse_raw() in Supervisor.register_model and Worker.register_model applies a caller-supplied is_builtin value verbatim. allow_trust_remote_code() treats is_builtin as proof that a model family was vetted and loaded from the bundled registry, so a client that sets is_builtin=true on its own custom registration gets the same trust_remote_code grant as a real built-in model. Both register_model methods now reset is_builtin to False right after parse_raw(), before the spec is handed to register_fn or cached. This mirrors the existing family.is_builtin = True assignment done for real built-ins in model/utils.py and llm/__init__.py. Model types without an is_builtin field are unaffected.
There was a problem hiding this comment.
Code Review
This pull request ensures that client-submitted model registrations cannot falsely claim to be built-in by resetting the 'is_builtin' attribute to 'False' during parsing in both the supervisor and worker. A security review comment suggests re-serializing the sanitized 'model_spec' back to the 'model' string in the supervisor to prevent forwarding the unsanitized payload to workers.
qinxuye
left a comment
There was a problem hiding this comment.
LGTM. Reviewed 89b9dbe; 132 focused tests passed locally. Additionally verified targeted-worker forwarding, the remote-code trust gate, and sanitized persistence serialization. The defense-in-depth review thread has been addressed and resolved. Existing persisted registrations are outside the stated scope of this patch.
|
Hi, I'm the reporter of the issue this PR fixes, filed privately as The advisory is still in triage and unpublished, so there's no public record telling
Correcting my earlier note on versions. I have now tested this against running Unauthenticated by default: The
I also verified your fix holds. On 3.5.0 the forged request is refused with the same One note for the advisory text if it is useful: on Two limits on my own testing, so you can weigh it. First, I fired the On timing, for the record rather than as news: I gave notice of a disclosure date on the Happy to draft the advisory text or fill in the CVE fields if that saves you time. |
What
Supervisor.register_model(xinference/core/supervisor.py:2200) andWorker.register_model(xinference/core/worker.py:2357) build the custom model spec withmodel_spec_cls.parse_raw(model), applying the caller's JSON as is.CustomLLMFamilyV2(xinference/model/llm/llm_family.py:202),CustomEmbeddingModelFamilyV2(xinference/model/embedding/core.py:105), the rerank equivalent (xinference/model/rerank/core.py:87) andCustomImageModelFamilyV2(xinference/model/image/core.py:58) declareis_builtin: bool = Falseas a plain field with no write protection, so a registration request body containing"is_builtin": trueproduces a spec with that field set regardless of theConfig.extrasetting.Why
allow_trust_remote_code()(xinference/model/utils.py:3261) returns true whenXINFERENCE_TRUST_REMOTE_CODEis set or whengetattr(model_family, "is_builtin", False)is true, and loaders such asxinference/model/embedding/sentence_transformers/core.py:303pass its result straight intotrust_remote_code.is_builtinmarks models the project itself loaded from the bundled registry:xinference/model/llm/__init__.py:308andxinference/model/utils.py:3234setfamily.is_builtin = Trueright after loading a real built-in family, never from external input.register_modelis the one path where a caller-controlled value reaches the same field unchecked.How
Both
register_modelmethods resetmodel_spec.is_builtin = Falseright afterparse_raw(), before the spec reachesregister_fnor the cache, guarded byhasattrso model types without anis_builtinfield (audio,flexible) stay untouched;videoandworldare not registrable through this code path at all. The reset is duplicated inworker.pybecauseSupervisor.register_modelforwards the raw, unmodified request body to every worker, both through_sync_register_model's cluster-wide fan-out and through an explicitworker_iptarget, without constructing a reset spec on the worker side, so the same gate is needed there independently. Nothing else in either function changed. Not covered by this change:register_custom_model()in eachxinference/model/<type>/__init__.pyreloads persisted registration JSON fromXINFERENCE_MODEL_DIRat process start viaparse_rawand calls the register function directly, so a file written before this fix keeps itsis_builtinvalue on restart.Testing
Environment: fresh clone at
origin/main99868ea7, editable install withpip install -e ".[dev]"(CPU-only torch wheel).pytest -vv xinference/core/tests/test_register_model_is_builtin.py(new test): 2 passed with the patch applied.git checkout origin/main -- xinference/core/supervisor.py xinference/core/worker.py): 2 failed, both assertingis_builtin is Falsewhere the actual value wasTrue, confirming the reset is what the test exercises.pre-commit run --files xinference/core/supervisor.py xinference/core/worker.py xinference/core/tests/test_register_model_is_builtin.py: black, end-of-file-fixer, trailing-whitespace, ruff check, isort, mypy, codespell all passed.pytest -vv xinference/model/llm/tests/test_llm_family.py: 69 passed, 2 skipped, 1 failed (test_query_engine_general, missingllama.cppengine entry). Same failure reproduces on the unpatched base with the identical command, so it predates this change (nollama-cppengine installed in this environment).AGENTS.mdandxinference/core/tests/test_restful_api.py, which need a running supervisor/worker cluster and heavier optional dependencies than this environment has.References
GHSA-v9h5-42jh-j8h5 (found during penetration test by turingpoint, reported privately, unpublished at time of writing)