PiCloud/security_audit/05_sandbox_exec.md

Security Audit — Rhai Sandbox & Script Execution

Scope: the boundary between untrusted user Rhai scripts and the picloud host process. Engine setup, sandbox limit enforcement, wall-clock + memory budgets, SDK attack surface, info disclosure.

Reference HEAD: main @ v1.1.9, files under crates/executor-core/ + crates/orchestrator-core/{client.rs,gate.rs} + crates/manager-core/dispatcher.rs.

Header counts: Critical 0 / High 4 / Medium 6 / Low 5 / Info 4.

The sandbox is well-thought-out at the engine-config level (all six Rhai per-call ceilings set + admin clamping at write time + per-app module resolver with cycle and depth detection + no unsafe blocks). The defensive boundary is breached not by code-injection but by resource amplification: a single anonymous script call can wedge a blocking thread for the full 5-minute hard cap, propagate uncensored internal error strings to the HTTP response, and consume unbounded SDK service calls per execution. No sandbox-escape was found.

---

Critical

None.

The four set_max_* calls plus module_resolvers::DummyModuleResolver (before the per-call PicloudModuleResolver replaces it) plus disable_symbol("print") plus the absence of register_custom_syntax, file/process/network primitives (file/process are absent; network is gated by SSRF in manager-core/src/ssrf.rs) keep the door shut to direct native code execution. No unsafe blocks exist anywhere in executor-core or orchestrator-core.

---

High

F-SE-H-01 — tokio::time::timeout over JoinHandle does NOT cancel a spawn_blocking task; runaway script keeps its OS thread until self-completion

• Location: crates/orchestrator-core/src/client.rs:207-216, crates/orchestrator-core/src/client.rs:240-249
• Summary: The code path is let join = spawn_blocking(...); tokio::time::timeout(timeout, join).await. When the timeout fires the future is dropped — but dropping a JoinHandle does not cancel spawn_blocking (Tokio cannot interrupt a synchronous OS thread). The script keeps running until either the operation-budget exhausts (currently 1M ops, ~seconds on tight loops, but a script that calls into the SDK block_on will only count one op per service call and may sleep arbitrarily) or it returns voluntarily. The author acknowledges this in the comment at lines 184-192 but the consequence is understated: the _permit releases, so the gate admits a new request — the blocking thread is NOT released. Under load, every timeout-exceeded script subtracts one thread permanently from the Tokio blocking pool (default 512), but on a low-memory single-Pi deployment the OS will run out of stack first.
• Concrete amplification: any anonymous script can do loop { kv::get("x", "y"); } (each iteration: 1 Rhai op + ~1 ms block_on → after 1M ops, ~16 min of wall-clock have passed; HARD_TIMEOUT_CAP = 300s will fire long before the op budget) → the orchestrator returns Timeout after 300s, but the thread keeps running until the budget exhausts (potentially another 10+ minutes). N concurrent attackers = N stuck threads.
• Severity: High (single anonymous request can permanently occupy a worker thread; chained → denial-of-service).
• Fix:
  1. Move the wall-clock interrupt to inside the Rhai engine via Engine::on_progress (the closure is called for every op; check Instant::now() - started > soft_deadline and return Some(Dynamic::UNIT) to halt eval). Use the existing op counter as the carrier.
  2. Wrap the spawn_blocking in a worker that also receives a tokio::sync::watch cancellation channel; the on_progress closure polls it and aborts.
  3. As a defense-in-depth, plumb an OS rlimit/cgroup cap on per-thread CPU time (Linux RLIMIT_CPU per task) so a wedged thread eventually dies from SIGXCPU.

F-SE-H-02 — SDK service calls (KV/docs/pubsub/queue/secrets/files/users/email/http/invoke) are not rate-limited per execution; one invocation can issue unbounded I/O

• Location: crates/executor-core/src/sdk/ — every register* function. No counter, no cap. Each SDK call costs 1 Rhai op (the function-call op), so Limits::max_operations = 1_000_000 is the only ceiling, and block_on time isn't counted at all.
• Summary: A 5-line script for n in 0..1_000_000 { kv.get("x"); } makes a million round-trips against Postgres. AUDIT.md F-S-001 already catches the per-call size leak; this finding catches the per-execution count leak. The sister storage services (docs, files, secrets, queue, pubsub, users) all share the shape — none of them maintain a per-execution counter visible to the caller.
• Severity: High (one anonymous public-HTTP route can saturate the DB pool and amplify into N file-system writes for files::create, N SMTP relays for users::send_verification_email, N outbox rows for pubsub::publish_durable).
• Fix: Add a Counters struct in SdkCallCx (AtomicU32 per service category + per-call cap, e.g., 1024 KV ops, 64 pubsub publishes, 16 emails, 8 outbound HTTP). Increment+check at the top of each register_* closure; throw EvalAltResult::ErrorRuntime("kv: per-execution call cap (1024) exceeded") on overflow. Same place a per-execution Counters lives, so invoke() callees get their own counters (defense for re-entrant abuse) but the same global per-root-execution counter is bumped to defeat chained-invoke amplification.

