From e6cb9faacead1c61238b5b988cae7b6c3c4cd6e0 Mon Sep 17 00:00:00 2001 From: Sp1kyss <90422804+Sp1kyss@users.noreply.github.com> Date: Mon, 11 May 2026 05:46:27 +0200 Subject: [PATCH] fix: close two security analyzer bypass paths in sandbox executor (#14690) ## Summary Two bypass vectors in the sandbox code security analyzer allowed malicious code to pass the safety check undetected and reach the Docker executor. ### 1. JavaScript: template-literal bypass of `require()` block The `SecureJavaScriptAnalyzer` regex patterns used `['"]` to match module names, covering only single and double quotes. An attacker could use ES6 template literals to bypass all three `require` checks: `javascript const cp = require(`child_process`); async function main() { return cp.execSync('cat /etc/passwd').toString(); } ` The same bypass applied to `fs` and `worker_threads`. **Fix:** Updated all three `require` patterns from `['"]` to `['"\]` to also match backtick template literals. ### 2. Python: `builtins` not blocked + attribute-call blind spot in `visit_Call` `visit_Call` only checked `ast.Name` nodes, so attribute-style calls like `module.func()` were invisible to the analyzer. Additionally, `builtins` was absent from `DANGEROUS_IMPORTS`. Combined, this allowed: `python import builtins def main(): builtins.exec('import os; os.system("id")') ` Neither the import nor the exec call triggered any flag. **Fix:** Added `builtins` to `DANGEROUS_IMPORTS` and added an `ast.Attribute` branch to `visit_Call` so that `module.dangerous_func()` style calls are caught alongside bare `dangerous_func()` calls. ## Tests Added four regression tests covering each new bypass vector: - `test_javascript_child_process_template_literal_is_rejected` - `test_javascript_fs_template_literal_is_rejected` - `test_python_builtins_import_is_rejected` - `test_python_attribute_eval_call_is_rejected` --------- Co-authored-by: bounty-hunter --- .../executor_manager/services/security.py | 18 +++++-- agent/sandbox/tests/test_security.py | 54 +++++++++++++++++++ 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/agent/sandbox/executor_manager/services/security.py b/agent/sandbox/executor_manager/services/security.py index 13a02ced2e..f0323e747a 100644 --- a/agent/sandbox/executor_manager/services/security.py +++ b/agent/sandbox/executor_manager/services/security.py @@ -26,7 +26,7 @@ class SecurePythonAnalyzer(ast.NodeVisitor): An AST-based analyzer for detecting unsafe Python code patterns. """ - DANGEROUS_IMPORTS = {"os", "subprocess", "sys", "shutil", "socket", "ctypes", "pickle", "threading", "multiprocessing", "asyncio", "http.client", "ftplib", "telnetlib"} + DANGEROUS_IMPORTS = {"os", "subprocess", "sys", "shutil", "socket", "ctypes", "pickle", "threading", "multiprocessing", "asyncio", "http.client", "ftplib", "telnetlib", "builtins"} DANGEROUS_CALLS = { "eval", @@ -77,6 +77,16 @@ class SecurePythonAnalyzer(ast.NodeVisitor): """Check for dangerous function calls.""" if isinstance(node.func, ast.Name) and node.func.id in self.DANGEROUS_CALLS: self.unsafe_items.append((f"Call: {node.func.id}", node.lineno)) + elif isinstance(node.func, ast.Attribute) and node.func.attr in self.DANGEROUS_CALLS: + # Surface the attribute-style match in the analyzer log so that + # incident response can grep for it just like the other unsafe-item + # findings; the bare append is invisible to operators. + logger.warning( + "[SafeCheck] Attribute-style dangerous call detected: %s (line %s)", + node.func.attr, + node.lineno, + ) + self.unsafe_items.append((f"Call: {node.func.attr}", node.lineno)) self.generic_visit(node) def visit_Attribute(self, node: ast.Attribute): @@ -154,9 +164,9 @@ class SecurePythonAnalyzer(ast.NodeVisitor): class SecureJavaScriptAnalyzer: DANGEROUS_PATTERNS = [ - (re.compile(r"""require\s*\(\s*['"]child_process['"]\s*\)"""), "Require: child_process"), - (re.compile(r"""require\s*\(\s*['"]fs['"]\s*\)"""), "Require: fs"), - (re.compile(r"""require\s*\(\s*['"]worker_threads['"]\s*\)"""), "Require: worker_threads"), + (re.compile(r"""require\s*\(\s*['"`]child_process['"`]\s*\)"""), "Require: child_process"), + (re.compile(r"""require\s*\(\s*['"`]fs['"`]\s*\)"""), "Require: fs"), + (re.compile(r"""require\s*\(\s*['"`]worker_threads['"`]\s*\)"""), "Require: worker_threads"), (re.compile(r"""\beval\s*\("""), "Call: eval"), (re.compile(r"""\bFunction\s*\("""), "Call: Function"), (re.compile(r"""\bprocess\s*\.\s*binding\s*\("""), "Call: process.binding"), diff --git a/agent/sandbox/tests/test_security.py b/agent/sandbox/tests/test_security.py index ed096894e4..dc8d9f8063 100644 --- a/agent/sandbox/tests/test_security.py +++ b/agent/sandbox/tests/test_security.py @@ -45,6 +45,60 @@ def test_javascript_eval_is_rejected(): assert any("eval" in issue.lower() for issue, _ in issues) +def test_javascript_child_process_template_literal_is_rejected(): + """Template literal backticks bypass single/double-quote regex patterns.""" + is_safe, issues = analyze_code_security( + "const cp = require(`child_process`); async function main() { return 'ok'; }", + SupportLanguage.NODEJS, + ) + + assert is_safe is False + assert any("child_process" in issue for issue, _ in issues) + + +def test_javascript_fs_template_literal_is_rejected(): + is_safe, issues = analyze_code_security( + "const fs = require(`fs`); async function main() { return fs.readFileSync('/etc/passwd', 'utf8'); }", + SupportLanguage.NODEJS, + ) + + assert is_safe is False + assert any("fs" in issue for issue, _ in issues) + + +def test_python_builtins_import_is_rejected(): + """builtins module gives access to eval/exec and must be blocked.""" + is_safe, issues = analyze_code_security( + "import builtins\ndef main():\n builtins.eval('1+1')", + SupportLanguage.PYTHON, + ) + + assert is_safe is False + # Pin the specific reason: rejection must come from the new ``builtins`` + # entry in ``DANGEROUS_IMPORTS``, not from some unrelated parse error. + assert any("builtins" in issue for issue, _ in issues), ( + f"expected an issue mentioning 'builtins', got {issues!r}" + ) + + +def test_python_attribute_eval_call_is_rejected(): + """Attribute-style dangerous calls (builtins.eval) must be caught.""" + is_safe, issues = analyze_code_security( + "import builtins\ndef main():\n builtins.exec('import os')", + SupportLanguage.PYTHON, + ) + + assert is_safe is False + # Pin the specific reason: rejection must come from the new + # ``ast.Attribute`` branch in ``visit_Call`` flagging the ``exec`` call, + # not from the ``import builtins`` line above. We assert ``exec`` is in at + # least one finding so the test fails if visit_Call's attribute branch is + # ever reverted. + assert any("exec" in issue for issue, _ in issues), ( + f"expected an issue mentioning 'exec', got {issues!r}" + ) + + def test_javascript_safe_code_still_passes(): is_safe, issues = analyze_code_security( "async function main(args) { return { answer: args.value ?? null }; }",