fix(guest): exec timeout watcher must SIGKILL, not SIGALRM by GHSA-xjhv-pp2r-6f82 (#543)

* fix(guest): exec timeout watcher must SIGKILL, not SIGALRM

The timeout watcher in src/guest/src/service/exec/timeout.rs sent
Signal::SIGALRM (signal 14, catchable) when an exec exceeded its
deadline. Any workload that installed `signal.signal(SIGALRM, SIG_IGN)`
in Python or `trap '' ALRM` in shell absorbed the signal and ran to
completion past the configured timeout, returning exit_code=0 —
bypassing the deadline entirely.

The inline comment already said "Kill process with SIGKILL"; only the
code passed the wrong Signal. Use Signal::SIGKILL (signal 9,
uncatchable) so the watcher guarantees termination regardless of
in-process signal handlers.

An audit of catchable-signal usage across the codebase found no other
instances of this bug class. All graceful-shutdown paths
(container/lifecycle.rs, exec/registry.rs, vmm/controller/shim.rs,
runtime/rt_impl.rs, cli/serve/mod.rs) already escalate SIGTERM → wait →
SIGKILL correctly.

Two regression tests guard the fix:

- sdks/python/tests/test_exec_timeout_sigalrm.py — Python pytest port
  of the reporter PoC: workload installs SIG_IGN for SIGALRM, sleeps 8s
  with timeout=3s. Asserts result.exit_code != 0 AND elapsed <
  TIMEOUT_S + 2.0.
- src/boxlite/tests/exec_options.rs::test_timeout_kills_sigalrm_ignoring_process
  — canonical Rust integration test (counterpart to
  security_enforcement.rs). Shell workload uses `trap '' ALRM; sleep 15`
  with timeout=2s; same assertion shape.

Verified via controlled experiment (reverting timeout.rs to SIGALRM):
both new tests FAIL in the bug state with rich diagnostics
(`exit_code=0`, elapsed near WORKLOAD_S), both PASS with the fix.
Three independent control tests (test_api_key_credential_get_token,
test_oci_symlink_escape_blocked, test_timeout_kills_long_command) PASS
in both states — confirming the fix is precisely scoped and the new
tests aren't trivially-passing or trivially-failing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(guest): two-stage SIGTERM/grace/SIGKILL on exec timeout

Replaces the single-step SIGKILL with the SIGTERM → grace → SIGKILL
pattern used elsewhere in the codebase (ExecRegistry::shutdown_all,
Container::shutdown, vmm/controller/shim.rs, runtime/rt_impl.rs).

Why two stages:

- SIGKILL alone is correct but harsh — cooperative workloads have no
  chance to flush buffers, close files, or commit transactions. Sandbox
  consumers (AI agents running short-lived scripts) usually want both
  "let me clean up" AND "hard deadline still enforced".
- Industry default (Docker --stop-timeout, K8s terminationGracePeriod,
  systemd TimeoutStopSec) is SIGTERM-first with SIGKILL fallback. Aligns
  with user expectations.
- The watcher now logs whether grace was respected (info: exited cleanly)
  or escalation was required (warn: workload absorbed SIGTERM).

The hard-deadline guarantee from the previous commit is preserved: any
workload that ignores SIGTERM via SIG_IGN / signal trap is still killed
by SIGKILL after the 2-second grace expires.

Stage-3 regression test:

sdks/python/tests/test_exec_timeout_sigalrm.py gains
test_exec_timeout_sigkill_fallback_when_sigterm_ignored. The workload
installs SIG_IGN for both SIGTERM and SIGALRM (every catchable signal
the watcher might fire in stages 1+2), so only the uncatchable SIGKILL
escalation after grace can stop it. Asserts exit_code != 0 AND
elapsed < TIMEOUT_S + GRACE_S + 2.0.

This complements the existing test_exec_timeout_kills_sigalrm_ignoring_process
(which exercises stage-1 SIGTERM against a workload that only traps
SIGALRM). Together the two tests cover all three stages of the watcher.

Verified via controlled experiments (logs in ../log/):

- 01-baseline-sigkill-*.log  — single-step SIGKILL baseline, 5/5 PASS
- 02-two-stage-*.log         — after refactor, 5/5 PASS (no regression
                                in existing tests)
- 03-stage3-test-*.log       — with new stage-3 test, 6/6 PASS
- 04-control-sigterm-only-*.log — timeout.rs reverted to SIGTERM-only
                                (no SIGKILL fallback); only the new
                                stage-3 test FAILS with diagnostic
                                "exec returned exit_code=0 after 8.08s",
                                proving the test catches a genuine
                                regression and is not always-pass

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Gamnaam Song <gamnaamsong@Airbus380-841-Main-Controller.local>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: 3 people

sdks/python/tests/test_exec_timeout_sigalrm.py

@@ -0,0 +1,147 @@
+"""Regression test for exec timeout bypass via SIGALRM.
+
+``box.exec(timeout=N)`` must terminate any process whose runtime exceeds N,
+including one that installs ``signal.signal(SIGALRM, SIG_IGN)``. The guest's
+timeout watcher must use SIGKILL (signal 9, uncatchable). If it sends SIGALRM
+(signal 14, catchable/ignorable) the workload absorbs the signal, runs to
+completion, and ``ExecResult.exit_code`` comes back as 0 — the deadline is
+bypassed.
+
+The Python SDK does NOT raise ``boxlite.TimeoutError`` on exec timeout; it
+returns an ``ExecResult`` whose ``exit_code`` reflects how the process died
+(non-zero / signal). The PoC at ``boxlite_poc/poc_sigalrm_bypass.py``
+confirms this: it inspects ``exit_code`` and elapsed wall-time, never an
+exception.
+
+Requirements:
+  - make dev:python  (build the Python SDK with native extension)
+  - VM runtime for integration tests (libkrun + Hypervisor.framework)
+"""
+
+from __future__ import annotations
+
+import time
+
+import pytest
+
+import boxlite
+
+pytestmark = [pytest.mark.integration, pytest.mark.asyncio]
+
+# Workload: install SIG_IGN for SIGALRM, then sleep WORKLOAD_S seconds.
+# If the timeout watcher fires SIGALRM, the workload absorbs it and the
+# loop completes (exit_code=0). If it fires SIGKILL, the process dies
+# mid-loop (exit_code != 0, elapsed ~ TIMEOUT_S).
+IGNORE_SIGALRM = """
+import sys, time, signal
+seconds = int(sys.argv[1]) if len(sys.argv) > 1 else 8
+signal.signal(signal.SIGALRM, signal.SIG_IGN)
+for _ in range(seconds):
+    time.sleep(1)
+"""
+
+# Stage-3 workload: ignore every catchable signal the watcher might send
+# in stages 1/2 (SIGTERM is the cooperative ask; SIGALRM is the historical
+# bug we already fixed). Only SIGKILL — which is uncatchable — can stop
+# this process. Used to verify the SIGKILL fallback after the grace window.
+IGNORE_TERM_AND_ALRM = """
+import sys, time, signal
+seconds = int(sys.argv[1]) if len(sys.argv) > 1 else 8
+signal.signal(signal.SIGTERM, signal.SIG_IGN)
+signal.signal(signal.SIGALRM, signal.SIG_IGN)
+for _ in range(seconds):
+    time.sleep(1)
+"""
+
+TIMEOUT_S = 3.0
+WORKLOAD_S = 8
+
+# Mirrors TIMEOUT_GRACE in src/guest/src/service/exec/timeout.rs.
+# Keep in sync if the guest grace constant changes.
+GRACE_S = 2.0
+
+
+async def test_exec_timeout_kills_sigalrm_ignoring_process():
+    """A workload that ignores SIGALRM must still be killed at the timeout.
+
+    Two-pronged assertion — both must hold:
+
+    1) ``exit_code != 0`` — the workload must NOT have completed normally.
+       With the SIGALRM bug, SIG_IGN absorbs the timeout signal, the loop
+       finishes, and Python exits cleanly with 0.
+
+    2) ``elapsed < TIMEOUT_S + 2.0`` — termination must happen near the
+       configured deadline, not at the workload's natural end. Even if
+       ``exit_code`` happens to be non-zero for some other reason, a long
+       elapsed time means the watcher did not fire promptly.
+
+    Fix is in ``src/guest/src/service/exec/timeout.rs``: send ``SIGKILL``
+    (uncatchable) rather than ``SIGALRM``.
+    """
+    async with boxlite.SimpleBox(image="python:3-alpine") as box:
+        t0 = time.time()
+        result = await box.exec(
+            "python3",
+            "-c",
+            IGNORE_SIGALRM,
+            str(WORKLOAD_S),
+            timeout=TIMEOUT_S,
+        )
+        elapsed = time.time() - t0
+
+        assert result.exit_code != 0, (
+            f"exec returned exit_code=0 after {elapsed:.2f}s — the {WORKLOAD_S}s "
+            f"workload completed normally despite timeout={TIMEOUT_S}s. The "
+            f"guest's timeout watcher is sending a catchable signal that the "
+            f"workload absorbs via SIG_IGN; the kill must use SIGKILL."
+        )
+        assert elapsed < TIMEOUT_S + 2.0, (
+            f"exec returned after {elapsed:.2f}s with exit_code={result.exit_code} "
+            f"— expected termination near {TIMEOUT_S}s. The timeout watcher "
+            f"is not killing the process promptly."
+        )
+
+
+async def test_exec_timeout_sigkill_fallback_when_sigterm_ignored():
+    """Stage-3 SIGKILL must terminate a workload that ignores SIGTERM.
+
+    Companion to ``test_exec_timeout_kills_sigalrm_ignoring_process``: that
+    test exercises stage-1 (cooperative SIGTERM kills the workload because
+    it only traps SIGALRM). This test traps both SIGTERM and SIGALRM, so
+    the watcher's stages 1+2 are absorbed and only the uncatchable SIGKILL
+    after the grace window can stop the process.
+
+    A regression that drops stage-3 — e.g., a single-stage watcher that
+    sends only SIGTERM and never escalates — would let this workload run
+    to natural completion (exit_code=0 after ~WORKLOAD_S).
+
+    Expected timing with the two-stage watcher:
+      - t = TIMEOUT_S:           SIGTERM sent, absorbed by SIG_IGN
+      - t = TIMEOUT_S + GRACE_S: SIGKILL sent, process dies (exit_code=-9)
+    """
+    async with boxlite.SimpleBox(image="python:3-alpine") as box:
+        t0 = time.time()
+        result = await box.exec(
+            "python3",
+            "-c",
+            IGNORE_TERM_AND_ALRM,
+            str(WORKLOAD_S),
+            timeout=TIMEOUT_S,
+        )
+        elapsed = time.time() - t0
+
+        assert result.exit_code != 0, (
+            f"stage-3 SIGKILL fallback broken: exec returned exit_code=0 "
+            f"after {elapsed:.2f}s. The workload ignores SIGTERM AND was not "
+            f"SIGKILL'd by the watcher — either grace expired without sending "
+            f"SIGKILL, or stage-3 escalation was removed from "
+            f"src/guest/src/service/exec/timeout.rs."
+        )
+        # Expected ~ TIMEOUT_S + GRACE_S = 5.0s. Allow +2.0s headroom for VM
+        # / SDK overhead. If elapsed approaches WORKLOAD_S, the watcher is
+        # not enforcing the deadline at all.
+        assert elapsed < TIMEOUT_S + GRACE_S + 2.0, (
+            f"stage-3 SIGKILL fallback fired too late: elapsed={elapsed:.2f}s "
+            f"(expected ~{TIMEOUT_S + GRACE_S:.1f}s, workload was {WORKLOAD_S}s) "
+            f"— grace period drift, or the watcher is not killing promptly."
+        )

src/boxlite/tests/exec_options.rs

@@ -93,6 +93,51 @@ async fn test_timeout_kills_long_command() {
     tb.teardown().await;
 }
 
+/// Regression test for exec timeout bypass via SIGALRM.
+///
+/// Companion to the Python-SDK PoC at
+/// `sdks/python/tests/test_exec_timeout_sigalrm.py`. The guest's timeout
+/// watcher must use SIGKILL (uncatchable). If it sends SIGALRM (catchable),
+/// the workload below — a shell that installs `trap '' ALRM` and then
+/// sleeps for 15 seconds — absorbs the signal, the underlying `sleep`
+/// runs to its natural end, and exec returns `exit_code=0` after ~15s,
+/// bypassing the 2-second deadline.
+///
+/// The fix lives in `src/guest/src/service/exec/timeout.rs`.
+#[tokio::test]
+async fn test_timeout_kills_sigalrm_ignoring_process() {
+    let tb = TestBox::new().await;
+
+    let start = std::time::Instant::now();
+    let execution = tb
+        .handle
+        .exec(
+            BoxCommand::new("sh")
+                .args(["-c", "trap '' ALRM; sleep 15"])
+                .timeout(Duration::from_secs(2)),
+        )
+        .await
+        .expect("exec failed");
+
+    let result = execution.wait().await.expect("wait failed");
+    let elapsed = start.elapsed();
+
+    assert_ne!(
+        result.exit_code, 0,
+        "timeout bypass: shell exited with exit_code=0 after {elapsed:?} \
+         despite timeout=2s — the guest is sending a catchable signal that \
+         the shell absorbs via `trap '' ALRM`; the kill must use SIGKILL"
+    );
+    assert!(
+        elapsed < Duration::from_secs(8),
+        "timeout did not curtail the workload: elapsed={elapsed:?} \
+         (expected near 2s, workload was 15s) — the watcher is not killing \
+         the process promptly"
+    );
+
+    tb.teardown().await;
+}
+
 /// Combine working_dir and user in a single command.
 #[tokio::test]
 async fn test_working_dir_with_user() {

src/guest/src/service/exec/timeout.rs

@@ -1,15 +1,28 @@
 //! Timeout management.
 //!
-//! Kills process if execution exceeds timeout duration.
+//! Two-stage termination when execution exceeds its deadline:
+//! SIGTERM first (cooperative cleanup), then SIGKILL after a grace
+//! period to enforce the hard deadline against workloads that ignore
+//! or trap SIGTERM.
 
 use crate::service::exec::state::ExecutionState;
 use std::time::Duration;
-use tracing::info;
+use tracing::{info, warn};
+
+/// Grace period between SIGTERM and SIGKILL on exec timeout.
+///
+/// Short enough that sandboxed execs still see a near-deadline kill,
+/// long enough that cooperative workloads can flush buffers, close
+/// files, and exit cleanly. Mirrors the SIGTERM→wait→SIGKILL pattern
+/// used by `ExecRegistry::shutdown_all` and `Container::shutdown`.
+const TIMEOUT_GRACE: Duration = Duration::from_secs(2);
 
 /// Start timeout watcher.
 ///
-/// After duration elapses, marks execution as timed out and kills
-/// the process if it's still running.
+/// After `timeout` elapses, sends SIGTERM and waits up to `TIMEOUT_GRACE`
+/// for the process to exit, then escalates to SIGKILL. SIGKILL is
+/// uncatchable, so a workload that installs `SIG_IGN`/handlers for
+/// SIGTERM (or SIGALRM, etc.) cannot outlive its deadline.
 pub(super) fn start_timeout_watcher(
     exec_state: ExecutionState,
     exec_id: String,
@@ -18,10 +31,33 @@ pub(super) fn start_timeout_watcher(
     tokio::spawn(async move {
         tokio::time::sleep(timeout).await;
 
-        // Kill process with SIGKILL
         use nix::sys::signal::Signal;
-        if exec_state.kill(Signal::SIGALRM).await {
-            info!(execution_id = %exec_id, "killed on timeout");
+
+        // Stage 1: SIGTERM — polite termination request.
+        if !exec_state.kill(Signal::SIGTERM).await {
+            // Process already exited on its own; nothing more to do.
+            return;
+        }
+        info!(
+            execution_id = %exec_id,
+            grace_ms = TIMEOUT_GRACE.as_millis() as u64,
+            "SIGTERM on timeout; grace before SIGKILL"
+        );
+
+        // Stage 2: wait for the grace window. `exec_state.kill` already
+        // returns false once the process is reaped, so we do not need to
+        // poll — we just sleep the full grace then ask for SIGKILL.
+        tokio::time::sleep(TIMEOUT_GRACE).await;
+
+        // Stage 3: SIGKILL fallback. Returns false if SIGTERM was honored
+        // during the grace window (clean exit, no escalation needed).
+        if exec_state.kill(Signal::SIGKILL).await {
+            warn!(
+                execution_id = %exec_id,
+                "SIGKILL after grace expired; workload did not exit on SIGTERM"
+            );
+        } else {
+            info!(execution_id = %exec_id, "exited within grace after SIGTERM");
         }
     });
 }