fix(security): restrict recipe _component_ imports to trusted roots - #2973
fix(security): restrict recipe _component_ imports to trusted roots#2973Solaris-star wants to merge 1 commit into
Conversation
Untrusted recipe YAML could set `_component_` to any import path (e.g. os.system), causing import side effects or direct dangerous calls during instantiate/validate. Allow only package roots used by official recipes/tests: torchtune, torch, and bitsandbytes. Local names from caller globals still work. Fixes meta-pytorch#2971 Signed-off-by: Solaris-star <820622658@qq.com>
|
Hi @Solaris-star! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
recheck |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
ErenAta16
left a comment
There was a problem hiding this comment.
The enforcement itself is written correctly, which is worth saying because this is where allowlists usually go wrong. It keys on parts[0] with exact set membership rather than a string prefix, so torchtune_evil.payload is rejected rather than sailing through on startswith("torchtune"). Good.
Two problems with the approach, though, and I think they are both blocking.
The allowlist does not meet its own threat model. The stated goal is that untrusted YAML cannot run arbitrary code. But torch is an allowed root, and several arbitrary-code primitives live under it:
torch.hub.load resolves -> function (root 'torch' allowlisted)
torch.load resolves -> function
torch.jit.load resolves -> function
torch.serialization.load resolves -> function
torch.hub.load fetches a repository and executes its hubconf.py, so _component_: torch.hub.load with attacker-chosen arguments is a remote code execution path that survives this change untouched. torch.load on an attacker-supplied path is the classic pickle deserialisation issue. A config author who can set _component_ can still reach all of them.
So the change moves the boundary from "any module" to "any module under three roots", and the three roots still contain what an attacker needs. Blocking os.system and subprocess.run makes the tests pass without making the property true, and I would rather see no allowlist than one that reads as a guarantee it does not provide. If the threat model really is untrusted YAML, the enforcement has to be on the resolved callable, not on the import root, and that is a much larger design question.
It breaks a documented feature. Custom components are a supported workflow with their own docs page, docs/source/basics/custom_components.rst, linked from the index, and tune_cli.rst explicitly tells users to mix custom recipes and configs. Every one of those points _component_ at a user module, none of which is torchtune, torch or bitsandbytes. This change makes all of them fail with "is not allowed" the moment it lands.
That is the part I would expect to generate issues immediately after release. Anyone with a custom dataset, message transform or optimiser has a config that stops working, with an error message that reads like a bug rather than a policy.
On the test changes. Rewriting os.path.join to torch.nn.Linear and os.nonexistent to torch.nonexistent throughout test_get_component_from_path is necessary given the new rule, but it does mean the existing tests no longer exercise the "arbitrary stdlib module" case they were originally written for. Worth noting in the description so it does not look like unrelated churn.
What I would suggest instead. If the concern is running someone else's config, the honest framing is that a torchtune config is executable input, in the same way a conftest.py or a Makefile is, and the fix is documentation plus not running configs you did not write. If you want a mechanism, an opt-in strict mode that users enable when processing untrusted configs would give the property without breaking the documented custom-component path for everyone else.
Happy to be wrong about the custom-component impact if there is a fallback path I missed, but I could not find one in _get_component_from_path: the multi-part branch raises before import_module is reached.
Summary
Recipe configs resolve
_component_viaimport_modulewith no allowlist. Loading or validating untrusted YAML can therefore import arbitrary modules and run top-level code, or call dangerous callables such asos.system.Restrict multi-part (and non-local single-part) component paths to the package roots used by official recipes/tests:
torchtune.*torch.*bitsandbytes.*Caller-local names (globals) remain resolvable for tests/helpers.
Testing
tests/torchtune/config/test_config_utils.py(allowed roots + blocksos.system/subprocess.run)Fixes #2971