From ba56a6d029abed931e3d8bcac5f7c91ddd2ccaf2 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Wed, 5 Feb 2025 15:26:32 +0000 Subject: [PATCH 1/7] Move test_image_filter.py --- tests/{ => data}/test_image_rw.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/{ => data}/test_image_rw.py (100%) diff --git a/tests/test_image_rw.py b/tests/data/test_image_rw.py similarity index 100% rename from tests/test_image_rw.py rename to tests/data/test_image_rw.py From 6b5fa64eac84f38efd58cda80db18ac14c72d3e8 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Tue, 11 Aug 2026 10:39:56 +0100 Subject: [PATCH 2/7] Fix GHSA-873f-pvrv-4x83: warn before executing a bundle's config 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 path via a caller-supplied config_file. Either way, loading or running a bundle whose config hasn't been reviewed can execute arbitrary code. create_workflow() -- the shared path both load() and run() use to parse a config file -- now raises a UserWarning immediately before doing so, describing what "_target_"/"$"-expression content can do and linking the advisory. This applies uniformly to every caller of create_workflow(), not just load(). No behavior is blocked: the config is still parsed and executed exactly as before, just with a warning first. An earlier version of this fix added an opt-in trust_remote_code flag to load(), but that was dropped after review: MONAI has no way to establish whether a bundle is actually trustworthy, so a flag like that would only teach callers to set it once and forget about it. Update docstrings on load(), run(), and create_workflow() to describe the risk and point at the advisory. Add TestLoadWarnsOnConfigExecution to tests/bundle/test_bundle_download.py: default load() warns and still executes the config, explicit model= skips config parsing entirely and warns about nothing, and run() warns via the same create_workflow() path. Co-Authored-By: Claude Sonnet 5 Signed-off-by: R. Garcia-Dias --- monai/bundle/scripts.py | 27 +++++++++++++ tests/bundle/test_bundle_download.py | 60 +++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/monai/bundle/scripts.py b/monai/bundle/scripts.py index 63a774bfea5..973f4a4dc24 100644 --- a/monai/bundle/scripts.py +++ b/monai/bundle/scripts.py @@ -648,6 +648,14 @@ def load( """ Load model weights or TorchScript module of a bundle. + Security note: if `model` is `None`, building `network_def` requires parsing the bundle's own + "{workflow_type}.json" config, which can define `"_target_"` components resolved to any importable + callable and `"$"`-prefixed expressions evaluated with Python `eval()`. Only call `load()` this way + for bundles from a source you trust; a warning is printed every time this happens + (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-873f-pvrv-4x83). To skip parsing + the bundle's config entirely, pass an explicit `model=` — only the weights are then loaded, via + `torch.load(..., weights_only=True)`. + Args: name: bundle name. If `None` and `url` is `None`, it must be provided in `args_file`. for example: @@ -935,6 +943,12 @@ def run( """ Specify `config_file` to run monai bundle components and workflows. + Security note: parsing `config_file` can run arbitrary code. Any `"_target_"` value is resolved to an + importable callable and invoked with no allow list, and any `"$"`-prefixed value is passed to Python + `eval()`. Only point this at config files you wrote or otherwise fully trust; never at a config + downloaded from, or otherwise sourced from, an untrusted party. A warning is printed every time this + happens (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-873f-pvrv-4x83). + Typical usage examples: .. code-block:: bash @@ -1929,6 +1943,12 @@ def create_workflow( The workflow should be subclass of `BundleWorkflow` and be available to import. It can be MONAI existing bundle workflows or user customized workflows. + Security note: parsing `config_file` can run arbitrary code. Any `"_target_"` value is resolved to an + importable callable and invoked with no allow list, and any `"$"`-prefixed value is passed to Python + `eval()`. Only point this at config files you wrote or otherwise fully trust; never at a config + downloaded from, or otherwise sourced from, an untrusted party. A warning is printed every time this + happens (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-873f-pvrv-4x83). + Typical usage examples: .. code-block:: python @@ -1966,6 +1986,13 @@ def create_workflow( ) if config_file is not None: + warnings.warn( + f'parsing config_file {config_file}: any `"_target_"` value in it is resolved to an importable ' + 'callable and invoked with no allow list, and any `"$"`-prefixed value is passed to Python ' + "`eval()`. Only proceed if this config is from a source you trust " + "(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-873f-pvrv-4x83).", + stacklevel=2, + ) workflow_ = workflow_class(config_file=config_file, **_args) else: workflow_ = workflow_class(**_args) diff --git a/tests/bundle/test_bundle_download.py b/tests/bundle/test_bundle_download.py index bb213cebd99..00440d96022 100644 --- a/tests/bundle/test_bundle_download.py +++ b/tests/bundle/test_bundle_download.py @@ -24,7 +24,7 @@ import monai.networks.nets as nets from monai.apps import check_hash -from monai.bundle import ConfigParser, create_workflow, load +from monai.bundle import ConfigParser, create_workflow, load, run from monai.bundle.scripts import _examine_monai_version, _list_latest_versions, download from monai.utils import optional_import from tests.test_utils import ( @@ -488,5 +488,63 @@ def test_ngc_download_bundle(self, bundle_name, version, remove_prefix, download ) +class TestLoadWarnsOnConfigExecution(unittest.TestCase): + """Regression tests for GHSA-873f-pvrv-4x83: `load()`/`create_workflow()` parse and execute a + bundle's own config (arbitrary `_target_`/`$`-expression content) whenever `model` is `None`. + There is no opt-in flag -- MONAI has no way to establish whether a bundle is actually + trustworthy, so a flag would only teach callers to always pass it and ignore the risk. Instead, + a `UserWarning` is raised every time this happens, in both `load()` (via `create_workflow()`) + and `run()` (also via `create_workflow()`).""" + + def _stage_malicious_bundle(self, tempdir: str, marker: str) -> str: + name = "evil_bundle" + bundle_root = os.path.join(tempdir, name) + os.makedirs(os.path.join(bundle_root, "configs")) + os.makedirs(os.path.join(bundle_root, "models")) + torch.save({"state_dict": {}}, os.path.join(bundle_root, "models", "model.pt")) + # `marker` is embedded via `!r` (not raw-interpolated) since this string is itself later + # evaluated as Python source -- on Windows, a raw path's backslashes would otherwise be + # misparsed as escape sequences. + payload = f"$__import__('os').system({('echo pwned > ' + marker)!r})" + malicious_config = {"network_def": payload, "initialize": []} + with open(os.path.join(bundle_root, "configs", "train.json"), "w") as f: + json.dump(malicious_config, f) + return name + + def test_default_warns_and_executes_config(self): + with tempfile.TemporaryDirectory() as tempdir: + marker = os.path.join(tempdir, "PWNED") + name = self._stage_malicious_bundle(tempdir, marker) + with self.assertWarns(UserWarning): + with self.assertRaises(AttributeError): + # the malicious config is missing metadata.json and returns a plain `int` for + # `network_def`, so the workflow construction fails after the payload has already + # run -- this mirrors the advisory's own PoC, where the failure happens *after* RCE. + load(name=name, bundle_dir=tempdir, source="github", repo="attacker/repo") + self.assertTrue(os.path.exists(marker)) + + def test_explicit_model_skips_config_parsing(self): + with tempfile.TemporaryDirectory() as tempdir: + marker = os.path.join(tempdir, "PWNED") + name = self._stage_malicious_bundle(tempdir, marker) + model = nets.UNet(spatial_dims=2, in_channels=1, out_channels=1, channels=(4, 8), strides=(2,)) + load(name=name, model=model, bundle_dir=tempdir, source="github", repo="attacker/repo") + self.assertFalse(os.path.exists(marker)) + + def test_run_warns_on_config_execution(self): + with tempfile.TemporaryDirectory() as tempdir: + marker = os.path.join(tempdir, "PWNED") + config_file = os.path.join(tempdir, "train.json") + with open(config_file, "w") as f: + payload = f"$__import__('os').system({('echo pwned > ' + marker)!r})" + json.dump({"initialize": [payload]}, f) + with self.assertWarns(UserWarning): + with self.assertRaises(ValueError): + # no "run" ID is defined, so `workflow.run()` fails after `initialize()` has + # already evaluated the payload above. + run(config_file=config_file) + self.assertTrue(os.path.exists(marker)) + + if __name__ == "__main__": unittest.main() From 9b51a655320b140c958fcb99bb922be9bdb87978 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Tue, 11 Aug 2026 20:22:09 +0100 Subject: [PATCH 3/7] fix: address PR #9057 review feedback - tests/bundle/test_bundle_download.py: assert the advisory-specific warning message (GHSA-873f-pvrv-4x83) instead of any UserWarning in test_default_warns_and_executes_config and test_run_warns_on_config_execution - tests/bundle/test_bundle_download.py: fail test_explicit_model_skips_config_parsing if load() emits a UserWarning, enforcing that the explicit-model path never parses the bundle config Signed-off-by: R. Garcia-Dias --- tests/bundle/test_bundle_download.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/bundle/test_bundle_download.py b/tests/bundle/test_bundle_download.py index 00440d96022..73142315500 100644 --- a/tests/bundle/test_bundle_download.py +++ b/tests/bundle/test_bundle_download.py @@ -15,6 +15,7 @@ import os import tempfile import unittest +import warnings from unittest.case import skipIf, skipUnless from unittest.mock import patch @@ -515,7 +516,7 @@ def test_default_warns_and_executes_config(self): with tempfile.TemporaryDirectory() as tempdir: marker = os.path.join(tempdir, "PWNED") name = self._stage_malicious_bundle(tempdir, marker) - with self.assertWarns(UserWarning): + with self.assertWarnsRegex(UserWarning, r"GHSA-873f-pvrv-4x83"): with self.assertRaises(AttributeError): # the malicious config is missing metadata.json and returns a plain `int` for # `network_def`, so the workflow construction fails after the payload has already @@ -528,7 +529,9 @@ def test_explicit_model_skips_config_parsing(self): marker = os.path.join(tempdir, "PWNED") name = self._stage_malicious_bundle(tempdir, marker) model = nets.UNet(spatial_dims=2, in_channels=1, out_channels=1, channels=(4, 8), strides=(2,)) - load(name=name, model=model, bundle_dir=tempdir, source="github", repo="attacker/repo") + with warnings.catch_warnings(): + warnings.simplefilter("error", UserWarning) + load(name=name, model=model, bundle_dir=tempdir, source="github", repo="attacker/repo") self.assertFalse(os.path.exists(marker)) def test_run_warns_on_config_execution(self): @@ -538,7 +541,7 @@ def test_run_warns_on_config_execution(self): with open(config_file, "w") as f: payload = f"$__import__('os').system({('echo pwned > ' + marker)!r})" json.dump({"initialize": [payload]}, f) - with self.assertWarns(UserWarning): + with self.assertWarnsRegex(UserWarning, r"GHSA-873f-pvrv-4x83"): with self.assertRaises(ValueError): # no "run" ID is defined, so `workflow.run()` fails after `initialize()` has # already evaluated the payload above. From a27a4ca935b6ace589ca0e45392538d051fe34fd Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 13 Aug 2026 11:55:03 +0100 Subject: [PATCH 4/7] fix: address PR #9057 review feedback - tests/bundle/test_bundle_download.py: quote the marker path for the shell explicitly (single-quoted inside the double-quoted command) instead of relying on Python's `!r`, so paths containing spaces are not split by the shell - tests/bundle/test_bundle_download.py: reuse `_stage_malicious_bundle` in test_run_warns_on_config_execution instead of duplicating the payload construction; the helper's config now carries the payload under both "network_def" and "initialize" so it fires whichever way the config is consumed Signed-off-by: R. Garcia-Dias --- tests/bundle/test_bundle_download.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/bundle/test_bundle_download.py b/tests/bundle/test_bundle_download.py index 73142315500..87af20fb0e2 100644 --- a/tests/bundle/test_bundle_download.py +++ b/tests/bundle/test_bundle_download.py @@ -503,11 +503,14 @@ def _stage_malicious_bundle(self, tempdir: str, marker: str) -> str: os.makedirs(os.path.join(bundle_root, "configs")) os.makedirs(os.path.join(bundle_root, "models")) torch.save({"state_dict": {}}, os.path.join(bundle_root, "models", "model.pt")) - # `marker` is embedded via `!r` (not raw-interpolated) since this string is itself later - # evaluated as Python source -- on Windows, a raw path's backslashes would otherwise be - # misparsed as escape sequences. - payload = f"$__import__('os').system({('echo pwned > ' + marker)!r})" - malicious_config = {"network_def": payload, "initialize": []} + # `marker` is wrapped in single quotes inside the double-quoted shell command so a path + # containing spaces isn't split by the shell (bash, cmd, and powershell all honor single + # quotes here); relying on Python's `!r`/`repr()` only protects the Python string literal, + # not how the shell itself tokenizes the resulting command. + payload = f"$__import__('os').system(\"echo pwned > '{marker}'\")" + # included under both keys so the payload runs whether the config is consumed via + # `network_def` (the `load()` tests) or via `initialize` (the `run()` test). + malicious_config = {"network_def": payload, "initialize": [payload]} with open(os.path.join(bundle_root, "configs", "train.json"), "w") as f: json.dump(malicious_config, f) return name @@ -537,10 +540,8 @@ def test_explicit_model_skips_config_parsing(self): def test_run_warns_on_config_execution(self): with tempfile.TemporaryDirectory() as tempdir: marker = os.path.join(tempdir, "PWNED") - config_file = os.path.join(tempdir, "train.json") - with open(config_file, "w") as f: - payload = f"$__import__('os').system({('echo pwned > ' + marker)!r})" - json.dump({"initialize": [payload]}, f) + name = self._stage_malicious_bundle(tempdir, marker) + config_file = os.path.join(tempdir, name, "configs", "train.json") with self.assertWarnsRegex(UserWarning, r"GHSA-873f-pvrv-4x83"): with self.assertRaises(ValueError): # no "run" ID is defined, so `workflow.run()` fails after `initialize()` has From 83e64b32ead1d8d97bf4f25cfb2b754e83489b34 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 13 Aug 2026 16:24:42 +0100 Subject: [PATCH 5/7] test(bundle): parameterize source/repo in download warning test Signed-off-by: R. Garcia-Dias --- tests/bundle/test_bundle_download.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/tests/bundle/test_bundle_download.py b/tests/bundle/test_bundle_download.py index 87af20fb0e2..baad7502a30 100644 --- a/tests/bundle/test_bundle_download.py +++ b/tests/bundle/test_bundle_download.py @@ -96,6 +96,15 @@ {"model.pt": "27952767e2e154e3b0ee65defc5aed38", "model.ts": "97746870fe591f69ac09827175b00675"}, ] + +# (source, repo) pairs covering every `source` accepted by `load()`/`download()`. `repo` only +# matters for sources that read it ("github", "huggingface_hub", "ngc_private"); it's unused +# otherwise but keeps the call shape realistic for each source. +TEST_CASE_SOURCE_GITHUB = ["github", "attacker/repo"] +TEST_CASE_SOURCE_MONAIHOSTING = ["monaihosting", None] +TEST_CASE_SOURCE_NGC = ["ngc", None] +TEST_CASE_SOURCE_HUGGINGFACE_HUB = ["huggingface_hub", "attacker/repo"] + TEST_CASE_NGC_1 = [ "spleen_ct_segmentation", "0.3.7", @@ -515,7 +524,13 @@ def _stage_malicious_bundle(self, tempdir: str, marker: str) -> str: json.dump(malicious_config, f) return name - def test_default_warns_and_executes_config(self): + @parameterized.expand( + [TEST_CASE_SOURCE_GITHUB, TEST_CASE_SOURCE_MONAIHOSTING, TEST_CASE_SOURCE_NGC, TEST_CASE_SOURCE_HUGGINGFACE_HUB] + ) + def test_default_warns_and_executes_config(self, source, repo): + # `source`/`repo` only steer where `download()` would fetch from -- irrelevant here since + # the bundle is already staged on disk, so `load()` never calls `download()`. Parameterized + # anyway to confirm the warning fires the same way regardless of `source`. with tempfile.TemporaryDirectory() as tempdir: marker = os.path.join(tempdir, "PWNED") name = self._stage_malicious_bundle(tempdir, marker) @@ -524,7 +539,7 @@ def test_default_warns_and_executes_config(self): # the malicious config is missing metadata.json and returns a plain `int` for # `network_def`, so the workflow construction fails after the payload has already # run -- this mirrors the advisory's own PoC, where the failure happens *after* RCE. - load(name=name, bundle_dir=tempdir, source="github", repo="attacker/repo") + load(name=name, bundle_dir=tempdir, source=source, repo=repo) self.assertTrue(os.path.exists(marker)) def test_explicit_model_skips_config_parsing(self): From 00e2ed550bf47f3cc57668b5df1488fc7362bc94 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Fri, 14 Aug 2026 08:06:01 +0100 Subject: [PATCH 6/7] DCO Remediation Commit for R. Garcia-Dias I, R. Garcia-Dias , hereby add my Signed-off-by to this commit: ba56a6d029abed931e3d8bcac5f7c91ddd2ccaf2 Signed-off-by: R. Garcia-Dias From c8df55aabd2b86e57515ca22c7389a061cfcff5f Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Fri, 14 Aug 2026 08:30:41 +0100 Subject: [PATCH 7/7] fix(tests): write marker via pathlib instead of shelling out os.system embeds the marker path into a string that is itself parsed as Python source (via ast.parse/eval), so a Windows path's backslashes broke parsing on the Windows CI job. Writing the marker directly through pathlib avoids the shell entirely, sidestepping both the backslash-escaping issue and the shell-splitting-on-spaces concern raised in review. Signed-off-by: R. Garcia-Dias --- tests/bundle/test_bundle_download.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/bundle/test_bundle_download.py b/tests/bundle/test_bundle_download.py index baad7502a30..5beff478ae7 100644 --- a/tests/bundle/test_bundle_download.py +++ b/tests/bundle/test_bundle_download.py @@ -512,11 +512,10 @@ def _stage_malicious_bundle(self, tempdir: str, marker: str) -> str: os.makedirs(os.path.join(bundle_root, "configs")) os.makedirs(os.path.join(bundle_root, "models")) torch.save({"state_dict": {}}, os.path.join(bundle_root, "models", "model.pt")) - # `marker` is wrapped in single quotes inside the double-quoted shell command so a path - # containing spaces isn't split by the shell (bash, cmd, and powershell all honor single - # quotes here); relying on Python's `!r`/`repr()` only protects the Python string literal, - # not how the shell itself tokenizes the resulting command. - payload = f"$__import__('os').system(\"echo pwned > '{marker}'\")" + # writes the marker directly via `pathlib` instead of shelling out through `os.system` -- + # `!r` yields a Python-source-safe literal (handling spaces and Windows backslashes alike) + # with no shell involved to reintroduce quoting/splitting issues. + payload = f"$__import__('pathlib').Path({marker!r}).write_text('pwned')" # included under both keys so the payload runs whether the config is consumed via # `network_def` (the `load()` tests) or via `initialize` (the `run()` test). malicious_config = {"network_def": payload, "initialize": [payload]}