Skip to content

Commit 2d14bb4

Browse files
committed
Fix GHSA-x6pr-233j-x5cw: warn before executing an FL-provisioned bundle config
MonaiAlgo/MonaiAlgoStats run a bundle whose whole app directory is provisioned by the FL system: initialize() resolves bundle_root against the server-supplied extra[APP_ROOT], builds a ConfigWorkflow over <app_root>/configs/train.json and runs its `initialize` expressions. Because FL tasks are dispatched per round and executed with no human in the loop, a malicious or compromised aggregation server gets silent code execution on every participating client. The warning added for GHSA-873f-pvrv-4x83 lives in create_workflow(), which this path never calls -- MonaiAlgo constructs ConfigWorkflow directly -- so nothing warned here at all. Executing the config stays unblocked, for the same reason a trust flag was dropped from the earlier fix: MONAI cannot establish whether a bundle is trustworthy, so a flag only teaches operators to set it once and forget it. Both initialize() methods now raise a UserWarning naming the trust boundary. Separately closes GHSA-wvpx-5qmp-46g3 on this path. ConfigWorkflow defaults logging_file to the bundle's own configs/logging.conf and hands it to logging.config.fileConfig, which eval()s the INI's class=/args= fields -- code execution at construction time, before any config is parsed, hidden in a plain INI rather than the MONAI $-DSL. The FL client now defaults extra[ExtraItems.LOGGING_FILE] to False so a server-written logging.conf is never applied; an FL system that wants bundle logging passes an explicit path. ConfigWorkflow and BundleWorkflow warn whenever fileConfig actually runs, so the warning is raised only when a file is really executed. Also fixes a "diable" typo in both initialize() docstrings. Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
1 parent fd8a819 commit 2d14bb4

4 files changed

Lines changed: 233 additions & 5 deletions

File tree

‎monai/bundle/workflows.py‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import os
1616
import sys
1717
import time
18+
import warnings
1819
from abc import ABC, abstractmethod
1920
from collections.abc import Sequence
2021
from copy import copy
@@ -34,6 +35,23 @@
3435
logger = get_logger(module_name=__name__)
3536

3637

38+
def _warn_logging_file_execution(logging_file: str) -> None:
39+
"""
40+
Warn that ``logging_file`` is about to be executed by `logging.config.fileConfig`.
41+
42+
Called immediately before every `fileConfig` invocation in this module, so the warning is only
43+
raised when the file is really executed -- not when it is missing or logging is disabled.
44+
"""
45+
warnings.warn(
46+
f"applying logging config {logging_file}: `logging.config.fileConfig` passes the `class=` and "
47+
"`args=` fields of the INI's handler and formatter sections to Python `eval()`, so this file "
48+
"runs as code. A bundle ships its own `configs/logging.conf` and it is applied by default, "
49+
"before any of the bundle's config is parsed. Only proceed if this file is from a source you "
50+
"trust (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).",
51+
stacklevel=3,
52+
)
53+
54+
3755
class BundleWorkflow(ABC):
3856
"""
3957
Base class for the workflow specification in bundle, it can be a training, evaluation or inference workflow.
@@ -55,6 +73,9 @@ class BundleWorkflow(ABC):
5573
meta_file: filepath of the metadata file, if this is a list of file paths, their contents will be merged in order.
5674
logging_file: config file for `logging` module in the program. for more details:
5775
https://docs.python.org/3/library/logging.config.html#logging.config.fileConfig.
76+
Security note: `fileConfig` passes the INI's `class=` and `args=` fields to Python
77+
`eval()`, so this file runs as code and a warning is raised every time it is applied
78+
(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
5879
5980
"""
6081

