Repository navigation
Fix math_animate scan bypass via re-exported modules (fixes #864) - #865
Open
adeeluddin01 wants to merge 1 commit into
Open
adeeluddin01 wants to merge 1 commit into
adeeluddin01 wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #864. The
math_animatesafety scan checked import roots against adenylist, but did not account for standard-library modules that re-export a
blocked module as an attribute.
import zipfileis not blocked, andzipfile.osis the realosmodule, so a scene could reach host commandexecution while the scan reported zero violations — on the default path,
without
allow_unsafe_code.What changed (
tools/graphics/math_animate.py)The scan (
_scan_scene_code) gains three rules. All existingdenylist / blocked-name / dunder rules and their messages are unchanged.
access whose name is itself a blocked module, rooted on a name bound by
an import, is flagged — e.g.
zipfile.os,tarfile.os, chained forms,aliased imports (
import zipfile as z; z.os), and rebinding forms(
x = zipfile; x.os). Taint from imports propagates through plainrebinding assignments so rebinding does not hide the module.
from zipfile import os.manimare violations — e.g.from zipfile import *(which would otherwise bind unknown names,possibly a blocked module, without any import node naming them).
from manim import *stays allowed: it is how scenes get Manim's API.A small helper,
_attr_root_name, resolves the root name of an attributechain for rule 1.
Tests (
tests/tools/test_math_animate_safety.py)7 new regression tests for #864:
test_blocks_os_via_reexported_module(zipfile.os, top level)test_blocks_os_via_reexport_inside_construct(tarfile.os)test_blocks_aliased_reexport(import zipfile as z)test_blocks_reexport_via_rebinding(x = zipfile; x.os)test_blocks_from_import_of_blocked_name(from zipfile import os)test_blocks_star_import_from_non_manim(from zipfile import *)test_safe_scene_attribute_access_still_passesFull suite run locally against the fixed module: 27 passed
(20 existing + 7 new). The existing safe scene (
manim+numpy+math, usingnp.pi/math.tau) still passes, so legitimate scenes areunaffected — no breaking change.
Scope notes (honest)
os, allowing host command execution on the default path #864 re-export class. The static scan remainsdefense-in-depth, not a sandbox (as the file header states); a full
sandbox for Manim execution is separate future work.
Code(code_file=...)file-read observations notedin math_animate safety scan bypass: unblocked stdlib module re-exports
os, allowing host command execution on the default path #864 are not addressed here and remain open observations.Manim/LaTeX on the analysis machine); verification is at the scan level
plus the repo's test suite.