Fix GHSA-x6pr-233j-x5cw: warn before executing an FL-provisioned bundle config (#9078)

## Summary

Fixes GHSA-x6pr-233j-x5cw:
https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-x6pr-233j-x5cw
Also closes GHSA-wvpx-5qmp-46g3:
https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3

`MonaiAlgo`/`MonaiAlgoStats` run a bundle whose entire app directory is
provisioned by the FL system. `initialize(extra)` resolves `bundle_root
= os.path.join(extra[APP_ROOT], self.bundle_root)` — `APP_ROOT` is
supplied by the aggregation server — then 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 server gets silent code execution
on every participating client.

The `UserWarning` added in #9057 for GHSA-873f-pvrv-4x83 lives in
`create_workflow()`. This path never calls it — `MonaiAlgo` constructs
`ConfigWorkflow` directly — so nothing warned here at all.

### Design

Executing the config stays unblocked, for the same reason the
`trust_remote_code` flag was dropped from #9057: MONAI has no mechanism
to establish whether a bundle is trustworthy, so a flag mostly teaches
operators to set it once and forget about it. Both `initialize()`
methods now warn, naming the trust boundary (`extra[APP_ROOT]`) and the
absence of per-round human interaction.

The one behaviour change is narrowly scoped, and it targets the sink
with no functional role in FL. `ConfigWorkflow` defaults `logging_file`
to the bundle's own `configs/logging.conf` and passes it to
`logging.config.fileConfig`, which `eval()`s the INI's `class=`/`args=`
fields. That is code execution at construction time, before any config
is parsed, and it hides in a plain INI rather than the MONAI `$`-DSL —
easy to miss when reviewing a bundle. The FL client now treats
`extra[ExtraItems.LOGGING_FILE]` as `False` both when the key is absent
and when it is explicitly `None`, so a server-written `logging.conf` is
never applied. `None` needs the same treatment as absent because it was
the pre-PR default and `ConfigWorkflow` reads it as "fall back to the
bundle's own `configs/logging.conf`" — exactly the file this change
exists to keep away from `fileConfig`. An FL system that wants bundle
logging passes an explicit path, through the key that already exists for
it.

The `fileConfig` warning sits inside the branch that actually calls it,
not at the top of `__init__`. That keeps it truthful (nothing runs when
the file is absent or logging is disabled, both common) and avoids
double-warning callers who already got the `create_workflow()` warning,
which is about `_target_`/`$` rather than the INI.

### Changes

- `monai/fl/client/monai_algo.py`: warning in both `initialize()`
methods; `ExtraItems.LOGGING_FILE` treated as `False` when absent or
explicitly `None`; security notes on both class docstrings; both
`initialize()` docstrings rewritten for the new default (this also fixes
a `diable` typo).
- `monai/bundle/workflows.py`: `_warn_logging_file_execution()` called
immediately before each of the two `fileConfig` invocations;
`logging_file` docstring entries updated on `BundleWorkflow`,
`PythonicWorkflow` and `ConfigWorkflow`.
- `tests/fl/monai_algo/test_fl_monai_algo.py`:
`TestFLMonaiAlgoWarnsOnProvisionedConfig` — stages an app whose
`train.json` and `logging.conf` each drop a distinct marker, for both
`MonaiAlgo` and `MonaiAlgoStats`. Asserts the config still executes with
the advisory warning; that the server's `logging.conf` no longer does,
whether the key is absent or explicitly `None`; that an explicit path
opts back in; and that no `fileConfig` warning fires when nothing is
executed.
- `tests/bundle/test_bundle_workflow.py`:
`TestConfigWorkflowWarnsOnLoggingConf` — a bundle's default
`configs/logging.conf` warns and still applies; `logging_file=False`
neither warns nor applies it.

Both new test classes snapshot and restore the root logger, closing any
handler `fileConfig` installs. The suite runs in one process, so without
that they would leak a root handler and formatter into every test that
follows.

## Test plan

- [x] `python -m unittest tests.fl.monai_algo.test_fl_monai_algo` — 17
passed
- [x] `python -m unittest tests.fl.test_fl_monai_algo_stats` — 3 passed
- [x] `python -m unittest
tests.bundle.test_bundle_workflow.TestConfigWorkflowWarnsOnLoggingConf`
— 2 passed
- [x] `python -m unittest
tests.bundle.test_bundle_download.TestLoadWarnsOnConfigExecution` — 6
passed, no double-warn regression on the #9057 fix
- [x] Each new assertion checked against the unpatched code first — the
advisory's payload writes its marker via the bundle config and via
`logging.conf` before the change, and only via the bundle config after
it
- [x] Root logger verified identical before and after both new test
classes run
- [x] `black`, `isort`, `ruff` clean on the changed files

### Types of changes
- [ ] Non-breaking change (fix or new feature that would not break
existing functionality).
- [x] Breaking change (fix or new feature that would cause existing
functionality to change).
- [x] New tests added to cover the changes.
- [x] In-line docstrings updated.

The breaking-change box is for the `LOGGING_FILE` default only: an FL
deployment relying on the bundle shipping its own `logging.conf` now has
to pass the path explicitly. Everything else is additive.

---------

Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>

Author: garciadias

monai/bundle/workflows.py

@@ -15,6 +15,7 @@
 import os
 import sys
 import time
+import warnings
 from abc import ABC, abstractmethod
 from collections.abc import Sequence
 from copy import copy
@@ -34,6 +35,23 @@
 logger = get_logger(module_name=__name__)
 
 
+def _warn_logging_file_execution(logging_file: str) -> None:
+    """
+    Warn that ``logging_file`` is about to be executed by `logging.config.fileConfig`.
+
+    Called immediately before every `fileConfig` invocation in this module, so the warning is only
+    raised when the file is really executed -- not when it is missing or logging is disabled.
+    """
+    warnings.warn(
+        f"applying logging config {logging_file}: `logging.config.fileConfig` passes the `class=` and "
+        "`args=` fields of the INI's handler and formatter sections to Python `eval()`, so this file "
+        "runs as code. A bundle ships its own `configs/logging.conf` and it is applied by default, "
+        "before any of the bundle's config is parsed. Only proceed if this file is from a source you "
+        "trust (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).",
+        stacklevel=3,
+    )
+
+
 class BundleWorkflow(ABC):
     """
     Base class for the workflow specification in bundle, it can be a training, evaluation or inference workflow.
@@ -55,6 +73,10 @@ class BundleWorkflow(ABC):
         meta_file: filepath of the metadata file, if this is a list of file paths, their contents will be merged in order.
         logging_file: config file for `logging` module in the program. for more details:
             https://docs.python.org/3/library/logging.config.html#logging.config.fileConfig.
+            Security note: `fileConfig` passes the INI's `class=` and `args=` fields to Python
+            `eval()`, so this file runs as code and applying it raises a warning -- once per call
+            site, as Python's default warning filter suppresses repeats
+            (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
 
     """
 
@@ -72,6 +94,7 @@ def __init__(
             if not os.path.isfile(logging_file):
                 raise FileNotFoundError(f"Cannot find the logging config file: {logging_file}.")
             logger.info(f"Setting logging properties based on config: {logging_file}.")
+            _warn_logging_file_execution(logging_file)
             fileConfig(logging_file, disable_existing_loggers=False)
 
         if meta_file is not None:
@@ -273,6 +296,10 @@ class PythonicWorkflow(BundleWorkflow):
         meta_file: filepath of the metadata file, if this is a list of file paths, their contents will be merged in order.
         logging_file: config file for `logging` module in the program. for more details:
             https://docs.python.org/3/library/logging.config.html#logging.config.fileConfig.
+            Security note: `fileConfig` passes the INI's `class=` and `args=` fields to Python
+            `eval()`, so this file runs as code and applying it raises a warning -- once per call
+            site, as Python's default warning filter suppresses repeats
+            (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
 
     """
 
@@ -375,6 +402,10 @@ class ConfigWorkflow(BundleWorkflow):
             https://docs.python.org/3/library/logging.config.html#logging.config.fileConfig.
             If None, default to "configs/logging.conf", which is commonly used for bundles in MONAI model zoo.
             If False, the logging logic for the bundle will not be modified.
+            Security note: `fileConfig` passes the INI's `class=` and `args=` fields to Python
+            `eval()`, so this file runs as code and applying it raises a warning -- once per call
+            site, as Python's default warning filter suppresses repeats
+            (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
         init_id: ID name of the expected config expression to initialize before running, default to "initialize".
             allow a config to have no `initialize` logic and the ID.
         run_id: ID name of the expected config expression to run, default to "run".
@@ -444,6 +475,7 @@ def __init__(
                 else:
                     raise FileNotFoundError(f"Cannot find the logging config file: {logging_file}.")
             else:
+                _warn_logging_file_execution(str(logging_file))
                 fileConfig(str(logging_file), disable_existing_loggers=False)
                 logger.info(f"Setting logging properties based on config: {logging_file}.")
 

monai/fl/client/monai_algo.py

@@ -13,6 +13,7 @@
 
 import os
 import time
+import warnings
 from collections.abc import Mapping, MutableMapping
 from typing import Any, cast
 
@@ -34,6 +35,26 @@
 logger = get_logger(__name__)
 
 
+def _warn_provisioned_config_execution(bundle_root: str) -> None:
+    """
+    Warn that the bundle under ``bundle_root`` is about to be executed.
+
+    In federated learning the whole app directory -- configs included -- is provisioned by the FL
+    system, and the aggregation server dispatches `initialize`/`train` tasks that the client runs
+    on its own, so there is no per-round human interaction to catch a poisoned config.
+    """
+    warnings.warn(
+        f"executing the bundle config under {bundle_root}, which is provisioned by the FL system: "
+        '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()`. A malicious or '
+        "compromised aggregation server therefore gets code execution on this client, without any "
+        "per-round human interaction. Only join a federation whose server and app-provisioning "
+        "channel you trust (see "
+        "https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-x6pr-233j-x5cw).",
+        stacklevel=3,
+    )
+
+
 def convert_global_weights(global_weights: Mapping, local_var_dict: MutableMapping) -> tuple[MutableMapping, int]:
     """Helper function to convert global weights to local weights format"""
     # Before loading weights, tensors might need to be reshaped to support HE for secure aggregation.
@@ -86,6 +107,15 @@ class MonaiAlgoStats(ClientAlgoStats):
     """
     Implementation of ``ClientAlgoStats`` to allow federated learning with MONAI bundle configurations.
 
+    Security note: the bundle under `bundle_root` is provisioned by the FL system -- `initialize()`
+    resolves it against `extra[ExtraItems.APP_ROOT]`, which the aggregation server supplies -- and
+    executing it runs whatever its config contains: any `"_target_"` value is resolved to an
+    importable callable and invoked with no allow list, and any `"$"`-prefixed value is passed to
+    Python `eval()`. A malicious or compromised server therefore gets code execution on this client,
+    with no per-round human interaction. Executing a config raises a warning -- once per call site,
+    as Python's default warning filter suppresses repeats
+    (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-x6pr-233j-x5cw).
+
     Args:
         bundle_root: directory path of the bundle.
         config_train_filename: bundle training config path relative to bundle_root. Can be a list of files;
@@ -135,18 +165,29 @@ def initialize(self, extra=None):
         Args:
             extra: Dict with additional information that should be provided by FL system,
                 i.e., `ExtraItems.CLIENT_NAME`, `ExtraItems.APP_ROOT` and `ExtraItems.LOGGING_FILE`.
-                You can diable the logging logic in the monai bundle by setting {ExtraItems.LOGGING_FILE} to False.
+                `{ExtraItems.LOGGING_FILE}` defaults to False here, and an explicit `None` is
+                treated the same way, so the bundle's own "configs/logging.conf" is not applied:
+                it is provisioned by the FL system and `logging.config.fileConfig` runs the INI's
+                `class=`/`args=` fields through `eval()`
+                (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
+                Set it to a logging config file path to opt back in to configuring logging.
 
         """
         if extra is None:
             extra = {}
         self.client_name = extra.get(ExtraItems.CLIENT_NAME, "noname")
-        logging_file = extra.get(ExtraItems.LOGGING_FILE, None)
+        logging_file = extra.get(ExtraItems.LOGGING_FILE, False)
+        if logging_file is None:
+            # `ConfigWorkflow` reads `None` as "fall back to the bundle's own configs/logging.conf",
+            # the FL-provisioned file this default exists to keep away from `fileConfig`. Passing the
+            # key explicitly as `None` has to mean the same as leaving it out.
+            logging_file = False
         self.logger.info(f"Initializing {self.client_name} ...")
 
         # FL platform needs to provide filepath to configuration files
         self.app_root = extra.get(ExtraItems.APP_ROOT, "")
         self.bundle_root = os.path.join(self.app_root, self.bundle_root)
+        _warn_provisioned_config_execution(self.bundle_root)
 
         if self.workflow is None:
             config_train_files = self._add_config_files(self.config_train_filename)
@@ -313,6 +354,15 @@ class MonaiAlgo(ClientAlgo, MonaiAlgoStats):
     """
     Implementation of ``ClientAlgo`` to allow federated learning with MONAI bundle configurations.
 
+    Security note: the bundle under `bundle_root` is provisioned by the FL system -- `initialize()`
+    resolves it against `extra[ExtraItems.APP_ROOT]`, which the aggregation server supplies -- and
+    executing it runs whatever its config contains: any `"_target_"` value is resolved to an
+    importable callable and invoked with no allow list, and any `"$"`-prefixed value is passed to
+    Python `eval()`. A malicious or compromised server therefore gets code execution on this client,
+    with no per-round human interaction. Executing a config raises a warning -- once per call site,
+    as Python's default warning filter suppresses repeats
+    (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-x6pr-233j-x5cw).
+
     Args:
         bundle_root: directory path of the bundle.
         local_epochs: number of local epochs to execute during each round of local training; defaults to 1.
@@ -416,19 +466,30 @@ def initialize(self, extra=None):
         Args:
             extra: Dict with additional information that should be provided by FL system,
                 i.e., `ExtraItems.CLIENT_NAME`, `ExtraItems.APP_ROOT` and `ExtraItems.LOGGING_FILE`.
-                You can diable the logging logic in the monai bundle by setting {ExtraItems.LOGGING_FILE} to False.
+                `{ExtraItems.LOGGING_FILE}` defaults to False here, and an explicit `None` is
+                treated the same way, so the bundle's own "configs/logging.conf" is not applied:
+                it is provisioned by the FL system and `logging.config.fileConfig` runs the INI's
+                `class=`/`args=` fields through `eval()`
+                (see https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-wvpx-5qmp-46g3).
+                Set it to a logging config file path to opt back in to configuring logging.
 
         """
         self._set_cuda_device()
         if extra is None:
             extra = {}
         self.client_name = extra.get(ExtraItems.CLIENT_NAME, "noname")
-        logging_file = extra.get(ExtraItems.LOGGING_FILE, None)
+        logging_file = extra.get(ExtraItems.LOGGING_FILE, False)
+        if logging_file is None:
+            # `ConfigWorkflow` reads `None` as "fall back to the bundle's own configs/logging.conf",
+            # the FL-provisioned file this default exists to keep away from `fileConfig`. Passing the
+            # key explicitly as `None` has to mean the same as leaving it out.
+            logging_file = False
         timestamp = time.strftime("%Y%m%d_%H%M%S")
         self.logger.info(f"Initializing {self.client_name} ...")
         # FL platform needs to provide filepath to configuration files
         self.app_root = extra.get(ExtraItems.APP_ROOT, "")
         self.bundle_root = os.path.join(self.app_root, self.bundle_root)
+        _warn_provisioned_config_execution(self.bundle_root)
 
         if self.train_workflow is None and self.config_train_filename is not None:
             config_train_files = self._add_config_files(self.config_train_filename)

tests/bundle/test_bundle_workflow.py

@@ -11,11 +11,14 @@
 
 from __future__ import annotations
 
+import json
+import logging
 import os
 import shutil
 import sys
 import tempfile
 import unittest
+import warnings
 from copy import deepcopy
 from pathlib import Path
 
@@ -268,5 +271,84 @@ def test_create_pythonic_workflow(self):
         workflow.finalize()
 
 
+class TestConfigWorkflowWarnsOnLoggingConf(unittest.TestCase):
+    """Regression test for GHSA-wvpx-5qmp-46g3: `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. It fires in `__init__`, before `initialize()` or `run()`, and
+    lives in a plain INI rather than the MONAI `$`-DSL, so it is easy to miss when reviewing a
+    bundle. Applying it is still not blocked -- as for GHSA-873f-pvrv-4x83, MONAI has no way to
+    establish whether a bundle is trustworthy -- but applying it now raises a `UserWarning`."""
+
+    def setUp(self):
+        # `fileConfig` reconfigures logging process-wide. Snapshot the root logger and restore it
+        # afterwards so these tests cannot leak a handler into the rest of the suite.
+        root = logging.getLogger()
+        level, handlers, filters = root.level, root.handlers[:], root.filters[:]
+        disabled = logging.root.manager.disable
+
+        def _restore():
+            # Detach whatever is on the root logger now, closing anything `fileConfig` installed so
+            # it does not linger in logging's handler registry, then put the snapshot back. Under
+            # `tests/runner.py` the root logger starts with no handlers, so there is nothing for
+            # `fileConfig` to have closed on the way in.
+            for handler in root.handlers[:]:
+                root.removeHandler(handler)
+                if handler not in handlers:
+                    handler.close()
+            root.setLevel(level)
+            root.filters[:] = filters
+            for handler in handlers:
+                root.addHandler(handler)
+            logging.disable(disabled)
+
+        self.addCleanup(_restore)
+
+    def test_default_logging_conf_warns_and_executes(self):
+        with tempfile.TemporaryDirectory() as tempdir:
+            configs = os.path.join(tempdir, "configs")
+            os.makedirs(configs)
+            marker = os.path.join(tempdir, "PWNED")
+            with open(os.path.join(configs, "train.json"), "w") as f:
+                json.dump({"initialize": []}, f)
+            # `fileConfig` eval()s the `class=` field, so the tuple subscript runs the payload and
+            # still yields a usable handler class.
+            with open(os.path.join(configs, "logging.conf"), "w") as f:
+                f.write(
+                    "[loggers]\nkeys=root\n[handlers]\nkeys=h\n[formatters]\nkeys=f\n"
+                    "[logger_root]\nlevel=NOTSET\nhandlers=h\n"
+                    "[handler_h]\n"
+                    f"class=(__import__('pathlib').Path({marker!r}).write_text('pwned'), "
+                    "__import__('logging').StreamHandler)[1]\nargs=()\nformatter=f\n"
+                    "[formatter_f]\nformat=%(message)s\n"
+                )
+            with self.assertWarnsRegex(UserWarning, r"GHSA-wvpx-5qmp-46g3"):
+                ConfigWorkflow(config_file=os.path.join(configs, "train.json"), workflow_type="train")
+            self.assertTrue(os.path.exists(marker))
+
+    def test_no_warning_when_logging_disabled(self):
+        """No warning when `fileConfig` is never reached -- the file exists but is opted out of."""
+        with tempfile.TemporaryDirectory() as tempdir:
+            configs = os.path.join(tempdir, "configs")
+            os.makedirs(configs)
+            marker = os.path.join(tempdir, "PWNED")
+            with open(os.path.join(configs, "train.json"), "w") as f:
+                json.dump({"initialize": []}, f)
+            with open(os.path.join(configs, "logging.conf"), "w") as f:
+                f.write(
+                    "[loggers]\nkeys=root\n[handlers]\nkeys=h\n[formatters]\nkeys=f\n"
+                    "[logger_root]\nlevel=NOTSET\nhandlers=h\n"
+                    "[handler_h]\n"
+                    f"class=(__import__('pathlib').Path({marker!r}).write_text('pwned'), "
+                    "__import__('logging').StreamHandler)[1]\nargs=()\nformatter=f\n"
+                    "[formatter_f]\nformat=%(message)s\n"
+                )
+            with warnings.catch_warnings():
+                warnings.simplefilter("error", UserWarning)
+                ConfigWorkflow(
+                    config_file=os.path.join(configs, "train.json"), workflow_type="train", logging_file=False
+                )
+            self.assertFalse(os.path.exists(marker))
+
+
 if __name__ == "__main__":
     unittest.main()