Skip to content

Commit 3e2ff2b

Browse files
garciadiasclaudeericspod
authored
Fix GHSA-873f-pvrv-4x83: warn before executing a bundle's config in load()/run() (#9057)
## Summary Fixes GHSA-873f-pvrv-4x83: GHSA-873f-pvrv-4x83 `monai.bundle.load()`, with its default `model=None`, builds a bundle's network by parsing the bundle's own config through `create_workflow()`. That parsing resolves any `"_target_"` value to an importable callable with no allow list, and passes any `"$"`-prefixed value to Python `eval()`. `monai.bundle.run()` reaches the same code path via a caller-supplied `config_file`. Either way, this means loading or running a bundle whose config you haven't reviewed can execute arbitrary code. ### Design An earlier version of this fix added an opt-in `trust_remote_code` flag to `load()`. Per review discussion, that was dropped: MONAI has no mechanism to actually establish whether a bundle is trustworthy (unlike, say, a per-repo "has custom code" check), so a flag like that mostly teaches people to set it once and forget about it, without giving them a real basis to decide. Instead: - `create_workflow()` — the shared path both `load()` and `run()` use to parse a config file — now raises a `UserWarning` immediately before doing so, spelling out exactly what `"_target_"`/`"$"`-expression content can do and linking this advisory. - No behavior is blocked. Default behavior is unchanged other than the added warning: `load()`/`run()` still parse and execute the config exactly as before. - The warning applies uniformly to every caller of `create_workflow()`, not just `load()`. ### Changes - `monai/bundle/scripts.py`: warning added in `create_workflow()`; docstrings on `load()`, `run()`, and `create_workflow()` updated to describe the risk and point at the advisory. - `tests/bundle/test_bundle_download.py`: `TestLoadWarnsOnConfigExecution` — default `load()` warns and still executes the config (no flag needed), explicit `model=` still skips config parsing entirely (and warns about nothing), and `run()` warns via the same `create_workflow()` path. ## Test plan - [x] `python3 -m unittest tests.bundle.test_bundle_download.TestLoadWarnsOnConfigExecution -v` - [x] Full `tests/bundle/test_bundle_download.py`, `tests/bundle/test_config_parser.py` — no new failures vs. `dev` (remaining failures are pre-existing environment gaps: missing `requests`/`nibabel`, one `pdb`/`bdb` quirk) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com> Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
1 parent 6644898 commit 3e2ff2b

2 files changed

Lines changed: 104 additions & 1 deletion

File tree

