-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix GHSA-873f-pvrv-4x83: warn before executing a bundle's config in load()/run() #9057
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| import os | ||
| import tempfile | ||
| import unittest | ||
| import warnings | ||
| from unittest.case import skipIf, skipUnless | ||
| from unittest.mock import patch | ||
|
|
||
|
|
@@ -24,7 +25,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 +489,65 @@ 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.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 | ||
| # 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") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other values of |
||
| 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,)) | ||
| 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)) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| 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})" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. Perhaps just use |
||
| json.dump({"initialize": [payload]}, f) | ||
| 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. | ||
| run(config_file=config_file) | ||
| self.assertTrue(os.path.exists(marker)) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment from Coderabbit was just about how a command string is put together. The issue is if
markercontains spaces which will cause the path to be split by the shell when executed. Your solution appears vulnerable to that still. Considermarker="/tmp/foo bar", what wouldpayloadbe with that value?I think bash, cmd, and powershell will work with single quotes around the path, and I'd rather add them explicitly like this rather than rely on repr with
!r.shlex.joindoesn't work properly here either since it wants to quote the>operator.