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#L207-L216), [crates/orchestrator-core/src/client.rs:240-249](../crates/orchestrator-core/src/client.rs#L240-L249)
- **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/](../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](../crates/executor-core/src/engine.rs#L317-L320)
- **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](../crates/orchestrator-core/src/api.rs#L753), error construction in [crates/executor-core/src/engine.rs:683-689](../crates/executor-core/src/engine.rs#L683-L689) (`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: ... <internal path>" → file paths.
  - "execution task panicked: <Rust backtrace fragment>" 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#L182-L188), [crates/executor-core/src/engine.rs:195-205](../crates/executor-core/src/engine.rs#L195-L205)
- **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](../crates/executor-core/src/sandbox.rs#L46-L58)
- **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](../crates/executor-core/src/sdk/invoke.rs#L119-L217)
- **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](../crates/executor-core/src/sdk/stdlib/json.rs#L17-L23)
- **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](../crates/executor-core/src/sdk/stdlib/regex.rs#L39-L51)
- **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](../crates/orchestrator-core/src/client.rs#L211-L213), 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](../crates/executor-core/src/sdk/stdlib/random.rs#L25-L35).

### 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](../crates/executor-core/src/engine.rs#L182-L188).

### 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](../crates/executor-core/src/engine.rs#L304-L332).

### F-SE-L-05 — `parse_target` in [crates/executor-core/src/sdk/invoke.rs:221-242](../crates/executor-core/src/sdk/invoke.rs#L221-L242) 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](../crates/executor-core/src/engine.rs#L213)) — 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](../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](../crates/orchestrator-core/src/client.rs#L184-L192) 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`).