From 339a7e86e6d6c548619ec51715a347aae1d3afb2 Mon Sep 17 00:00:00 2001 From: rubenuni1009 <183279777+rubenG1009@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:35:49 +0200 Subject: [PATCH 1/2] fix(utils): confine safe_eval name resolution to caller-supplied variables `safe_eval` passed `None` as the globals mapping whenever `globals_vars` was empty, so `eval` fell back to this module's own globals, and Python inserted the real `__builtins__` into whichever mapping it received. Expressions could therefore resolve names the caller never supplied: safe_eval("np") -> safe_eval("ast") -> safe_eval("int") -> safe_eval("__builtins__") -> the real builtins mapping The default `SAFE_TYPES` allow-list keeps this from being exploitable today, since attribute access, subscripting and calls are all rejected. But `allowed_types` is a documented parameter, and the existing `int.__class__.__init__.__globals__` test case shows the escape chain is only one widening away. Removing the objects from scope is stronger than relying on the allow-list alone to keep them out of reach. Names now resolve only against `globals_vars` and `locals_vars`; anything else raises `NameError`. A caller that wants builtins can still pass its own `__builtins__` through `globals_vars`. `_get_fake_spatial_shape` is the only caller in the tree and is unaffected: it passes `p` and `n` explicitly, and `rewrite_np=True` keeps supplying `np` through `locals_vars`. Assisted-by: Claude Code Co-Authored-By: Claude Opus 5 Signed-off-by: rubenuni1009 <183279777+rubenG1009@users.noreply.github.com> --- monai/utils/safeeval.py | 17 +++++++++++++-- tests/utils/test_safe_eval.py | 41 ++++++++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/monai/utils/safeeval.py b/monai/utils/safeeval.py index e8f0bcb1ae..dc19b943b2 100644 --- a/monai/utils/safeeval.py +++ b/monai/utils/safeeval.py @@ -71,6 +71,12 @@ def safe_eval( expressions with constants and names can be evaluated, so excludes attribute access, indexing, and calls. Code injection is infeasible through such expressions, so this is a safe and secure way of evaluating simple expressions. + Names are resolved only against `globals_vars` and `locals_vars`. The builtins are not placed in scope and the + globals of this module are never used as a fallback, so an expression naming anything the caller did not supply + raises a `NameError` instead of silently resolving to a builtin or to an imported module. This matters when + `allowed_types` is widened beyond `SAFE_TYPES`, since node types such as `ast.Attribute` and `ast.Call` would + otherwise make those objects reachable. + If `rewrite_np` is True, int and float constants in the given expression will be wrapped with Numpy types as given by `int_type_str` and `float_type_str`. These are expected to be constructor names prefixed with `np.` as Numpy will be present in the expression global variables under that name. The values can be changed to other types if @@ -79,7 +85,8 @@ def safe_eval( Args: expr: expression to evaluate, this will be stripped before parsing to avoid indentation complaints - globals_vars: global variable mapping, this will be treated as read-only for this function, unlike `eval` + globals_vars: global variable mapping, this will be treated as read-only for this function, unlike `eval`. + An empty `__builtins__` is added to the copy unless the mapping provides its own value for that key locals_vars: local variable mapping allowed_types: sequence of allowed AST types which can be found in `expr` when parsed rewrite_np: if True, wrap int or float literals in Numpy types @@ -88,6 +95,7 @@ def safe_eval( Raises: ValueError: raised when any node in the AST parsed from `expr` has a type not in `allowed_types` + NameError: raised when `expr` names a variable not present in `globals_vars` or `locals_vars` Returns: The evaluated expression value, using `eval` with `globals_vars` and `locals_vars` @@ -105,4 +113,9 @@ def safe_eval( ast.fix_missing_locations(parsed) locals_vars = {**(locals_vars or {}), "np": np} - return eval(compile(parsed, "", "eval"), dict(globals_vars) if globals_vars else None, locals_vars) + # an explicit globals mapping stops `eval` from falling back to this module's globals, and an empty + # `__builtins__` keeps the builtins out of scope, so only names the caller supplied can be resolved + eval_globals: dict[str, Any] = dict(globals_vars) if globals_vars else {} + eval_globals.setdefault("__builtins__", {}) + + return eval(compile(parsed, "", "eval"), eval_globals, locals_vars) diff --git a/tests/utils/test_safe_eval.py b/tests/utils/test_safe_eval.py index d578ced9ac..31eecae64f 100644 --- a/tests/utils/test_safe_eval.py +++ b/tests/utils/test_safe_eval.py @@ -17,7 +17,7 @@ import numpy as np from parameterized import parameterized -from monai.utils import safe_eval +from monai.utils import SAFE_TYPES, safe_eval GOOD_EXPRS = [ ("1+2", None, None, 3), @@ -86,6 +86,45 @@ def test_rewrite_np_preserves_bool(self): result = safe_eval("False", rewrite_np=True) self.assertIs(result, False) + def test_builtins_not_in_scope(self): + """Test that builtins cannot be reached as bare names.""" + for expr in ("int", "type", "object", "abs"): + with self.subTest(expr=expr), self.assertRaises(NameError): + safe_eval(expr) + + # `__builtins__` itself still names the mapping `eval` looks names up in, but it is now empty + self.assertEqual(safe_eval("__builtins__"), {}) + + def test_module_globals_not_in_scope(self): + """Test that the names imported by the safeeval module itself cannot be reached.""" + for expr in ("np", "ast", "safe_eval", "SAFE_TYPES"): + with self.subTest(expr=expr), self.assertRaises(NameError): + safe_eval(expr) + + def test_unknown_name_raises(self): + """Test that a name the caller did not supply raises NameError rather than resolving elsewhere.""" + with self.assertRaises(NameError): + safe_eval("x+1", {"y": 2}) + + self.assertEqual(safe_eval("x+1", {"x": 2}), 3) + + def test_widened_allowed_types_cannot_escape(self): + """Test that attribute access and calls stay harmless once `allowed_types` is widened.""" + allowed = (*SAFE_TYPES, ast.Attribute, ast.Call, ast.Subscript) + + for expr in ("int.__class__.__init__.__globals__", "np.ndarray", "safe_eval('1')"): + with self.subTest(expr=expr), self.assertRaises(NameError): + safe_eval(expr, allowed_types=allowed) + + # the builtins mapping is reachable by name but holds nothing to escape with + with self.assertRaises(KeyError): + safe_eval("__builtins__['__import__']", allowed_types=allowed) + + def test_globals_can_opt_out_of_empty_builtins(self): + """Test that a caller supplying its own `__builtins__` keeps control of the namespace.""" + result = safe_eval("abs", {"__builtins__": {"abs": abs}}) + self.assertIs(result, abs) + def test_rewrite_np_inf_constant(self): """Test that rewrite_np handles overflowing infinity literals.""" result = safe_eval("1e309", rewrite_np=True) From c941a284ee83204b8ac6f9c27b38230e091538b9 Mon Sep 17 00:00:00 2001 From: rubenuni1009 <183279777+rubenG1009@users.noreply.github.com> Date: Mon, 21 Sep 2026 17:37:37 +0200 Subject: [PATCH 2/2] docs(utils): state safe_eval's two implicit names precisely The name-resolution paragraph claimed every unsupplied name raises `NameError`, which overstates it in two ways review caught: - `__builtins__` resolves, to the empty mapping this change installs - `np` resolves when `rewrite_np=True`, since the rewritten constants are calls into it Both are intended and both were already pinned by the new tests, so this corrects the wording and the `Raises:` entry rather than the behaviour, and adds the `rewrite_np` case to `test_module_globals_not_in_scope`. Assisted-by: Claude Code Co-Authored-By: Claude Opus 5 Signed-off-by: rubenuni1009 <183279777+rubenG1009@users.noreply.github.com> --- monai/utils/safeeval.py | 12 +++++++----- tests/utils/test_safe_eval.py | 3 +++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/monai/utils/safeeval.py b/monai/utils/safeeval.py index dc19b943b2..e79ae7683e 100644 --- a/monai/utils/safeeval.py +++ b/monai/utils/safeeval.py @@ -71,11 +71,12 @@ def safe_eval( expressions with constants and names can be evaluated, so excludes attribute access, indexing, and calls. Code injection is infeasible through such expressions, so this is a safe and secure way of evaluating simple expressions. - Names are resolved only against `globals_vars` and `locals_vars`. The builtins are not placed in scope and the - globals of this module are never used as a fallback, so an expression naming anything the caller did not supply - raises a `NameError` instead of silently resolving to a builtin or to an imported module. This matters when + Names are resolved only against `globals_vars` and `locals_vars`, so an expression naming anything the caller did + not supply raises a `NameError` rather than silently resolving to a builtin or to a module imported here. Two + names resolve without being supplied: `__builtins__`, which is bound to an empty mapping so that nothing can be + reached through it, and `np`, which `rewrite_np` adds because the rewritten constants need it. This matters when `allowed_types` is widened beyond `SAFE_TYPES`, since node types such as `ast.Attribute` and `ast.Call` would - otherwise make those objects reachable. + otherwise make the builtins and this module's imports reachable. If `rewrite_np` is True, int and float constants in the given expression will be wrapped with Numpy types as given by `int_type_str` and `float_type_str`. These are expected to be constructor names prefixed with `np.` as Numpy @@ -95,7 +96,8 @@ def safe_eval( Raises: ValueError: raised when any node in the AST parsed from `expr` has a type not in `allowed_types` - NameError: raised when `expr` names a variable not present in `globals_vars` or `locals_vars` + NameError: raised when `expr` names a variable not present in `globals_vars` or `locals_vars`, other than + `__builtins__` and, when `rewrite_np` is True, `np` Returns: The evaluated expression value, using `eval` with `globals_vars` and `locals_vars` diff --git a/tests/utils/test_safe_eval.py b/tests/utils/test_safe_eval.py index 31eecae64f..669613e5b1 100644 --- a/tests/utils/test_safe_eval.py +++ b/tests/utils/test_safe_eval.py @@ -101,6 +101,9 @@ def test_module_globals_not_in_scope(self): with self.subTest(expr=expr), self.assertRaises(NameError): safe_eval(expr) + # `rewrite_np` supplies `np` itself, since the constants it rewrites are calls into it + self.assertIs(safe_eval("np", rewrite_np=True), np) + def test_unknown_name_raises(self): """Test that a name the caller did not supply raises NameError rather than resolving elsewhere.""" with self.assertRaises(NameError):