F-SE-H-03 — print is disabled but debug is not; Rhai's debug() writes to stderr by default and the comment claims both are disabled

• Location: crates/executor-core/src/engine.rs:317-320
• Summary: The comment says "Rhai's built-in print and debug map to stdout/stderr by default; we never want scripts dumping there directly." But only engine.disable_symbol("print") is called. debug(value) (the Rhai built-in) still routes through whatever on_debug callback is installed; if none is installed, it goes to eprintln!. On a serverless host where stderr is captured to a journald/Docker log stream, an attacker can write arbitrary bytes — control characters, color escapes, ANSI cursor manipulation, long lines — into the host's stderr stream. This is an info-disclosure / log-injection vector that the codebase's author already noticed but didn't completely fix.
• Severity: High (any anonymous script can write arbitrary lines into the operator's terminal/logs; trivially escapes the operator's grep/filter).
• Fix: engine.disable_symbol("debug"); on the line below, OR install engine.on_debug(|_, _, _| {}) (no-op handler). Belt-and-suspenders: also disable_symbol("eval") to be explicit about it being unavailable.

F-SE-H-04 — ExecError::Runtime propagates verbatim Rhai/SDK error text to the public HTTP response body (info disclosure)

• Location: crates/orchestrator-core/src/api.rs:753, error construction in crates/executor-core/src/engine.rs:683-689 (map_eval_error falls through to ExecError::Runtime(other.to_string()) for every non-TooManyOperations non-ErrorParsing variant).
• Summary: Anything that throws inside the sandbox — kv: connection refused, http: blocked by SSRF policy: link-local, secrets: cannot decrypt with master key (key rotation in progress), invoke: depth limit exceeded, and any panic in a registered fn — surfaces as a 502 Bad Gateway with the full message in the JSON body, sent to whoever called the public route. A red-team attacker uses this as a reconnaissance oracle:
  • "blocked by SSRF policy: link-local" → confirms the host runs in a cloud environment with metadata service.
  • "kv: pool acquire timeout" → confirms DB pool exhaustion (lets the attacker time their attacks).
  • "secrets: ... " → file paths.
  • "execution task panicked: " if RUST_BACKTRACE=1 (line 211-213 — format!("execution task panicked: {join_err}") flows into Runtime, which renders directly in the JSON body).
  • The module resolver explicitly redacts this case for backend errors (module_resolver.rs:344-355 — "module backend unavailable; check server logs") — but the rest of the SDK does not.
• Severity: High (low-effort reconnaissance plus log-injection-by-script via throw "<crafted message>" — the thrown string is the runtime error string, which becomes the response body, reflecting attacker-controlled content into operator-facing JSON without a Content-Type check).
• Fix: In ApiError::into_response for unauthenticated requests, replace ExecError::Runtime(s) body with a stable generic message and log the original with tracing::error! at INFO/ERROR. Add a principal: Option<Principal> field to ApiError (carried from the request) and gate verbose error text on authentication. Mirror the module-resolver redaction.

---

Medium

F-SE-M-01 — No script-source byte cap; parser cost is unbounded at script-upload + per-Engine::execute (when AST cache misses)

