diff --git a/Cargo.lock b/Cargo.lock index 23460e4e2..292d5ff3d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -481,6 +481,8 @@ dependencies = [ "regex", "regex-automata", "reqwest", + "ruff_python_ast", + "ruff_python_parser", "russh", "rustls", "ryu", diff --git a/crates/bashkit/Cargo.toml b/crates/bashkit/Cargo.toml index 3a6870e05..51e56f850 100644 --- a/crates/bashkit/Cargo.toml +++ b/crates/bashkit/Cargo.toml @@ -154,6 +154,11 @@ monty-types = { version = "=0.0.19", optional = true } # breaks the `GetSize` derive (#2381). Dependabot ignores it for that reason; # keep the ignore in .github/dependabot.yml in step with this pin. get-size2 = { version = "=0.10.1", optional = true } +# Same parser Monty uses (already in the tree through monty =0.0.19). Bashkit +# parses user code with it to find `import` statements; see +# `is_disabled_import` in builtins/python.rs. Keep in step with Monty's pin. +ruff_python_parser = { version = "=0.0.3", optional = true } +ruff_python_ast = { version = "=0.0.3", optional = true } # Embedded TypeScript interpreter (optional) @@ -261,7 +266,13 @@ git = [] ssh = ["russh"] # Enable python/python3 builtins via embedded Monty interpreter # Usage: cargo build --features python -python = ["dep:monty", "dep:monty-types", "dep:get-size2"] +python = [ + "dep:monty", + "dep:monty-types", + "dep:get-size2", + "dep:ruff_python_parser", + "dep:ruff_python_ast", +] # Enable python/python3 builtins via real CPython 3.14 in a WebAssembly sandbox # Usage: cargo build --features cpython cpython = ["dep:bashkit-cpython-wasm", "dep:wasmtime"] diff --git a/crates/bashkit/src/builtins/python.rs b/crates/bashkit/src/builtins/python.rs index 3f6050ea5..cc41eeacd 100644 --- a/crates/bashkit/src/builtins/python.rs +++ b/crates/bashkit/src/builtins/python.rs @@ -346,66 +346,53 @@ impl Default for Python { } } +// Important security decision: find imports by parsing, not by scanning lines. +// A line-prefix scan missed `pass; import re`, `if x: import re` and any other +// statement that puts the import after a prefix. Monty 0.0.19 has no import +// hook, so bashkit parses with the same parser Monty runs first +// (`ruff_python_parser::parse_module`) and walks every statement body. +// Monty has no `__import__` or `importlib`, so only `import` / `from` statements +// can load a module. Code that fails to parse is let through: Monty parses it +// the same way, reports the SyntaxError and runs nothing. fn is_disabled_import(code: &str) -> Option<&'static str> { - let is_disabled_module = |module: &str| { - DISABLED_STDLIB_MODULES.iter().any(|disabled| { - module == *disabled - || (module.starts_with(disabled) - && module - .as_bytes() - .get(disabled.len()) - .is_some_and(|next| *next == b'.')) + use ruff_python_ast::statement_visitor::{StatementVisitor, walk_stmt}; + use ruff_python_ast::{Stmt, StmtImport, StmtImportFrom}; + + fn disabled(module: &str) -> Option<&'static str> { + DISABLED_STDLIB_MODULES.iter().copied().find(|disabled| { + module + .strip_prefix(disabled) + .is_some_and(|rest| rest.is_empty() || rest.starts_with('.')) }) - }; + } - for raw_line in code.lines() { - let line = raw_line.split('#').next().unwrap_or("").trim(); - if line.is_empty() { - continue; - } + struct ImportFinder(Option<&'static str>); - if let Some(imports) = line.strip_prefix("import ") { - for import in imports.split(',') { - let module = import - .split_whitespace() - .next() - .unwrap_or("") - .trim() - .trim_matches(|c: char| c == '\'' || c == '"') - .trim_end_matches(|c: char| { - !(c.is_ascii_alphanumeric() || c == '_' || c == '.') - }) - .to_string(); - if is_disabled_module(&module) { - return Some("re"); - } + impl<'a> StatementVisitor<'a> for ImportFinder { + fn visit_stmt(&mut self, stmt: &'a Stmt) { + if self.0.is_some() { + return; } - } - - if let Some(from_stmt) = line.strip_prefix("from ") { - let module = from_stmt - .split_whitespace() - .next() - .unwrap_or("") - .trim() - .trim_matches(|c: char| c == '\'' || c == '"') - .trim_end_matches(|c: char| !(c.is_ascii_alphanumeric() || c == '_' || c == '.')) - .to_string(); - if is_disabled_module(&module) { - return Some("re"); + match stmt { + Stmt::Import(StmtImport { names, .. }) => { + self.0 = names.iter().find_map(|alias| disabled(alias.name.as_str())); + } + Stmt::ImportFrom(StmtImportFrom { module, names, .. }) => { + self.0 = match module { + Some(module) => disabled(module.as_str()), + // `from . import re` + None => names.iter().find_map(|alias| disabled(alias.name.as_str())), + }; + } + _ => walk_stmt(self, stmt), } } - - if line.contains("__import__('re'") || line.contains("__import__(\"re\"") { - return Some("re"); - } - if line.contains("importlib.import_module('re'") - || line.contains("importlib.import_module(\"re\"") - { - return Some("re"); - } } - None + + let parsed = ruff_python_parser::parse_module(code).ok()?; + let mut finder = ImportFinder(None); + finder.visit_body(&parsed.syntax().body); + finder.0 } #[async_trait] @@ -1913,10 +1900,61 @@ mod tests { } #[tokio::test] - async fn test_re_module_dynamic_import_is_blocked() { - let r = run(&["-c", "m = __import__('re')"], None).await; + async fn test_re_module_dynamic_import_is_unavailable() { + // Monty has no `__import__` or `importlib`, so import statements are + // the only way to load a module. Pin that: if Monty adds either, the + // AST gate in `is_disabled_import` must learn about it. + let r = run(&["-c", "m = __import__('re')\nprint(m)"], None).await; + assert_eq!(r.exit_code, 1, "{}", r.stdout); + assert!(r.stdout.is_empty(), "{}", r.stdout); + let r = run(&["-c", "import importlib"], None).await; assert_eq!(r.exit_code, 1); - assert!(r.stderr.contains("importing module 're' is disabled")); + assert!( + r.stderr.contains("No module named 'importlib'"), + "{}", + r.stderr + ); + } + + #[tokio::test] + async fn test_re_module_hidden_imports_are_blocked() { + // Regression: the gate used to match only lines starting with + // `import`/`from`, so any statement prefix bypassed it. + for code in [ + "pass; import re", + "x = 1; from re import split", + "if True: import re", + "if True:\n import re", + "def f():\n import re\n return re\nf()", + "try:\n import re\nexcept Exception:\n pass", + "for _ in [1]: import json, re", + "with open('/tmp/x', 'w') as f: import re.sub", + "import json as j, re as r", + "class C:\n import re", + ] { + let r = run(&["-c", code], None).await; + assert_eq!(r.exit_code, 1, "{code}: {}", r.stderr); + assert!( + r.stderr.contains("importing module 're' is disabled"), + "{code}: {}", + r.stderr + ); + } + } + + #[tokio::test] + async fn test_re_named_text_is_not_an_import() { + // Strings, comments and other modules that merely mention `re` run. + let r = run( + &[ + "-c", + "import json\nre = 'import re'\n# import re\nprint(re, json.dumps('re'))", + ], + None, + ) + .await; + assert_eq!(r.exit_code, 0, "{}", r.stderr); + assert_eq!(r.stdout, "import re \"re\"\n"); } #[tokio::test] diff --git a/crates/bashkit/tests/integration/python_security_tests.rs b/crates/bashkit/tests/integration/python_security_tests.rs index 65145eb16..b9ca2789c 100644 --- a/crates/bashkit/tests/integration/python_security_tests.rs +++ b/crates/bashkit/tests/integration/python_security_tests.rs @@ -925,6 +925,24 @@ mod blackbox_dos { // (this test itself has tokio timeout protection) } + #[tokio::test] + async fn regex_module_cannot_be_imported_behind_a_statement_prefix() { + let mut bash = bash_python_limits(PythonLimits::default()); + for code in [ + "pass; import re", + "if True: import re", + "def f():\n import re\nf()", + ] { + let r = bash + .exec(&format!("python3 - <<'EOF'\n{code}\nprint('loaded')\nEOF")) + .await + .unwrap(); + assert_eq!(r.exit_code, 1, "{code}: {}", r.stdout); + assert!(r.stdout.is_empty(), "{code}: {}", r.stdout); + assert!(r.stderr.contains("importing module 're' is disabled")); + } + } + #[tokio::test] async fn math_factorial_bomb() { let mut bash = bash_python_tight(); diff --git a/knowledge/log.md b/knowledge/log.md index 63224f415..25097a030 100644 --- a/knowledge/log.md +++ b/knowledge/log.md @@ -1,5 +1,9 @@ # Bashkit Knowledge Update Log +## 2026-11-05 + +* **Python `re` gate parses code**: the embedded Monty `python3` blocked `import re` by matching lines that start with `import`/`from`, so `pass; import re` or `if True: import re` loaded the module anyway (found by a downstream integration review). The gate now walks the parsed statement tree. See [Python Builtin](runtimes/python-builtin.md). + ## 2026-11-03 * **Walk order**: the default `walk` visits directories in path order. It used `read_dir` order (hash order for `InMemoryFs`), so a batch cut short by `max_entries` prefetched listings `tree` never printed and `tree_prefetch_stays_within_entry_budget` read 8-15 directories run to run. See [Virtual Filesystem](foundations/vfs.md#remote-backends-round-trips). diff --git a/knowledge/runtimes/python-builtin.md b/knowledge/runtimes/python-builtin.md index 2310c69e0..ecb7455a3 100644 --- a/knowledge/runtimes/python-builtin.md +++ b/knowledge/runtimes/python-builtin.md @@ -229,7 +229,13 @@ I/O, no network/classes/third-party imports) through the general `Builtin::llm_hint()` mechanism (hints deduplicated automatically). The regex module `re` is intentionally disabled in Bashkit due to -catastrophic-backtracking DoS risk in untrusted code execution. +catastrophic-backtracking DoS risk in untrusted code execution. The gate parses +the code with `ruff_python_parser` (the parser Monty itself runs first) and walks +every statement body, so `pass; import re`, `if x: import re` or an import inside +a function, class, loop or `try` is caught. Strings and comments that mention +`re` are not. A line-prefix scan used before 2026-11-05 missed all of these. +Monty 0.0.19 has no `__import__` or `importlib`, so `import`/`from` statements +are the only module loaders; a unit test pins that. ### Integration with Bashkit