From f265afde5d06748b703259a9f6524746f9a7921d Mon Sep 17 00:00:00 2001 From: 0xDevNinja Date: Thu, 2 Jul 2026 11:54:58 +0530 Subject: [PATCH] fix(math_animate): harden scan against no-import builtins/reflection bypass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scan only flagged dangerous builtins as direct call targets (ast.Name func) and dunders as attribute access, so it missed indirection like `__builtins__['open']('.env').read()` and `getattr(o, '__class__')` — the default path still executed secret-reading code. Block dangerous identifiers wherever they appear as a bare name (open, eval, exec, compile, __import__, __builtins__, getattr/setattr/delattr, globals/ locals/vars) rather than only as a call target, and extend the blocked dunder set (__class__, __dict__, __getattribute__, __reduce__, ...). This closes the reported no-import bypass while genuine math scenes still pass. Still defense-in-depth, not a full sandbox; the allow_unsafe_code opt-out and explicit code-execution contract remain. A subprocess-level sandbox is the right follow-up for complete isolation. Refs #219 --- tests/tools/test_math_animate_safety.py | 40 ++++++++++++++++++++++++- tools/graphics/math_animate.py | 23 +++++++++----- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/tests/tools/test_math_animate_safety.py b/tests/tools/test_math_animate_safety.py index dafc487d..a83ced7f 100644 --- a/tests/tools/test_math_animate_safety.py +++ b/tests/tools/test_math_animate_safety.py @@ -54,7 +54,45 @@ def test_blocks_dangerous_calls(call): " def construct(self):\n" f" {call}('x')\n" ) - assert f"call to '{call}()'" in MathAnimate._scan_scene_code(code) + assert f"use of '{call}'" in MathAnimate._scan_scene_code(code) + + +def test_blocks_no_import_builtins_secret_read(): + # Regression for the reported bypass: no dangerous import, secret read via + # __builtins__ indexing. The whole expression roots on the bare __builtins__ + # name (the 'open' inside [] is a string literal), so blocking that name + # blocks the payload. + code = ( + "from manim import *\n" + "class S(Scene):\n" + " def construct(self):\n" + " __builtins__['open']('.env').read()\n" + ) + assert "use of '__builtins__'" in MathAnimate._scan_scene_code(code) + + +def test_blocks_getattr_reflection_bypass(): + # getattr-based attribute reflection is a classic denylist evasion; blocking + # the getattr name removes the primitive. + code = ( + "from manim import *\n" + "class S(Scene):\n" + " def construct(self):\n" + " cls = getattr(object(), '__class__')\n" + ) + assert "use of 'getattr'" in MathAnimate._scan_scene_code(code) + + +def test_blocks_aliased_dangerous_builtin(): + # Binding a blocked builtin to another name must still trip on the name use. + code = ( + "from manim import *\n" + "class S(Scene):\n" + " def construct(self):\n" + " f = open\n" + " f('.env')\n" + ) + assert "use of 'open'" in MathAnimate._scan_scene_code(code) def test_blocks_sandbox_escape_dunders(): diff --git a/tools/graphics/math_animate.py b/tools/graphics/math_animate.py index 1caf808e..63be79b7 100644 --- a/tools/graphics/math_animate.py +++ b/tools/graphics/math_animate.py @@ -45,12 +45,20 @@ _BLOCKED_IMPORTS = frozenset({ "importlib", "builtins", "multiprocessing", "threading", "pty", "glob", "resource", "signal", "tempfile", "webbrowser", "pathlib", }) -_BLOCKED_CALLS = frozenset({ +# Dangerous identifiers blocked wherever they appear as a bare name — not just +# as a direct call. This catches indirection like `__builtins__['open']`, +# `f = open`, or `getattr(x, '__class__')` that a call-target-only or +# attribute-only check would miss. +_BLOCKED_NAMES = frozenset({ "eval", "exec", "compile", "__import__", "open", "input", "breakpoint", + "__builtins__", "__loader__", "globals", "locals", "vars", + "getattr", "setattr", "delattr", }) +# Sandbox-escape / reflection dunders blocked as attribute access. _BLOCKED_ATTRS = frozenset({ - "__globals__", "__builtins__", "__subclasses__", "__bases__", "__mro__", - "__code__", + "__globals__", "__builtins__", "__subclasses__", "__bases__", "__base__", + "__mro__", "__code__", "__class__", "__dict__", "__getattribute__", + "__closure__", "__reduce__", "__reduce_ex__", "__subclasshook__", }) @@ -230,10 +238,11 @@ class MathAnimate(BaseTool): root = (node.module or "").split(".")[0] if root in _BLOCKED_IMPORTS: violations.append(f"from '{node.module}' import ...") - elif isinstance(node, ast.Call): - fn = node.func - if isinstance(fn, ast.Name) and fn.id in _BLOCKED_CALLS: - violations.append(f"call to '{fn.id}()'") + elif isinstance(node, ast.Name): + # Blocks direct calls (eval(...)) and indirection alike: + # `__builtins__['open']`, `f = open`, `getattr(o, '__class__')`. + if node.id in _BLOCKED_NAMES: + violations.append(f"use of '{node.id}'") elif isinstance(node, ast.Attribute): if node.attr in _BLOCKED_ATTRS: violations.append(f"attribute access '.{node.attr}'")