Reject sharded-checkpoint shard paths that escape the checkpoint folder - #4138
Open
AbdullahRasheed45 wants to merge 1 commit into
Open
Reject sharded-checkpoint shard paths that escape the checkpoint folder#4138AbdullahRasheed45 wants to merge 1 commit into
AbdullahRasheed45 wants to merge 1 commit into
Conversation
load_checkpoint_in_model built its shard list by joining the values of a checkpoint's *.index.json weight_map straight onto the checkpoint folder. Those values come from the checkpoint author, and os.path.join follows .. segments and discards the folder entirely for an absolute path, so a malicious checkpoint could make the loader open files anywhere on disk. Validate each shard name before joining: reject absolute paths, and reject normalized paths that land outside the checkpoint folder. The containment check compares normalized path strings rather than realpath, so that legitimately symlinked checkpoints -- the layout the Hugging Face hub cache uses, where a snapshot entry points at a blob outside the snapshot directory -- keep loading.
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.
Closes #4067 (the path-traversal half of the report)
The problem
load_checkpoint_in_modelbuilds its list of shard files from theweight_mapof a sharded checkpoint's*.index.json:Those values come from whoever authored the checkpoint.
os.path.joinfollows..segments, and discards the folder entirely when the second argument is absolute, so a crafted index makes the loader open files outside the checkpoint directory. It is reachable through plainload_checkpoint_in_model(model, "<dir>")andload_checkpoint_and_dispatch(...)— notrust_remote_code, no opt-in flag.Reproduced on
mainwith a checkpoint whoseweight_mappoints at../outside/payload.safetensors, and again with an absolute path: both loaded the outside file successfully.This is not remote code execution — safetensors goes through
safe_openand the torch branch already passesweights_only=True. The impact is an out-of-directory file open, which doubles as an existence and parse oracle for paths on the host.The change
Shard names are validated before they are joined: absolute paths are rejected, and the normalized result must stay inside the checkpoint folder.
The containment check deliberately compares normalized path strings rather than
realpath. That distinction matters: the Hugging Face hub cache storessnapshots/<rev>/model.safetensorsas a symlink into../../blobs/<sha>, so arealpath-based check would resolve outside the snapshot directory and reject perfectly legitimate cached checkpoints. Normalizing the string blocks..traversal without following symlinks.Verification
Both attack vectors are now refused with a clear
ValueError. I also checked the layouts that must keep working, since a fix here can easily be too strict:../outside/payload.safetensorsmodel-00001.safetensorsshards/model-00001.safetensors../../blobs/<sha>./model-00001.safetensorsTests added to
tests/test_modeling_utils.py: one covering the escaping names (relative, nested-relative, absolute), one covering the nested-subdirectory and symlinked-shard layouts so the guard cannot be tightened into breaking the hub cache. The first fails onmain.pytest tests/test_modeling_utils.py→ 43 passed, 3 skipped.ruff(0.13.1, as pinned insetup.py) format and check are clean.Not covered here
The report also describes a DoS where a shard pointed at a FIFO makes
torch.loadblock forever. That needs a separate decision about rejecting non-regular files, which has its own compatibility surface, so I left it out rather than bundling it. Happy to follow up if you'd like it handled the same way.A previous attempt at this (#4070) was closed by the stale bot rather than on review, and the vulnerability is still present on
main.