Repository navigation
Reject path traversal in the transformers_weights config field - #46890
Cyrilvallez merged 2 commits into
Conversation
|
Hey! In #46237 I think we used a different approach so it doesn't break when loading from snapshot dirs, right? |
188e7bb to
070134b
Compare
|
Good catch — you're right, thanks. I'd used os.path.realpath, which follows symlinks, so it would reject loads from a populated HF cache: the snapshot files are symlinks into the sibling blobs/ dir and resolve outside the snapshot dir. Confirmed that locally. Switched to the same lexical approach as #46237. os.path.abspath + os.path.commonpath (no realpath/resolve, so symlinks aren't followed). It still blocks .. and absolute paths but leaves snapshot loads intact. Added a regression test that loads a symlinked snapshot-style weights file so it can't regress. |
|
Looks good! One last point: Can we get rid of the big Claude comments? They can just be removed entirely, or reduced to a single line that explains we're catching path traversal bugs. Also this touches core code so cc @Cyrilvallez @vasqu if you're okay with it! |
`transformers_weights` in config.json names the checkpoint file to load. During `from_pretrained` it was joined onto the model directory and opened after only a suffix check, with no containment validation, so a malicious local model directory could make `AutoModel.from_pretrained(...)` read a `.safetensors` file outside the model directory (path traversal, CWE-22), with no `trust_remote_code`. Validate that the resolved path stays inside the model directory using a purely lexical `os.path.abspath` + `os.path.commonpath` check (not realpath/Path.resolve, so it does not follow symlinks and does not break loading from HF-cache snapshot dirs whose files symlink into a sibling blobs/ directory) — same approach as huggingface#46237. A legitimate in-repo filename still loads. Regression tests: rejects `..`/absolute `transformers_weights`, and still loads a symlinked snapshot-style weights file.
070134b to
260862e
Compare
|
Done — removed the comments. Let me know if anything else is needed! |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
CI recapDashboard: View test results in Grafana |
|
Thanks for the review, @Cyrilvallez! Could you merge it when you have a moment? I don't have write access. Appreciate it! |
|
Merging! |
…gingface#46890) * Reject path traversal in the `transformers_weights` config field `transformers_weights` in config.json names the checkpoint file to load. During `from_pretrained` it was joined onto the model directory and opened after only a suffix check, with no containment validation, so a malicious local model directory could make `AutoModel.from_pretrained(...)` read a `.safetensors` file outside the model directory (path traversal, CWE-22), with no `trust_remote_code`. Validate that the resolved path stays inside the model directory using a purely lexical `os.path.abspath` + `os.path.commonpath` check (not realpath/Path.resolve, so it does not follow symlinks and does not break loading from HF-cache snapshot dirs whose files symlink into a sibling blobs/ directory) — same approach as huggingface#46237. A legitimate in-repo filename still loads. Regression tests: rejects `..`/absolute `transformers_weights`, and still loads a symlinked snapshot-style weights file. * clearer --------- Co-authored-by: Cyril Vallez <cyril.vallez@gmail.com>
…es when probing the expert layout (#15386) ## Summary `gpt_oss_checkpoint_has_fused_experts` decides between the fused and the split (pre-quantized) bitsandbytes expert layout by reading tensor names from the checkpoint. `_checkpoint_tensor_dtypes` only looked at the standard filenames (`model.safetensors`, its index, `pytorch_model.bin`, its index), but Transformers loads `config.transformers_weights` first whenever it is set, and never falls back to the standard names in that case: - transformers 4.57.6: `PreTrainedModel.from_pretrained` reads `getattr(config, "transformers_weights", None)` and `_get_resolved_checkpoint_files` uses it ahead of every standard name for local directories and Hub repos (`modeling_utils.py` lines 918-921 and 1010-1012, call site at 4869). - transformers main (5.x): same behaviour in `_get_resolved_checkpoint_files` (`modeling_utils.py` lines 556-590 and 636-638, call site at 4259), plus a path traversal check added in huggingface/transformers#46890. So a repo that has a split `model.safetensors` next to a fused `chosen.safetensors` with `"transformers_weights": "chosen.safetensors"` was probed as split, which routes the load onto the split-expert classes and skips the fused-layout float16 refusal in `loader.py`. The reverse (fused standard file, split explicit file) was probed as fused. ## Change - `_checkpoint_tensor_dtypes` takes `transformers_weights`. When it names a `*.safetensors` file or a `*.safetensors.index.json` index, only that file is read (no variant, no `use_safetensors`, no `.bin` fallback, no standard-name Hub metadata shortcut), matching Transformers. - A name that escapes the model directory returns "unknown", since newer Transformers rejects it. - A name that is not a safetensors file or index keeps the existing order (Transformers raises on it at load anyway). - `gpt_oss_checkpoint_has_fused_experts` forwards `config.transformers_weights`. Configs without the field behave exactly as before. ## Reachability None of the public gpt-oss configs I checked set the field (`openai/gpt-oss-20b`, `openai/gpt-oss-120b`, `unsloth/gpt-oss-20b-unsloth-bnb-4bit`, `unsloth/gpt-oss-20b-bnb-4bit`, `unsloth/gpt-oss-20b-BF16`), and `PretrainedConfig.save_pretrained` deletes it before writing. Only a hand-edited config can carry it, so this is a low-severity consistency fix. ## Tests New `tests/test_gpt_oss_transformers_weights_layout.py` (CPU, tiny generated safetensors): | Test | base (origin/main) | this branch | | --- | --- | --- | | `test_explicit_file_decides_the_layout[split_standard_fused_chosen]` | FAIL | PASS | | `test_explicit_file_decides_the_layout[fused_standard_split_chosen]` | FAIL | PASS | | `test_explicit_index_decides_the_layout` | FAIL | PASS | | `test_explicit_file_ignores_variant_and_use_safetensors` | FAIL | PASS | | `test_invalid_or_escaping_explicit_names_never_read_other_files` | FAIL | PASS | | `test_hub_probe_reads_the_explicit_file` | FAIL | PASS | The existing gpt-oss layout suites (`test_gpt_oss_bnb_checkpoint_layout.py`, `test_gpt_oss_bf16_load_in_4bit.py`, `test_gpt_oss_fused_4bit_checkpoint_roundtrip.py`) still pass. I also checked the probe against Transformers' own `_get_resolved_checkpoint_files` (5.17.0) on both layouts: the probe now agrees with the file Transformers actually loads. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
from_pretrainedresolves which checkpoint file to load via_get_resolved_checkpoint_files. When a model'sconfig.jsoncarries atransformers_weightsfield, that untrusted value is joined onto the model directory and opened after only a suffix check (it must end.safetensors/.safetensors.index.json, or equaladapter_model.bin) — with no../ absolute-path / containment validation (modeling_utils.py:597). Becauseos.path.joindrops the prefix on an absolute component and..walks out of the directory, a malicious local model directory makesAutoModel.from_pretrained(...)open a.safetensors-format file outside the model directory (path traversal, CWE-22), with notrust_remote_code.The escape base is
pretrained_model_name_or_path— the directory the caller chose to load, not a config field — so the file is expected to live inside it; the same containment mistake was fixed on the write side for chat templates (#46191) and Bark voice presets (#46237), and this is the read side in the core loader. The read is constrained to.safetensors-format files (no arbitrary-text disclosure) andweights_only=Truemeans no code execution, but a crafted local model can still read an out-of-directory checkpoint and acts as a file-existence oracle — in multi-tenant model hosting this can surface another tenant's weights.The fix validates that the resolved
transformers_weightspath stays inside the model directory (rejecting..and absolute paths), leaving the sibling branches (which use hard-coded constants) untouched. A legitimate in-repo filename still loads.AI assistance was used while drafting this change; I reviewed every line and ran the tests below.
What does this PR do?
Adds a containment check so a
transformers_weightsvalue inconfig.jsoncannot makefrom_pretrainedopen a.safetensorsfile outside the model directory (path traversal, CWE-22, notrust_remote_code), matching the existing write-side traversal fixes (#46191, #46237).Tests run (
torch==2.12.1):Reported via huntr: https://huntr.com/bounties/bdd08581-3537-4ca1-bd8c-de724529617a
Fixes # (security hardening)
Before submitting
Who can review?