monai/bundle/scripts.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -648,6 +648,14 @@ def load(
648648
"""
649649
Load model weights or TorchScript module of a bundle.
650650
651+
Security note: if `model` is `None`, building `network_def` requires parsing the bundle's own
652+
"{workflow_type}.json" config, which can define `"_target_"` components resolved to any importable
653+
callable and `"$"`-prefixed expressions evaluated with Python `eval()`. Only call `load()` this way
654+
for bundles from a source you trust; a warning is printed every time this happens
655+
(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-873f-pvrv-4x83). To skip parsing
656+
the bundle's config entirely, pass an explicit `model=` — only the weights are then loaded, via
657+
`torch.load(..., weights_only=True)`.
658+
651659
Args:
652660
name: bundle name. If `None` and `url` is `None`, it must be provided in `args_file`.
653661
for example:
@@ -935,6 +943,12 @@ def run(
935943
"""
936944
Specify `config_file` to run monai bundle components and workflows.
937945
946+
Security note: parsing `config_file` can run arbitrary code. Any `"_target_"` value is resolved to an
947+
importable callable and invoked with no allow list, and any `"$"`-prefixed value is passed to Python
948+
`eval()`. Only point this at config files you wrote or otherwise fully trust; never at a config
949+
downloaded from, or otherwise sourced from, an untrusted party. A warning is printed every time this
950+
happens (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-873f-pvrv-4x83).
951+
938952
Typical usage examples:
939953
940954
.. code-block:: bash
@@ -1929,6 +1943,12 @@ def create_workflow(
19291943
The workflow should be subclass of `BundleWorkflow` and be available to import.
19301944
It can be MONAI existing bundle workflows or user customized workflows.
19311945
1946+
Security note: parsing `config_file` can run arbitrary code. Any `"_target_"` value is resolved to an
1947+
importable callable and invoked with no allow list, and any `"$"`-prefixed value is passed to Python
1948+
`eval()`. Only point this at config files you wrote or otherwise fully trust; never at a config
1949+
downloaded from, or otherwise sourced from, an untrusted party. A warning is printed every time this
1950+
happens (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-873f-pvrv-4x83).
1951+
19321952
Typical usage examples:
19331953
19341954
.. code-block:: python
@@ -1966,6 +1986,13 @@ def create_workflow(
19661986
)
19671987

19681988
if config_file is not None:
1989+
warnings.warn(
1990+
f'parsing config_file {config_file}: any `"_target_"` value in it is resolved to an importable '
1991+
'callable and invoked with no allow list, and any `"$"`-prefixed value is passed to Python '
1992+
"`eval()`. Only proceed if this config is from a source you trust "
1993+
"(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-873f-pvrv-4x83).",
1994+
stacklevel=2,
1995+
)
19691996
# pyrefly: ignore [unexpected-keyword]
19701997
workflow_ = workflow_class(config_file=config_file, **_args)
19711998
else:

tests/bundle/test_bundle_download.py

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import os
1616
import tempfile
1717
import unittest
18+
import warnings
1819
from unittest.case import skipIf, skipUnless
1920
from unittest.mock import patch
2021

@@ -24,7 +25,7 @@
2425

2526
import monai.networks.nets as nets
2627
from monai.apps import check_hash
27-
from monai.bundle import ConfigParser, create_workflow, load
28+
from monai.bundle import ConfigParser, create_workflow, load, run
2829
from monai.bundle.scripts import _examine_monai_version, _list_latest_versions, download
2930
from monai.utils import optional_import
3031
from tests.test_utils import (
@@ -95,6 +96,15 @@
9596
{"model.pt": "27952767e2e154e3b0ee65defc5aed38", "model.ts": "97746870fe591f69ac09827175b00675"},
9697
]
9798

99+
100+
# (source, repo) pairs covering every `source` accepted by `load()`/`download()`. `repo` only
101+
# matters for sources that read it ("github", "huggingface_hub", "ngc_private"); it's unused
102+
# otherwise but keeps the call shape realistic for each source.
103+
TEST_CASE_SOURCE_GITHUB = ["github", "attacker/repo"]
104+
TEST_CASE_SOURCE_MONAIHOSTING = ["monaihosting", None]
105+
TEST_CASE_SOURCE_NGC = ["ngc", None]
106+
TEST_CASE_SOURCE_HUGGINGFACE_HUB = ["huggingface_hub", "attacker/repo"]
107+
98108
TEST_CASE_NGC_1 = [
99109
"spleen_ct_segmentation",
100110
"0.3.7",
@@ -488,5 +498,71 @@ def test_ngc_download_bundle(self, bundle_name, version, remove_prefix, download
488498
)
489499

490500

501+
class TestLoadWarnsOnConfigExecution(unittest.TestCase):
502+
"""Regression tests for GHSA-873f-pvrv-4x83: `load()`/`create_workflow()` parse and execute a
503+
bundle's own config (arbitrary `_target_`/`$`-expression content) whenever `model` is `None`.
504+
There is no opt-in flag -- MONAI has no way to establish whether a bundle is actually
505+
trustworthy, so a flag would only teach callers to always pass it and ignore the risk. Instead,
506+
a `UserWarning` is raised every time this happens, in both `load()` (via `create_workflow()`)
507+
and `run()` (also via `create_workflow()`)."""
508+
509+
def _stage_malicious_bundle(self, tempdir: str, marker: str) -> str:
510+
name = "evil_bundle"
511+
bundle_root = os.path.join(tempdir, name)
512+
os.makedirs(os.path.join(bundle_root, "configs"))
513+
os.makedirs(os.path.join(bundle_root, "models"))
514+
torch.save({"state_dict": {}}, os.path.join(bundle_root, "models", "model.pt"))
515+
# writes the marker directly via `pathlib` instead of shelling out through `os.system` --
516+
# `!r` yields a Python-source-safe literal (handling spaces and Windows backslashes alike)
517+
# with no shell involved to reintroduce quoting/splitting issues.
518+
payload = f"$__import__('pathlib').Path({marker!r}).write_text('pwned')"
519+
# included under both keys so the payload runs whether the config is consumed via
520+
# `network_def` (the `load()` tests) or via `initialize` (the `run()` test).
521+
malicious_config = {"network_def": payload, "initialize": [payload]}
522+
with open(os.path.join(bundle_root, "configs", "train.json"), "w") as f:
523+
json.dump(malicious_config, f)
524+
return name
525+
526+
@parameterized.expand(
527+
[TEST_CASE_SOURCE_GITHUB, TEST_CASE_SOURCE_MONAIHOSTING, TEST_CASE_SOURCE_NGC, TEST_CASE_SOURCE_HUGGINGFACE_HUB]
528+
)
529+
def test_default_warns_and_executes_config(self, source, repo):
530+
# `source`/`repo` only steer where `download()` would fetch from -- irrelevant here since
531+
# the bundle is already staged on disk, so `load()` never calls `download()`. Parameterized
532+
# anyway to confirm the warning fires the same way regardless of `source`.
533+
with tempfile.TemporaryDirectory() as tempdir:
534+
marker = os.path.join(tempdir, "PWNED")
535+
name = self._stage_malicious_bundle(tempdir, marker)
536+
with self.assertWarnsRegex(UserWarning, r"GHSA-873f-pvrv-4x83"):
537+
with self.assertRaises(AttributeError):
538+
# the malicious config is missing metadata.json and returns a plain `int` for
539+
# `network_def`, so the workflow construction fails after the payload has already
540+
# run -- this mirrors the advisory's own PoC, where the failure happens *after* RCE.
541+
load(name=name, bundle_dir=tempdir, source=source, repo=repo)
542+
self.assertTrue(os.path.exists(marker))
543+
544+
def test_explicit_model_skips_config_parsing(self):
545+
with tempfile.TemporaryDirectory() as tempdir:
546+
marker = os.path.join(tempdir, "PWNED")
547+
name = self._stage_malicious_bundle(tempdir, marker)
548+
model = nets.UNet(spatial_dims=2, in_channels=1, out_channels=1, channels=(4, 8), strides=(2,))
549+
with warnings.catch_warnings():
550+
warnings.simplefilter("error", UserWarning)
551+
load(name=name, model=model, bundle_dir=tempdir, source="github", repo="attacker/repo")
552+
self.assertFalse(os.path.exists(marker))
553+
554+
def test_run_warns_on_config_execution(self):
555+
with tempfile.TemporaryDirectory() as tempdir:
556+
marker = os.path.join(tempdir, "PWNED")
557+
name = self._stage_malicious_bundle(tempdir, marker)
558+
config_file = os.path.join(tempdir, name, "configs", "train.json")
559+
with self.assertWarnsRegex(UserWarning, r"GHSA-873f-pvrv-4x83"):
560+
with self.assertRaises(ValueError):
561+
# no "run" ID is defined, so `workflow.run()` fails after `initialize()` has
562+
# already evaluated the payload above.
563+
run(config_file=config_file)
564+
self.assertTrue(os.path.exists(marker))
565+
566+
491567
if __name__ == "__main__":
492568
unittest.main()

0 commit comments

Comments
 (0)