• Location: crates/executor-core/src/engine.rs:182-188, crates/executor-core/src/engine.rs:195-205
• Summary: Engine::compile and Engine::execute(source, ...) accept arbitrary &str. Neither path checks source length. The admin script-upload endpoint may impose its own cap (not audited here — agent 4's scope), but LocalExecutorClient::execute (legacy non-identity path; line 207) does not. A 100 MB Rhai source parses for a long time and allocates a correspondingly huge AST. The max_expr_depth only catches deeply nested expressions, not wide ones.
• Severity: Medium (this needs an attacker to have script-write authz, so it's lower-impact, but it's a one-line script edit to crash the parser).
• Fix: Add const MAX_SOURCE_BYTES: usize = 256 * 1024; validated in Engine::compile / Engine::validate / Engine::execute; reject above with ExecError::Parse("script source exceeds 256 KiB"). Match the per-key value cap in PICLOUD_KV_MAX_VALUE_BYTES.

F-SE-M-02 — Limits does not include a max_modules cap; deep but valid import graphs allocate unbounded module instances

• Location: crates/executor-core/src/sandbox.rs:46-58
• Summary: module_import_depth_max = 8 caps the import chain depth, but a single module can import 1000 distinct other modules. Each one compiles + evaluates → 1000 Shared<Module> allocations + their AST + their Scope. The LRU module cache (DEFAULT_MODULE_CACHE_SIZE = 512) caps long-term memory but not the per-execution allocation pressure during a cold-cache scenario.
• Severity: Medium (requires the attacker to be a script author with write authz; matches F-SE-M-01's threat model).
• Fix: Add max_modules_per_execution: u32 (default 64) to Limits; track in the resolver state alongside depth, throw on overflow.

F-SE-M-03 — invoke() re-entry inherits caller's permits (no new gate acquire) and shares the caller's blocking thread; a depth-8 chain holds one slot for 8x as long

• Location: crates/executor-core/src/sdk/invoke.rs:119-217
• Summary: invoke_blocking calls self_engine.execute_ast(&ast, req) directly from inside the caller's spawn_blocking thread. Comment at line 120-122 explains this as a feature ("the outer execution is already gate-admitted, no new spawn_blocking — we'd deadlock if we nested inside the blocking pool"). Correct as written, but: each level of invoke() runs synchronously inside the same thread, accumulating the full call tree's CPU time against one slot. trigger_depth_max = 8, so a worst-case chain is 8 scripts deep, each doing 5 minutes of work → 40 minutes of one blocking thread held under one gate permit.
• Severity: Medium (the global thread budget is 32 by default; 8 attackers each calling a depth-8 chain saturates the gate for 40 min).
• Fix: Per-execution wall-clock budget that is inclusive of invoke() callees: pass Instant + deadline into SdkCallCx, check in invoke before each callee's execute_ast, throw DeadlineExceeded early. Combined with F-SE-H-01's on_progress hook this becomes airtight.

F-SE-M-04 — json::parse accepts arbitrary nesting; serde_json default recursion is unbounded → stack overflow

• Location: crates/executor-core/src/sdk/stdlib/json.rs:17-23
• Summary: serde_json::from_str recurses on nested objects/arrays. A {"a":{"a":{"a":...}}} with 10K levels stack-overflows the OS thread. This is spawn_blocking, so the stack size is whatever Tokio sets (default 2 MB) — 10-30k levels of object nesting hits it. Resulting abort is process-fatal for the whole picloud binary on a single-node deployment.
• Severity: Medium (process kill = service outage; needs a script with json::parse and untrusted input — which is the default for any public HTTP route reading ctx.request.body).
• Fix: Switch to serde_json::Deserializer::from_str(s).into_iter::<Value>() with a recursion-limiting read::SliceRead, OR re-use Rhai's max_expr_depth semantics by writing a small bounded-depth parser. The cheapest fix: catch with std::panic::catch_unwind (won't catch stack overflow, but catches most parser panics) and document the limitation.

F-SE-M-05 — regex::* accepts user-supplied patterns with no complexity / size cap; ReDoS DoS surface

• Location: crates/executor-core/src/sdk/stdlib/regex.rs:39-51
• Summary: The Rust regex crate is non-backtracking (no exponential ReDoS), but compilation is bounded only by Regex::DEFAULT_SIZE_LIMIT (10 MB), and the patterns are cached per-thread without an upper bound on individual pattern complexity. A long compile time per call multiplied by the LRU cache (capacity 128 per thread) means a script can fill the cache with maximum-complexity patterns and force the cache eviction churn to dominate. Plus: there's no input-size cap on text — regex::find("a", massive_string) is O(n) but n is uncapped.
• Severity: Medium (DoS via 100 ms-per-regex × 1M ops = 27 hours; but capped by max_operations, so realistic ceiling is "tens of seconds before op-budget throws").
• Fix: RegexBuilder::new(pattern).size_limit(64 * 1024).dfa_size_limit(256 * 1024) for tighter compile bounds, and add a text.len() <= MAX_REGEX_TEXT_BYTES (e.g., 1 MiB) guard in the call sites.

F-SE-M-06 — Stack-overflow / panic in a register_fn closure aborts the spawn_blocking thread; the join error message reaches the HTTP body as ExecError::Runtime("execution task panicked: ...")

• Location: crates/orchestrator-core/src/client.rs:211-213, same in line 244.
• Summary: If any Rhai-registered native function panics (e.g., a Dynamic::try_cast that the bridge code missed, an arithmetic overflow in a stdlib fn), Tokio's JoinHandle::await returns Err(JoinError). The current handling does format!("execution task panicked: {join_err}") and bubbles it into ExecError::Runtime — which (per F-SE-H-04) flows verbatim to the HTTP body.
• Severity: Medium (info disclosure; chains with F-SE-H-04).
• Fix: When JoinError::is_panic(), log the panic detail with tracing::error! and return a generic ExecError::Runtime("internal script execution error".into()). Bonus: install a std::panic::set_hook in the picloud binary's startup that captures the panic site + script_id so operators have a forensic trail.

---

Low

F-SE-L-01 — random::int(MIN, MAX) with min == MAX = i64::MAX panics inside gen_range (panics on empty range when low > high, but the code guards min > max, not the inclusive range overflow). Verify or add a (low_inclusive..=high_inclusive) guard.

• crates/executor-core/src/sdk/stdlib/random.rs:25-35.

F-SE-L-02 — time::add_seconds / diff_seconds return overflow as error message including raw inputs — fine for the values themselves, but a stylistic minor leak. No real fix needed.

F-SE-L-03 — Engine::compile (line 182-188) does NOT install the per-app PicloudModuleResolver — it uses DummyModuleResolver. That's correct for validate() (parse-only), but compile() ASTs are cached by LocalExecutorClient::get_or_compile and re-used by execute_ast. Since execute_ast installs the real resolver per-call, modules are resolved lazily at eval time — still correct, but worth a comment that compile() deliberately doesn't pre-resolve. crates/executor-core/src/engine.rs:182-188.

F-SE-L-04 — No engine.set_optimization_level(OptimizationLevel::None) — the default Simple runs constant-folding at parse time. Mostly desirable, but a script with let x = a * b * c * d * ... (long constant chain in a const-context) takes parse-time CPU proportional to the chain length. Mitigated by F-SE-M-01's source-size cap. crates/executor-core/src/engine.rs:304-332.

F-SE-L-05 — parse_target in crates/executor-core/src/sdk/invoke.rs:221-242 accepts any 36-char string as a UUID candidate. Not a security issue, but on a Name-style invoke a 36-char tenant name unexpectedly tries UUID parsing first → script-visible error surface that varies on input length. Document the heuristic or split into explicit constructors.

---

Info

F-SE-I-01 — Strong defenses in place

• All six Rhai set_max_* knobs configured (operations, string size, array size, map size, call levels, expr depths).
• disable_symbol("print") (debug not disabled — see F-SE-H-03).
• No unsafe blocks anywhere in executor-core or orchestrator-core.
• No register_custom_syntax — Rhai's parser shape is fixed.
• No file-system / process / network primitives registered (network goes through manager-core/src/ssrf.rs deny-list, agent 9's scope).
• The Engine struct holds an Arc<Engine> self-weak so invoke() re-uses the same Limits and Services; one script cannot mutate the engine config visible to the next.
• Per-call rhai::Engine instance is freshly built in execute_ast (engine.rs:213) — confirmed: no shared parser state between scripts.
• PicloudModuleResolver has cycle detection (in-progress stack) + acyclic-depth cap + per-app cache key + backend-error redaction.
• args_to_json in invoke rejects FnPtr closures crossing the invoke boundary.

F-SE-I-02 — Concurrency gate (ExecutionGate) defaults to 32, sized to deny pool starvation. Token returned on future drop, not blocking-thread completion (see F-SE-H-01 follow-on). crates/orchestrator-core/src/gate.rs.

F-SE-I-03 — Engine::compile accepts arbitrary source from LocalExecutorClient::execute(source, ...); the orchestrator path normally uses execute_with_identity (cached). The non-identity execute path is documented as "tests + fallback" — verify it isn't reachable on public routes, otherwise F-SE-M-01 becomes a public-data-plane DoS rather than admin-only.

F-SE-I-04 — Wall-clock interrupt design assumption — LocalExecutorClient::execute was written assuming spawn_blocking could be cancelled via JoinHandle drop; the code's own comment block at client.rs:184-192 admits this is wrong but accepts the trade-off. The trade-off was reasonable in v1.0 single-node MVP; at v1.1.9 with public-HTTP scripts running unauthenticated, it is the highest-impact unmitigated DoS surface in the engine.

---

Summary recommendation

If a single follow-up is funded out of this report, fix F-SE-H-01 — install engine.on_progress with a deadline check. The other three Highs (per-execution SDK-call cap, debug symbol, error-message scrubbing) are all small, well-scoped patches and should land together in the same hardening release. None of them require schema changes, and the test surface is well-defined (the executor-core test pattern already covers respects_operation_budget).