@@ -72,6 +93,7 @@ def __init__(
7293
if not os.path.isfile(logging_file):
7394
raise FileNotFoundError(f"Cannot find the logging config file: {logging_file}.")
7495
logger.info(f"Setting logging properties based on config: {logging_file}.")
96+
_warn_logging_file_execution(logging_file)
7597
fileConfig(logging_file, disable_existing_loggers=False)
7698

7799
if meta_file is not None:
@@ -273,6 +295,9 @@ class PythonicWorkflow(BundleWorkflow):
273295
meta_file: filepath of the metadata file, if this is a list of file paths, their contents will be merged in order.
274296
logging_file: config file for `logging` module in the program. for more details:
275297
https://docs.python.org/3/library/logging.config.html#logging.config.fileConfig.
298+
Security note: `fileConfig` passes the INI's `class=` and `args=` fields to Python
299+
`eval()`, so this file runs as code and a warning is raised every time it is applied
300+
(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
276301
277302
"""
278303

@@ -375,6 +400,9 @@ class ConfigWorkflow(BundleWorkflow):
375400
https://docs.python.org/3/library/logging.config.html#logging.config.fileConfig.
376401
If None, default to "configs/logging.conf", which is commonly used for bundles in MONAI model zoo.
377402
If False, the logging logic for the bundle will not be modified.
403+
Security note: `fileConfig` passes the INI's `class=` and `args=` fields to Python
404+
`eval()`, so this file runs as code and a warning is raised every time it is applied
405+
(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
378406
init_id: ID name of the expected config expression to initialize before running, default to "initialize".
379407
allow a config to have no `initialize` logic and the ID.
380408
run_id: ID name of the expected config expression to run, default to "run".
@@ -444,6 +472,7 @@ def __init__(
444472
else:
445473
raise FileNotFoundError(f"Cannot find the logging config file: {logging_file}.")
446474
else:
475+
_warn_logging_file_execution(str(logging_file))
447476
fileConfig(str(logging_file), disable_existing_loggers=False)
448477
logger.info(f"Setting logging properties based on config: {logging_file}.")
449478

‎monai/fl/client/monai_algo.py‎

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
import os
1515
import time
16+
import warnings
1617
from collections.abc import Mapping, MutableMapping
1718
from typing import Any, cast
1819

@@ -34,6 +35,26 @@
3435
logger = get_logger(__name__)
3536

3637

38+
def _warn_provisioned_config_execution(bundle_root: str) -> None:
39+
"""
40+
Warn that the bundle under ``bundle_root`` is about to be executed.
41+
42+
In federated learning the whole app directory -- configs included -- is provisioned by the FL
43+
system, and the aggregation server dispatches `initialize`/`train` tasks that the client runs
44+
on its own, so there is no per-round human interaction to catch a poisoned config.
45+
"""
46+
warnings.warn(
47+
f"executing the bundle config under {bundle_root}, which is provisioned by the FL system: "
48+
'any `"_target_"` value in it is resolved to an importable callable and invoked with no '
49+
'allow list, and any `"$"`-prefixed value is passed to Python `eval()`. A malicious or '
50+
"compromised aggregation server therefore gets code execution on this client, without any "
51+
"per-round human interaction. Only join a federation whose server and app-provisioning "
52+
"channel you trust (see "
53+
"https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-x6pr-233j-x5cw).",
54+
stacklevel=3,
55+
)
56+
57+
3758
def convert_global_weights(global_weights: Mapping, local_var_dict: MutableMapping) -> tuple[MutableMapping, int]:
3859
"""Helper function to convert global weights to local weights format"""
3960
# Before loading weights, tensors might need to be reshaped to support HE for secure aggregation.
@@ -86,6 +107,14 @@ class MonaiAlgoStats(ClientAlgoStats):
86107
"""
87108
Implementation of ``ClientAlgoStats`` to allow federated learning with MONAI bundle configurations.
88109
110+
Security note: the bundle under `bundle_root` is provisioned by the FL system -- `initialize()`
111+
resolves it against `extra[ExtraItems.APP_ROOT]`, which the aggregation server supplies -- and
112+
executing it runs whatever its config contains: any `"_target_"` value is resolved to an
113+
importable callable and invoked with no allow list, and any `"$"`-prefixed value is passed to
114+
Python `eval()`. A malicious or compromised server therefore gets code execution on this client,
115+
with no per-round human interaction. A warning is raised every time a config is executed
116+
(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-x6pr-233j-x5cw).
117+
89118
Args:
90119
bundle_root: directory path of the bundle.
91120
config_train_filename: bundle training config path relative to bundle_root. Can be a list of files;
@@ -135,18 +164,23 @@ def initialize(self, extra=None):
135164
Args:
136165
extra: Dict with additional information that should be provided by FL system,
137166
i.e., `ExtraItems.CLIENT_NAME`, `ExtraItems.APP_ROOT` and `ExtraItems.LOGGING_FILE`.
138-
You can diable the logging logic in the monai bundle by setting {ExtraItems.LOGGING_FILE} to False.
167+
`{ExtraItems.LOGGING_FILE}` defaults to False here, so the bundle's own
168+
"configs/logging.conf" is not applied: it is provisioned by the FL system and
169+
`logging.config.fileConfig` runs the INI's `class=`/`args=` fields through `eval()`
170+
(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
171+
Set it to a logging config file path to opt back in to configuring logging.
139172
140173
"""
141174
if extra is None:
142175
extra = {}
143176
self.client_name = extra.get(ExtraItems.CLIENT_NAME, "noname")
144-
logging_file = extra.get(ExtraItems.LOGGING_FILE, None)
177+
logging_file = extra.get(ExtraItems.LOGGING_FILE, False)
145178
self.logger.info(f"Initializing {self.client_name} ...")
146179

147180
# FL platform needs to provide filepath to configuration files
148181
self.app_root = extra.get(ExtraItems.APP_ROOT, "")
149182
self.bundle_root = os.path.join(self.app_root, self.bundle_root)
183+
_warn_provisioned_config_execution(self.bundle_root)
150184

151185
if self.workflow is None:
152186
config_train_files = self._add_config_files(self.config_train_filename)
@@ -313,6 +347,14 @@ class MonaiAlgo(ClientAlgo, MonaiAlgoStats):
313347
"""
314348
Implementation of ``ClientAlgo`` to allow federated learning with MONAI bundle configurations.
315349
350+
Security note: the bundle under `bundle_root` is provisioned by the FL system -- `initialize()`
351+
resolves it against `extra[ExtraItems.APP_ROOT]`, which the aggregation server supplies -- and
352+
executing it runs whatever its config contains: any `"_target_"` value is resolved to an
353+
importable callable and invoked with no allow list, and any `"$"`-prefixed value is passed to
354+
Python `eval()`. A malicious or compromised server therefore gets code execution on this client,
355+
with no per-round human interaction. A warning is raised every time a config is executed
356+
(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-x6pr-233j-x5cw).
357+
316358
Args:
317359
bundle_root: directory path of the bundle.
318360
local_epochs: number of local epochs to execute during each round of local training; defaults to 1.
@@ -416,19 +458,24 @@ def initialize(self, extra=None):
416458
Args:
417459
extra: Dict with additional information that should be provided by FL system,
418460
i.e., `ExtraItems.CLIENT_NAME`, `ExtraItems.APP_ROOT` and `ExtraItems.LOGGING_FILE`.
419-
You can diable the logging logic in the monai bundle by setting {ExtraItems.LOGGING_FILE} to False.
461+
`{ExtraItems.LOGGING_FILE}` defaults to False here, so the bundle's own
462+
"configs/logging.conf" is not applied: it is provisioned by the FL system and
463+
`logging.config.fileConfig` runs the INI's `class=`/`args=` fields through `eval()`
464+
(see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
465+
Set it to a logging config file path to opt back in to configuring logging.
420466
421467
"""
422468
self._set_cuda_device()
423469
if extra is None:
424470
extra = {}
425471
self.client_name = extra.get(ExtraItems.CLIENT_NAME, "noname")
426-
logging_file = extra.get(ExtraItems.LOGGING_FILE, None)
472+
logging_file = extra.get(ExtraItems.LOGGING_FILE, False)
427473
timestamp = time.strftime("%Y%m%d_%H%M%S")
428474
self.logger.info(f"Initializing {self.client_name} ...")
429475
# FL platform needs to provide filepath to configuration files
430476
self.app_root = extra.get(ExtraItems.APP_ROOT, "")
431477
self.bundle_root = os.path.join(self.app_root, self.bundle_root)
478+
_warn_provisioned_config_execution(self.bundle_root)
432479

433480
if self.train_workflow is None and self.config_train_filename is not None:
434481
config_train_files = self._add_config_files(self.config_train_filename)

‎tests/bundle/test_bundle_workflow.py‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,13 @@
1111

1212
from __future__ import annotations
1313

14+
import json
1415
import os
1516
import shutil
1617
import sys
1718
import tempfile
1819
import unittest
20+
import warnings
1921
from copy import deepcopy
2022
from pathlib import Path
2123

@@ -268,5 +270,60 @@ def test_create_pythonic_workflow(self):
268270
workflow.finalize()
269271

270272

273+
class TestConfigWorkflowWarnsOnLoggingConf(unittest.TestCase):
274+
"""Regression test for GHSA-wvpx-5qmp-46g3: `ConfigWorkflow` defaults `logging_file` to the
275+
bundle's own "configs/logging.conf" and hands it to `logging.config.fileConfig`, which `eval()`s
276+
the INI's `class=`/`args=` fields. It fires in `__init__`, before `initialize()` or `run()`, and
277+
lives in a plain INI rather than the MONAI `$`-DSL, so it is easy to miss when reviewing a
278+
bundle. Applying it is still not blocked -- as for GHSA-873f-pvrv-4x83, MONAI has no way to
279+
establish whether a bundle is trustworthy -- but a `UserWarning` is now raised every time."""
280+
281+
def test_default_logging_conf_warns_and_executes(self):
282+
with tempfile.TemporaryDirectory() as tempdir:
283+
configs = os.path.join(tempdir, "configs")
284+
os.makedirs(configs)
285+
marker = os.path.join(tempdir, "PWNED")
286+
with open(os.path.join(configs, "train.json"), "w") as f:
287+
json.dump({"initialize": []}, f)
288+
# `fileConfig` eval()s the `class=` field, so the tuple subscript runs the payload and
289+
# still yields a usable handler class.
290+
with open(os.path.join(configs, "logging.conf"), "w") as f:
291+
f.write(
292+
"[loggers]\nkeys=root\n[handlers]\nkeys=h\n[formatters]\nkeys=f\n"
293+
"[logger_root]\nlevel=NOTSET\nhandlers=h\n"
294+
"[handler_h]\n"
295+
f"class=(__import__('pathlib').Path({marker!r}).write_text('pwned'), "
296+
"__import__('logging').StreamHandler)[1]\nargs=()\nformatter=f\n"
297+
"[formatter_f]\nformat=%(message)s\n"
298+
)
299+
with self.assertWarnsRegex(UserWarning, r"GHSA-wvpx-5qmp-46g3"):
300+
ConfigWorkflow(config_file=os.path.join(configs, "train.json"), workflow_type="train")
301+
self.assertTrue(os.path.exists(marker))
302+
303+
def test_no_warning_when_logging_disabled(self):
304+
"""No warning when `fileConfig` is never reached -- the file exists but is opted out of."""
305+
with tempfile.TemporaryDirectory() as tempdir:
306+
configs = os.path.join(tempdir, "configs")
307+
os.makedirs(configs)
308+
marker = os.path.join(tempdir, "PWNED")
309+
with open(os.path.join(configs, "train.json"), "w") as f:
310+
json.dump({"initialize": []}, f)
311+
with open(os.path.join(configs, "logging.conf"), "w") as f:
312+
f.write(
313+
"[loggers]\nkeys=root\n[handlers]\nkeys=h\n[formatters]\nkeys=f\n"
314+
"[logger_root]\nlevel=NOTSET\nhandlers=h\n"
315+
"[handler_h]\n"
316+
f"class=(__import__('pathlib').Path({marker!r}).write_text('pwned'), "
317+
"__import__('logging').StreamHandler)[1]\nargs=()\nformatter=f\n"
318+
"[formatter_f]\nformat=%(message)s\n"
319+
)
320+
with warnings.catch_warnings():
321+
warnings.simplefilter("error", UserWarning)
322+
ConfigWorkflow(
323+
config_file=os.path.join(configs, "train.json"), workflow_type="train", logging_file=False
324+
)
325+
self.assertFalse(os.path.exists(marker))
326+
327+
271328
if __name__ == "__main__":
272329
unittest.main()

0 commit comments

Comments
 (0)