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

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,75 @@ 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():
+            root.setLevel(level)
+            root.handlers[:] = handlers
+            root.filters[:] = filters
+            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()