diff --git a/CHANGELOG.md b/CHANGELOG.md index 43a699c..2695644 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ All notable changes to ShellClaw are documented here. Format follows [Keep a Cha ## [Unreleased] ### Fixed +- Sandboxed shell commands no longer inherit the agent environment: the command process keeps only `PATH`, locale (`LANG`, `LC_ALL`, `LC_CTYPE`), `TERM`, `TZ`, and `HOME` (set to the workspace), so a prompt-injected `printenv` cannot read provider API keys or bot tokens. `LD_*`, `CUDA_*`, and proxy variables are dropped before `exec`; see the environment model in `docs/SECURITY.md` (#109). +- The shell command timeout is enforced against a monotonic wall clock on both the sandboxed and the unsandboxed paths, so a command that keeps stdout trickling can no longer outlive `shell_timeout_sec` and hold the agent lock until the output buffer fills (#116). - Telegram `getUpdates` requests a single update per poll and skips a message without a numeric sender id, so a backlog larger than the 256 KiB read buffer (or a channel/anonymous post) no longer pins the poll offset and stalls the inbox. - Telegram `sendMessage` delivery is verified from the HTTP status code and the `ok` body field, so an API refusal (HTTP 400/429) is no longer reported as a successful send and acknowledged as delivered. - Long Telegram and Discord replies are chunked to the platform limits (4096/2000 bytes, UTF-8 boundary-safe) through the shared `channel_send_chunked` helper, so a reply over the cap is delivered as several messages instead of failing the whole send; chunk posts are paced for the ~1 msg/s Telegram chat rate, and a failed mid-reply post logs its chunk index and byte offset. diff --git a/Makefile b/Makefile index 8262f86..762bfec 100644 --- a/Makefile +++ b/Makefile @@ -127,7 +127,12 @@ ASAP_INVOKE_TEST_O := $(BINDIR)/asap_invoke_test.o # Sandbox (Phase 3 §5) SANDBOX_CORE_O := src/sandbox/sandbox.o SANDBOX_LANDLOCK_O := src/sandbox/sandbox_landlock.o -SANDBOX_O := $(SANDBOX_CORE_O) $(SANDBOX_LANDLOCK_O) +SANDBOX_ENV_O := src/sandbox/sandbox_env.o +# Sandbox unit test object: exposes drain_pipe helpers (SHELLCLAW_SANDBOX_TEST) +# so coverage does not depend on namespace privileges the CI runners revoke. +# Lives under src/sandbox/ so the coverage.sh capture (--directory src) merges it. +SANDBOX_TEST_O := src/sandbox/sandbox_test.o +SANDBOX_O := $(SANDBOX_CORE_O) $(SANDBOX_LANDLOCK_O) $(SANDBOX_ENV_O) ALLOWLIST_SCAN_O := src/sandbox/allowlist.o ALLOWLIST_PATH_O := src/sandbox/allowlist_path.o ALLOWLIST_O := $(ALLOWLIST_SCAN_O) $(ALLOWLIST_PATH_O) @@ -385,9 +390,15 @@ $(SHELL_O): src/tools/shell.c src/tools/tool.h src/tools/shell.h src/core/config src/sandbox/sandbox.h src/sandbox/allowlist.h $(CC) $(CFLAGS) $(INC) -c -o $@ src/tools/shell.c -$(SANDBOX_CORE_O): src/sandbox/sandbox.c src/sandbox/sandbox.h src/sandbox/sandbox_landlock.h +$(SANDBOX_CORE_O): src/sandbox/sandbox.c src/sandbox/sandbox.h src/sandbox/sandbox_landlock.h src/sandbox/sandbox_env.h $(CC) $(CFLAGS) $(INC) -c -o $@ src/sandbox/sandbox.c +$(SANDBOX_ENV_O): src/sandbox/sandbox_env.c src/sandbox/sandbox_env.h + $(CC) $(CFLAGS) $(INC) -c -o $@ src/sandbox/sandbox_env.c + +$(SANDBOX_TEST_O): src/sandbox/sandbox.c src/sandbox/sandbox.h src/sandbox/sandbox_landlock.h src/sandbox/sandbox_env.h + $(CC) $(CFLAGS) $(INC) -DSHELLCLAW_SANDBOX_TEST -c -o $@ src/sandbox/sandbox.c + $(SANDBOX_LANDLOCK_O): src/sandbox/sandbox_landlock.c src/sandbox/sandbox_landlock.h $(CC) $(CFLAGS) $(INC) -c -o $@ src/sandbox/sandbox_landlock.c @@ -763,9 +774,9 @@ test_static: tests/test_static.c src/gateway/ui_assets.h src/gateway/static.o $(CC) $(CFLAGS) $(LDFLAGS) $(INC) -o $(BINDIR)/$@ tests/test_static.c src/gateway/static.o $(LDLIBS) $(DSYM_SCRIPT) -test_sandbox: tests/test_sandbox.c $(SANDBOX_O) +test_sandbox: tests/test_sandbox.c $(SANDBOX_TEST_O) $(SANDBOX_LANDLOCK_O) $(SANDBOX_ENV_O) @mkdir -p $(BINDIR) - $(CC) $(CFLAGS) $(LDFLAGS) $(INC) -o $(BINDIR)/$@ tests/test_sandbox.c $(SANDBOX_O) $(LDLIBS) + $(CC) $(CFLAGS) $(LDFLAGS) $(INC) -o $(BINDIR)/$@ tests/test_sandbox.c $(SANDBOX_TEST_O) $(SANDBOX_LANDLOCK_O) $(SANDBOX_ENV_O) $(LDLIBS) $(DSYM_SCRIPT) test_allowlist: tests/test_allowlist.c $(ALLOWLIST_O) @@ -923,9 +934,9 @@ clean-root-dsym: @rm -f shellclaw test_agent test_anthropic test_channel test_cli test_config test_file test_memory test_local_provider test_openai test_provider test_router test_shell test_skill test_telegram test_web_search test_ws clean: clean-root-dsym - rm -f $(OBJS) $(PROVIDER_COMMON_O) $(STUB_O) $(ANTHROPIC_O) $(OPENAI_COMPAT_O) $(OPENAI_O) $(LOCAL_O) $(ROUTER_O) $(CJSON_O) $(TWEETNACL_O) $(ANTHROPIC_TEST_O) $(OPENAI_TEST_O) $(LOCAL_TEST_O) $(CONTEXT_TEST_OBJS) $(HEARTBEAT_TEST_O) $(CHANNEL_TG_TEST_O) $(CHANNEL_COMMON_O) $(CHANNEL_STUB_O) $(CHANNEL_CLI_O) $(CHANNEL_TG_O) $(CHANNEL_DISCORD_O) $(DISCORD_HELPERS_O) $(CHANNEL_HEARTBEAT_O) $(CHANNEL_WEBCHAT_O) $(AUTH_O) $(STATIC_O) $(HTTP_O) $(HTTP_LWS_O) $(ASAP_HTTP_BODY_O) $(ROUTES_O) $(ROUTES_HARDWARE_O) $(WS_O) $(MANIFEST_O) $(MANIFEST_PROFILES_O) $(MANIFEST_BUILD_O) $(MANIFEST_SIGN_O) $(MANIFEST_KEYS_O) $(ENVELOPE_O) $(ULID_O) $(CLIENT_O) $(ASAP_REGISTRY_O) $(SERVER_O) $(ASAP_LOG_O) $(RATE_LIMIT_O) $(SHELL_O) $(WEBSEARCH_O) $(FILE_O) $(REGISTRY_O) $(CONTEXT_O) $(CONTEXT_CACHE_O) $(CONTEXT_HTTP_O) $(CONTEXT_GEO_O) $(CRYPTO_O) $(JCS_O) $(HARDWARE_STUB_O) $(HARDWARE_INIT_O) $(HARDWARE_GPIO_SNAPSHOT_O) $(HARDWARE_TEGRASTATS_O) $(HARDWARE_TOOLS_O) $(BOARD_DETECT_O) src/hardware/hardware_libgpiod.o $(HARDWARE_I2C_O) $(HARDWARE_CAMERA_O) $(CRON_O) $(ASAP_INVOKE_O) $(SANDBOX_CORE_O) $(SANDBOX_LANDLOCK_O) $(ALLOWLIST_SCAN_O) $(ALLOWLIST_PATH_O) + rm -f $(OBJS) $(PROVIDER_COMMON_O) $(STUB_O) $(ANTHROPIC_O) $(OPENAI_COMPAT_O) $(OPENAI_O) $(LOCAL_O) $(ROUTER_O) $(CJSON_O) $(TWEETNACL_O) $(ANTHROPIC_TEST_O) $(OPENAI_TEST_O) $(LOCAL_TEST_O) $(CONTEXT_TEST_OBJS) $(HEARTBEAT_TEST_O) $(CHANNEL_TG_TEST_O) $(CHANNEL_COMMON_O) $(CHANNEL_STUB_O) $(CHANNEL_CLI_O) $(CHANNEL_TG_O) $(CHANNEL_DISCORD_O) $(DISCORD_HELPERS_O) $(CHANNEL_HEARTBEAT_O) $(CHANNEL_WEBCHAT_O) $(AUTH_O) $(STATIC_O) $(HTTP_O) $(HTTP_LWS_O) $(ASAP_HTTP_BODY_O) $(ROUTES_O) $(ROUTES_HARDWARE_O) $(WS_O) $(MANIFEST_O) $(MANIFEST_PROFILES_O) $(MANIFEST_BUILD_O) $(MANIFEST_SIGN_O) $(MANIFEST_KEYS_O) $(ENVELOPE_O) $(ULID_O) $(CLIENT_O) $(ASAP_REGISTRY_O) $(SERVER_O) $(ASAP_LOG_O) $(RATE_LIMIT_O) $(SHELL_O) $(WEBSEARCH_O) $(FILE_O) $(REGISTRY_O) $(CONTEXT_O) $(CONTEXT_CACHE_O) $(CONTEXT_HTTP_O) $(CONTEXT_GEO_O) $(CRYPTO_O) $(JCS_O) $(HARDWARE_STUB_O) $(HARDWARE_INIT_O) $(HARDWARE_GPIO_SNAPSHOT_O) $(HARDWARE_TEGRASTATS_O) $(HARDWARE_TOOLS_O) $(BOARD_DETECT_O) src/hardware/hardware_libgpiod.o $(HARDWARE_I2C_O) $(HARDWARE_CAMERA_O) $(CRON_O) $(ASAP_INVOKE_O) $(SANDBOX_CORE_O) $(SANDBOX_LANDLOCK_O) $(SANDBOX_ENV_O) $(ALLOWLIST_SCAN_O) $(ALLOWLIST_PATH_O) rm -f src/gateway/ui_assets.h find . -name '*.gcno' -o -name '*.gcda' -o -name '*.gcov' | xargs rm -f 2>/dev/null || true - rm -f $(WS_TEST_O) $(BINDIR)/asap_registry_test.o $(BINDIR)/asap_invoke_test.o $(CONTEXT_TEST_OBJS) $(HEARTBEAT_TEST_O) $(BINDIR)/shellclaw $(BINDIR)/test_tweetnacl_smoke $(BINDIR)/test_config $(BINDIR)/test_config_patch $(BINDIR)/test_memory $(BINDIR)/test_skill $(BINDIR)/test_provider $(BINDIR)/test_anthropic $(BINDIR)/test_openai $(BINDIR)/test_local_provider $(BINDIR)/test_router $(BINDIR)/test_heartbeat $(BINDIR)/test_crypto $(BINDIR)/test_hardware_stub $(BINDIR)/test_board_detect $(BINDIR)/test_hardware_libgpiod $(BINDIR)/test_hardware_i2c $(BINDIR)/test_hardware_camera $(BINDIR)/test_pin_tables $(BINDIR)/test_hardware_init $(BINDIR)/test_hardware_tools $(BINDIR)/test_registry $(BINDIR)/test_ws $(BINDIR)/test_agent $(BINDIR)/test_channel $(BINDIR)/test_cli $(BINDIR)/test_shell $(BINDIR)/test_file $(BINDIR)/test_telegram $(BINDIR)/test_discord_helpers $(BINDIR)/test_web_search $(BINDIR)/test_cron $(BINDIR)/test_context $(BINDIR)/test_manifest_build $(BINDIR)/test_manifest_keys $(BINDIR)/test_jcs $(BINDIR)/test_asap_envelope $(BINDIR)/test_asap_ulid $(BINDIR)/test_asap_client $(BINDIR)/test_asap_registry $(BINDIR)/test_asap_server $(BINDIR)/test_asap_invoke $(BINDIR)/test_asap_log $(BINDIR)/test_auth $(BINDIR)/test_gateway_http $(BINDIR)/test_gateway_db_lock $(BINDIR)/test_static $(BINDIR)/test_sandbox $(BINDIR)/test_allowlist $(BINDIR)/test_rate_limit + rm -f $(WS_TEST_O) $(SANDBOX_TEST_O) $(BINDIR)/asap_registry_test.o $(BINDIR)/asap_invoke_test.o $(CONTEXT_TEST_OBJS) $(HEARTBEAT_TEST_O) $(BINDIR)/shellclaw $(BINDIR)/test_tweetnacl_smoke $(BINDIR)/test_config $(BINDIR)/test_config_patch $(BINDIR)/test_memory $(BINDIR)/test_skill $(BINDIR)/test_provider $(BINDIR)/test_anthropic $(BINDIR)/test_openai $(BINDIR)/test_local_provider $(BINDIR)/test_router $(BINDIR)/test_heartbeat $(BINDIR)/test_crypto $(BINDIR)/test_hardware_stub $(BINDIR)/test_board_detect $(BINDIR)/test_hardware_libgpiod $(BINDIR)/test_hardware_i2c $(BINDIR)/test_hardware_camera $(BINDIR)/test_pin_tables $(BINDIR)/test_hardware_init $(BINDIR)/test_hardware_tools $(BINDIR)/test_registry $(BINDIR)/test_ws $(BINDIR)/test_agent $(BINDIR)/test_channel $(BINDIR)/test_cli $(BINDIR)/test_shell $(BINDIR)/test_file $(BINDIR)/test_telegram $(BINDIR)/test_discord_helpers $(BINDIR)/test_web_search $(BINDIR)/test_cron $(BINDIR)/test_context $(BINDIR)/test_manifest_build $(BINDIR)/test_manifest_keys $(BINDIR)/test_jcs $(BINDIR)/test_asap_envelope $(BINDIR)/test_asap_ulid $(BINDIR)/test_asap_client $(BINDIR)/test_asap_registry $(BINDIR)/test_asap_server $(BINDIR)/test_asap_invoke $(BINDIR)/test_asap_log $(BINDIR)/test_auth $(BINDIR)/test_gateway_http $(BINDIR)/test_gateway_db_lock $(BINDIR)/test_static $(BINDIR)/test_sandbox $(BINDIR)/test_allowlist $(BINDIR)/test_rate_limit rm -rf $(BINDIR)/*.dSYM $(DSYMDIR) rm -f $(BOOTSTRAP_DISPATCH_STUB_O) $(TOOL_RELOAD_STUB_O) $(RELOAD_CHANNEL_STUB_O) $(HTTP_RELOAD_STUB_O) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 4a2b3fd..c6ac5e1 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -39,8 +39,8 @@ The primary goals are: prevent sandboxed shell commands from escaping to host de | Surface | Mechanism | Notes | |---------|-----------|-------| -| Shell (sandbox on) | `fork()` + user ns when needed + `unshare(CLONE_NEWNS \| CLONE_NEWNET \| CLONE_NEWPID)` + fork for PID 1 + Landlock workspace bound + `prctl(PR_SET_NO_NEW_PRIVS)` | Fail-closed if namespaces or Landlock cannot apply. Isolation uses a control pipe, not `sh` exit 122/123. See [Linux sandbox (Jetson)](#linux-sandbox-jetson) | -| Shell (sandbox off) | Plain `fork()` + substring fallback blocklist | **Not** a security boundary; stderr warning | +| Shell (sandbox on) | `fork()` + user ns when needed + `unshare(CLONE_NEWNS \| CLONE_NEWNET \| CLONE_NEWPID)` + fork for PID 1 + Landlock workspace bound + `prctl(PR_SET_NO_NEW_PRIVS)` + secret-free environment | Fail-closed if namespaces or Landlock cannot apply. The command child keeps `PATH`, locale, `TERM`, `TZ`, and `HOME` set to the workspace. Host API keys and bot tokens are not inherited. Isolation uses a control pipe, not `sh` exit 122/123. See [Linux sandbox (Jetson)](#linux-sandbox-jetson) | +| Shell (sandbox off) | Plain `fork()` + substring fallback blocklist | **Not** a security boundary; the full host environment is inherited — `printenv` works there (see [Command environment model](#command-environment-model)); stderr warning | | Allowlist | Substring blocklist + workspace containment (ancestor walk, quoted/embedded `/` `~`, `$HOME`/`$PWD`, `file:` URLs, relative names) | Defense-in-depth string scan; Landlock is the kernel host-FS bound | | Landlock | Ruleset on configured `workspace_path` (RW workspace + traverse-only `/` + RO `/bin` `/usr` `/lib` and `/etc/ssl/certs` only; RW `/dev/null`, `/dev/zero`, `/dev/urandom`, `/dev/tty` without `IOCTL_DEV`). Child `fchdir`s the workspace before `restrict_self`. A failed `landlock_add_rule` fails the ruleset closed. | Primary host-FS gate; `/` is `READ_DIR` only so `/etc/passwd` stays closed. `/etc/ssl/private` is not granted. Blocks symlink and `chr(47)+` host reads | | cgroups v2 | `memory.max`, `cpu.max`; isolator writes `cgroup.procs` before the command fork | Best-effort; non-fatal if cgroup write fails | @@ -48,6 +48,19 @@ The primary goals are: prevent sandboxed shell commands from escaping to host de Implementation: [`src/sandbox/sandbox.c`](../src/sandbox/sandbox.c), [`src/sandbox/allowlist.c`](../src/sandbox/allowlist.c). +### Command environment model + +The two shell paths differ in environment exposure as much as in isolation, so the model is stated explicitly: + +| Path | Environment the command sees | `printenv` inside the command | +|------|------------------------------|------------------------------| +| Sandboxed (`sandbox.enabled = true`) | Scrubbed whitelist only: `PATH`, `LANG`, `LC_ALL`, `LC_CTYPE`, `TERM`, and `TZ` keep their host values when set, and `HOME` is set to the configured workspace (when one is set). Everything else is dropped (`clearenv` before `exec`). | Only the whitelist: provider keys, bot tokens, and loader variables are gone. | +| Unsandboxed fallback (`sandbox.enabled = false`, the default when the key is omitted) | The full agent (host) environment is inherited. | Works — it prints the agent's environment, secrets included. Intentional: this path is reduced isolation, not a security boundary; a stderr warning is printed per command. | + +Jetson-relevant variables that deliberately do **not** pass into the sandbox: `LD_LIBRARY_PATH` and all other `LD_*` (including `LD_PRELOAD`), `CUDA_HOME` and other `CUDA_*`, `TMPDIR`, `SSL_CERT_FILE`, and proxy variables (`http_proxy`, `https_proxy`, `no_proxy`, …). Sandboxed workloads that rely on them must set them per command (for example `LD_LIBRARY_PATH=… cmd`); there is no `[sandbox]` passthrough list in v1.0. Tool paths that genuinely need host setup (camera capture) run argv-only `execvp` in the agent process, outside the shell sandbox. + +Implementation: [`src/sandbox/sandbox_env.c`](../src/sandbox/sandbox_env.c). + --- ## Linux sandbox (Jetson) diff --git a/src/sandbox/sandbox.c b/src/sandbox/sandbox.c index 4aac1a7..fefddc5 100644 --- a/src/sandbox/sandbox.c +++ b/src/sandbox/sandbox.c @@ -12,7 +12,9 @@ * procfs, applies Landlock, sets PR_SET_PDEATHSIG, and closes fds >= 3 * so a timeout SIGKILL of the isolator cannot leave the command under * host init. The isolator joins its cgroup before the command fork so - * memory.max and cpu.max apply to sh -c. Limits degrade if unavailable. + * memory.max and cpu.max apply to sh -c. The command process then + * replaces the inherited environment so provider keys and bot tokens + * are not readable via printenv. Limits degrade if unavailable. * * Non-Linux path: plain fork() + execl(); a warning is emitted to stderr. */ @@ -20,6 +22,7 @@ #define _POSIX_C_SOURCE 200809L #include "sandbox/sandbox.h" +#include "sandbox/sandbox_env.h" #include "sandbox/sandbox_landlock.h" #include #include @@ -107,36 +110,90 @@ static void cgroup_remove(const char *base, const char *name) #endif /* __linux__ */ -static size_t drain_pipe(int fd, char *buf, size_t cap, int timeout_ms) +/* Sandbox unit tests link drain_pipe as a global under + * SHELLCLAW_SANDBOX_TEST (CR-21 provider-test pattern). CI runners revoke + * user namespaces, so the sandbox_exec success-path tests skip there and + * the wall-clock deadline loop would otherwise stay uncovered. */ +#ifdef SHELLCLAW_SANDBOX_TEST +#define SANDBOX_UNIT +#else +#define SANDBOX_UNIT static +#endif + +/* Docs in sandbox.h. */ +long monotonic_elapsed_ms(const struct timespec *start) +{ + struct timespec now; + long ms; + + if (clock_gettime(CLOCK_MONOTONIC, &now) != 0) + return -1; + ms = (now.tv_sec - start->tv_sec) * 1000L + + (now.tv_nsec - start->tv_nsec) / 1000000L; + return ms < 0 ? 0 : ms; +} + +int poll_remain_ms(int timeout_ms, long elapsed_ms, int slice_cap) { + long remain; + + remain = (long)timeout_ms - elapsed_ms; + if (remain > slice_cap) + return slice_cap; + return (int)remain; +} + +/* + * Stop at timeout_ms of wall time, not idle poll time. A writer that keeps + * the pipe readable otherwise never advances the old counter, so the agent + * stays blocked until cap fills. + */ +SANDBOX_UNIT size_t drain_pipe(int fd, char *buf, size_t cap, int timeout_ms, int *hit_deadline) +{ + struct timespec start; size_t total = 0; - int elapsed_ms = 0; char chunk[512]; - if (cap == 0) return 0; - while (total < cap - 1 && elapsed_ms < timeout_ms) { + + if (hit_deadline) + *hit_deadline = 0; + if (cap == 0) + return 0; + /* Unchecked on purpose: CLOCK_MONOTONIC failure is not a realistic case + * (vDSO). If it ever failed, monotonic_elapsed_ms keeps returning -1, + * the deadline never fires, and poll slices stay bounded. */ + (void)clock_gettime(CLOCK_MONOTONIC, &start); + while (total < cap - 1) { struct pollfd pfd; + long elapsed_ms; int slice; int r; ssize_t n; + + elapsed_ms = monotonic_elapsed_ms(&start); + if (elapsed_ms >= (long)timeout_ms) { + if (hit_deadline) + *hit_deadline = 1; + break; + } + slice = poll_remain_ms(timeout_ms, elapsed_ms, PIPE_POLL_SLICE_MS); pfd.fd = fd; pfd.events = POLLIN; pfd.revents = 0; - slice = timeout_ms - elapsed_ms; - if (slice > PIPE_POLL_SLICE_MS) slice = PIPE_POLL_SLICE_MS; r = poll(&pfd, 1, slice); if (r < 0) { - if (errno == EINTR) continue; + if (errno == EINTR) + continue; break; } - if (r == 0) { - elapsed_ms += slice; + if (r == 0) continue; - } n = read(fd, chunk, sizeof(chunk)); - if (n <= 0) break; + if (n <= 0) + break; { size_t add = (size_t)n; - if (total + add >= cap - 1) add = cap - 1 - total; + if (total + add >= cap - 1) + add = cap - 1 - total; memcpy(buf + total, chunk, add); total += add; } @@ -446,6 +503,7 @@ int sandbox_exec(const char *cmd, char *out, size_t out_cap, #endif close(errpipe[1]); close_inherited_fds(); + sandbox_scrub_command_environment(workspace); execl("/bin/sh", "sh", "-c", cmd, (char *)NULL); _exit(127); } @@ -467,9 +525,14 @@ int sandbox_exec(const char *cmd, char *out, size_t out_cap, return report_isolation_failure(out, out_cap, iso); } } - total = drain_pipe(pipefd[0], out, out_cap, timeout_ms); - close(pipefd[0]); - timed_out = reap_child(pid, NULL); + { + int drain_deadline = 0; + total = drain_pipe(pipefd[0], out, out_cap, timeout_ms, &drain_deadline); + close(pipefd[0]); + timed_out = reap_child(pid, NULL); + if (drain_deadline) + timed_out = 1; + } if (timed_out && total < out_cap - 40) snprintf(out + total, out_cap - total, "\n[Sandbox: command timed out after %d ms]", timeout_ms); diff --git a/src/sandbox/sandbox.h b/src/sandbox/sandbox.h index 277d513..37bc7e8 100644 --- a/src/sandbox/sandbox.h +++ b/src/sandbox/sandbox.h @@ -16,6 +16,7 @@ #define SHELLCLAW_SANDBOX_H #include +#include #ifdef __cplusplus extern "C" { @@ -45,6 +46,25 @@ typedef struct sandbox_config { const char *cgroup_base; } sandbox_config_t; +/** + * Wall-clock milliseconds elapsed since @p start, or -1 when the monotonic + * clock cannot be read. Shared by the sandbox drain loop and the unsandboxed + * shell loop so steady output cannot bypass a timeout (#116). + * + * Example: long ms = monotonic_elapsed_ms(&start); + */ +long monotonic_elapsed_ms(const struct timespec *start); + +/** + * Poll slice for the remaining wall-clock deadline: the positive remainder + * of @p timeout_ms minus @p elapsed_ms, capped at @p slice_cap. Call only + * after checking @p elapsed_ms < @p timeout_ms; an expired deadline is not + * clamped here, so a post-deadline call can return a negative value. + * + * Example: poll_remain_ms(1000, 950, 100) returns 50. + */ +int poll_remain_ms(int timeout_ms, long elapsed_ms, int slice_cap); + /** * Execute @p cmd inside an isolated child process and capture output. * @@ -65,6 +85,8 @@ typedef struct sandbox_config { * @param out Buffer for captured stdout+stderr. Always NUL-terminated on success. * @param out_cap Capacity of @p out (including NUL byte). Must be > 0. * @param timeout_ms Maximum wall-clock milliseconds before SIGKILL. 0 = default (10 000 ms). + * The timeout clock starts only after the isolation handshake, + * so slow namespace/Landlock setup is not counted. * @param cfg Optional sandbox configuration. NULL = use built-in defaults. * @return 0 on success (command ran; check output for exit status text), * -1 on system error (pipe/fork/clone/isolation failure). diff --git a/src/sandbox/sandbox_env.c b/src/sandbox/sandbox_env.c new file mode 100644 index 0000000..158cedf --- /dev/null +++ b/src/sandbox/sandbox_env.c @@ -0,0 +1,77 @@ +/** + * @file sandbox_env.c + * @brief Secret-free environment for the sandboxed command process. + * + * Provider keys and channel tokens live in the agent environment. The + * shell tool's stdout is returned to the model, so sh -c must not inherit + * them. A prompt-injected printenv would otherwise leak the values. + */ +#define _GNU_SOURCE +#define _POSIX_C_SOURCE 200809L + +#include "sandbox/sandbox_env.h" + +#include +#include +#if !defined(__linux__) +#include +#endif + +static const char FALLBACK_PATH[] = + "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"; + +static char *dup_env(const char *name) +{ + const char *v; + + v = getenv(name); + if (!v || !v[0]) + return NULL; + return strdup(v); +} + +static void put_env(const char *name, const char *value) +{ + if (value && value[0]) + setenv(name, value, 1); +} + +static void free_kept(char *path, char *lang, char *lc_all, char *lc_ctype, + char *term, char *tz) +{ + free(path); + free(lang); + free(lc_all); + free(lc_ctype); + free(term); + free(tz); +} + +void sandbox_scrub_command_environment(const char *workspace) +{ + char *path = dup_env("PATH"); + char *lang = dup_env("LANG"); + char *lc_all = dup_env("LC_ALL"); + char *lc_ctype = dup_env("LC_CTYPE"); + char *term = dup_env("TERM"); + char *tz = dup_env("TZ"); +#if !defined(__linux__) + static char *empty_env[] = { NULL }; + extern char **environ; +#endif + +#if defined(__linux__) + clearenv(); +#else + environ = empty_env; +#endif + put_env("PATH", path ? path : FALLBACK_PATH); + put_env("LANG", lang); + put_env("LC_ALL", lc_all); + put_env("LC_CTYPE", lc_ctype); + put_env("TERM", term); + put_env("TZ", tz); + if (workspace && workspace[0]) + setenv("HOME", workspace, 1); + free_kept(path, lang, lc_all, lc_ctype, term, tz); +} diff --git a/src/sandbox/sandbox_env.h b/src/sandbox/sandbox_env.h new file mode 100644 index 0000000..2d4e951 --- /dev/null +++ b/src/sandbox/sandbox_env.h @@ -0,0 +1,18 @@ +/** + * @file sandbox_env.h + * @brief Drop host secrets before exec of a sandboxed shell command. + */ +#ifndef SHELLCLAW_SANDBOX_ENV_H +#define SHELLCLAW_SANDBOX_ENV_H + +/** + * Replace the process environment with PATH, locale, and an optional HOME. + * + * Call only in the child that is about to execl sh -c. API keys, bot + * tokens, and LD_* variables from the agent are not passed through. + * + * Example: sandbox_scrub_command_environment("/home/me/.shellclaw/workspace"); + */ +void sandbox_scrub_command_environment(const char *workspace); + +#endif diff --git a/src/tools/shell.c b/src/tools/shell.c index 65a2d97..bc70a2e 100644 --- a/src/tools/shell.c +++ b/src/tools/shell.c @@ -115,7 +115,8 @@ static int run_unsandboxed(const char *command, int timeout_sec, pid_t pid; size_t total = 0; int timed_out = 0; - int elapsed_ms = 0; + long timeout_ms; + struct timespec start_ts; char buf[256]; if (pipe(pipefd) != 0) { snprintf(result_buf, max_len, "{\"error\":\"pipe failed\"}"); @@ -140,30 +141,36 @@ static int run_unsandboxed(const char *command, int timeout_sec, (void)setpgid(pid, pid); close(pipefd[1]); result_buf[0] = '\0'; - while (total < max_len - 1 && elapsed_ms < timeout_sec * 1000) { + /* Unchecked on purpose: CLOCK_MONOTONIC failure is not a realistic case + * (vDSO). If it ever failed, monotonic_elapsed_ms keeps returning -1, + * the deadline never fires, and poll slices stay bounded. */ + (void)clock_gettime(CLOCK_MONOTONIC, &start_ts); + /* Wall time, not idle poll time: a steady writer never used to trip + * the timeout, so the agent lock stayed held until the buffer filled. */ + timeout_ms = (long)timeout_sec * 1000L; + while (total < max_len - 1) { struct pollfd pfd; + long elapsed_ms; int rem; int r; ssize_t n; + elapsed_ms = monotonic_elapsed_ms(&start_ts); + if (elapsed_ms >= timeout_ms) { + timed_out = 1; + kill_command_tree(pid); + break; + } pfd.fd = pipefd[0]; pfd.events = POLLIN; pfd.revents = 0; - rem = timeout_sec * 1000 - elapsed_ms; - if (rem > 5000) rem = 5000; + rem = poll_remain_ms((int)timeout_ms, elapsed_ms, 5000); r = poll(&pfd, 1, rem); if (r < 0) { if (errno == EINTR) continue; break; } - if (r == 0) { - elapsed_ms += rem; - if (elapsed_ms >= timeout_sec * 1000) { - timed_out = 1; - kill_command_tree(pid); - break; - } + if (r == 0) continue; - } n = read(pipefd[0], buf, sizeof(buf)); if (n <= 0) break; append_unsandboxed_output(result_buf, max_len, &total, buf, (size_t)n); diff --git a/tests/test_sandbox.c b/tests/test_sandbox.c index 45fa2d0..e082059 100644 --- a/tests/test_sandbox.c +++ b/tests/test_sandbox.c @@ -17,13 +17,16 @@ #include "sandbox/sandbox.h" #include "sandbox/sandbox_landlock.h" +#include "sandbox/sandbox_env.h" #include #include +#include #include #include #include #include #include +#include #ifdef __linux__ #include #include @@ -31,7 +34,6 @@ #include #include #include -#include #endif #define ASSERT(c) do { \ @@ -147,6 +149,43 @@ static int test_stderr_captured(void) return 0; } +/** Host API keys must not be readable from sh -c. printenv is not blocklisted. */ +static int test_command_does_not_inherit_host_secrets(void) +{ + char out[4096]; + char ws[] = "/tmp/sc_sb_env_XXXXXX"; + sandbox_config_t cfg; + char *dir; + int rc; + const char *secret = "leaked_via_env"; + + ASSERT(setenv("SHELLCLAW_AUDIT_SECRET", secret, 1) == 0); + ASSERT(setenv("ANTHROPIC_API_KEY", secret, 1) == 0); + dir = mkdtemp(ws); + if (!dir) { + fprintf(stderr, "test_command_does_not_inherit_host_secrets: mkdtemp failed\n"); + return 1; + } + memset(&cfg, 0, sizeof cfg); + cfg.workspace_path = dir; + rc = sandbox_exec( + "printenv SHELLCLAW_AUDIT_SECRET ANTHROPIC_API_KEY; " + "tr '\\0' '\\n' < /proc/1/environ 2>/dev/null; " + "echo HOME=$(printenv HOME); echo PATH_OK", + out, sizeof out, 5000, &cfg); + rmdir(dir); + unsetenv("SHELLCLAW_AUDIT_SECRET"); + unsetenv("ANTHROPIC_API_KEY"); + if (skip_if_isolation_denied(rc, out, "test_command_does_not_inherit_host_secrets")) + return 0; + ASSERT(rc == 0); + ASSERT(strstr(out, secret) == NULL); + /* A real workspace exercises the HOME=workspace branch of the scrub. */ + ASSERT(strstr(out, dir) != NULL); + ASSERT(strstr(out, "PATH_OK") != NULL); + return 0; +} + static int test_null_cmd_returns_error(void) { char out[256]; @@ -277,6 +316,57 @@ static int test_missing_workspace_fail_closed(void) return 0; } +static void trickle_timeout_watchdog(int sig) +{ + (void)sig; + fprintf(stderr, "FAIL: sandbox timeout did not fire while stdout trickled\n"); + _exit(2); +} + +static int test_timeout_kills_trickle_output(void) +{ + char out[8192]; + struct timespec t0; + struct timespec t1; + long elapsed_ms; + int rc; + + ASSERT(clock_gettime(CLOCK_MONOTONIC, &t0) == 0); + signal(SIGALRM, trickle_timeout_watchdog); + alarm(4); + /* The background sleep is a grandchild in the command process group, + * so the deadline kill must wipe it along with the trickling shell. */ + rc = sandbox_exec("sleep 9999 & while true; do echo x; sleep 0.05; done", + out, sizeof(out), 400, NULL); + alarm(0); + signal(SIGALRM, SIG_DFL); + if (skip_if_isolation_denied(rc, out, "test_timeout_kills_trickle_output")) + return 0; + ASSERT(clock_gettime(CLOCK_MONOTONIC, &t1) == 0); + elapsed_ms = (t1.tv_sec - t0.tv_sec) * 1000L + + (t1.tv_nsec - t0.tv_nsec) / 1000000L; + ASSERT(rc == 0); + ASSERT(strstr(out, "timed out") != NULL); + ASSERT(elapsed_ms < 2500); +#ifdef __linux__ + { + int tries; + for (tries = 0; tries < 20; tries++) { + if (!proc_cmdline_has("sleep 9999")) + break; + { + struct timespec ts; + ts.tv_sec = 0; + ts.tv_nsec = 50 * 1000 * 1000; + nanosleep(&ts, NULL); + } + } + ASSERT(proc_cmdline_has("sleep 9999") == 0); + } +#endif + return 0; +} + static int test_timeout_kills_process(void) { char out[4096]; @@ -817,14 +907,152 @@ static int test_command_inherits_cgroup(void) } #endif +/* --- No-namespace units for the wall-clock deadline rewrite (#116) and the + * secret-free child environment (#109). GitHub runners reject unshare, so + * the sandbox_exec paths above skip and these units keep the helpers + * covered on every host. Linked from sandbox_test.o. --- */ + +size_t drain_pipe(int fd, char *buf, size_t cap, int timeout_ms, int *hit_deadline); + +static int test_poll_remain_ms_bounds(void) +{ + ASSERT(poll_remain_ms(1000, 0, 100) == 100); + ASSERT(poll_remain_ms(1000, 950, 100) == 50); + ASSERT(poll_remain_ms(1000, 999, 100) == 1); + ASSERT(poll_remain_ms(1000, 0, 5000) == 1000); + return 0; +} + +static int test_monotonic_elapsed_ms_never_negative(void) +{ + struct timespec start; + struct timespec future; + + ASSERT(clock_gettime(CLOCK_MONOTONIC, &start) == 0); + usleep(30000); + ASSERT(monotonic_elapsed_ms(&start) >= 20); + ASSERT(monotonic_elapsed_ms(&start) < 60000); + ASSERT(clock_gettime(CLOCK_MONOTONIC, &future) == 0); + future.tv_sec += 10; + ASSERT(monotonic_elapsed_ms(&future) == 0); + return 0; +} + +static int test_drain_pipe_reads_until_eof(void) +{ + char buf[64]; + const char msg[] = "hi"; + size_t total; + int hit = -1; + int fds[2]; + pid_t pid; + int st; + + ASSERT(pipe(fds) == 0); + pid = fork(); + if (pid == 0) { + ssize_t w; + close(fds[0]); + w = write(fds[1], msg, sizeof(msg) - 1); + (void)w; + close(fds[1]); + exit(0); + } + close(fds[1]); + total = drain_pipe(fds[0], buf, sizeof(buf), 5000, &hit); + close(fds[0]); + waitpid(pid, &st, 0); + ASSERT(total == 2); + ASSERT(hit == 0); + ASSERT(buf[0] == 'h' && buf[1] == 'i'); + return 0; +} + +static int test_drain_pipe_stops_at_wall_deadline(void) +{ + char buf[256]; + char byte = 'x'; + size_t total; + int hit = -1; + int fds[2]; + pid_t pid; + int st; + + ASSERT(pipe(fds) == 0); + pid = fork(); + if (pid == 0) { + close(fds[0]); + /* Trickle one byte every 20 ms so the pipe stays readable and the + * cap is never reached: only the wall deadline can stop the loop. */ + for (;;) { + if (write(fds[1], &byte, 1) <= 0) + exit(0); + usleep(20000); + } + } + close(fds[1]); + total = drain_pipe(fds[0], buf, sizeof(buf), 400, &hit); + close(fds[0]); + waitpid(pid, &st, 0); + ASSERT(hit == 1); + ASSERT(total >= 1); + ASSERT(total < sizeof(buf) - 1); + return 0; +} + +static int test_scrubbed_env_child_loses_secrets(void) +{ + pid_t pid; + int st; + int status; + + pid = fork(); + if (pid == 0) { + if (setenv("SHELLCLAW_AUDIT_SECRET", "leak", 1) != 0) + exit(4); + if (setenv("PATH", "/usr/bin", 1) != 0) + exit(5); + if (setenv("LD_PRELOAD", "/no/such/sc_fake_lib.so", 1) != 0) + exit(6); + sandbox_scrub_command_environment("/ws/home"); + if (getenv("SHELLCLAW_AUDIT_SECRET")) + exit(1); + if (getenv("LD_PRELOAD")) + exit(7); + if (!getenv("PATH") || strcmp(getenv("PATH"), "/usr/bin") != 0) + exit(2); + if (!getenv("HOME") || strcmp(getenv("HOME"), "/ws/home") != 0) + exit(3); + exit(0); + } + if (waitpid(pid, &st, 0) < 0) { + fprintf(stderr, "FAIL: waitpid scrub child: %s\n", strerror(errno)); + return 1; + } + if (!WIFEXITED(st)) + return 1; + status = WEXITSTATUS(st); + if (status != 0) { + fprintf(stderr, "FAIL: scrub env child exit=%d\n", status); + return 1; + } + return 0; +} + int main(void) { RUN(test_isolation_denied_helper()); RUN(test_fail_closed_reports_isolation_error()); RUN(test_output_capture()); RUN(test_stderr_captured()); + RUN(test_command_does_not_inherit_host_secrets()); RUN(test_null_cmd_returns_error()); RUN(test_zero_cap_returns_error()); + RUN(test_scrubbed_env_child_loses_secrets()); + RUN(test_poll_remain_ms_bounds()); + RUN(test_monotonic_elapsed_ms_never_negative()); + RUN(test_drain_pipe_reads_until_eof()); + RUN(test_drain_pipe_stops_at_wall_deadline()); RUN(test_exit_nonzero_runs()); RUN(test_exit_122_is_not_isolation_failure()); RUN(test_exit_123_with_workspace_is_not_isolation_failure()); @@ -832,6 +1060,7 @@ int main(void) RUN(test_dev_null_is_writable()); RUN(test_workspace_chdir()); RUN(test_missing_workspace_fail_closed()); + RUN(test_timeout_kills_trickle_output()); RUN(test_timeout_kills_process()); #ifdef __linux__ RUN(test_landlock_probe_without_restrict()); diff --git a/tests/test_shell.c b/tests/test_shell.c index a37341b..b612139 100644 --- a/tests/test_shell.c +++ b/tests/test_shell.c @@ -281,6 +281,62 @@ static void test_shell_caps_output_without_hanging(void) "sequential sleep 9999 did not leak"); } +static void trickle_timeout_watchdog(int sig) +{ + (void)sig; + fprintf(stderr, "FAIL: shell timeout did not fire while stdout trickled\n"); + _exit(2); +} + +/* Idle-only poll accounting used to ignore shell_timeout_sec while the + * child kept the pipe readable. The agent lock stayed held until the + * output buffer filled. */ +static void test_shell_trickle_output_hits_timeout(void) +{ + const tool_t *t = tool_shell_get(); + config_t *cfg = NULL; + char errbuf[256]; + char cfg_path[64]; + char buf[8192]; + FILE *f; + struct timespec t0; + struct timespec t1; + long elapsed_ms; + int before; + int r; + + snprintf(cfg_path, sizeof(cfg_path), "/tmp/sc_shell_to_%d.toml", (int)getpid()); + f = fopen(cfg_path, "w"); + MU_ASSERT(f != NULL, "write timeout config"); + fputs("[agent]\nmodel = \"test\"\n[sandbox]\nenabled = false\nshell_timeout_sec = 1\n", f); + fclose(f); + MU_ASSERT(config_load(cfg_path, &cfg, errbuf, sizeof(errbuf)) == 0, "load timeout config"); + tool_shell_set_config(cfg); + before = count_sleep_argv("9999"); + MU_ASSERT(before >= 0, "can count sleep 9999 before trickle timeout"); + memset(buf, 0, sizeof(buf)); + signal(SIGALRM, trickle_timeout_watchdog); + alarm(4); + MU_ASSERT(clock_gettime(CLOCK_MONOTONIC, &t0) == 0, "clock start"); + /* The background sleep sits in the command process group, so the + * timeout path must group-kill it together with the trickling shell. */ + r = t->execute("{\"command\":\"sleep 9999 & while true; do echo x; sleep 0.05; done\"}", + buf, sizeof(buf)); + alarm(0); + signal(SIGALRM, SIG_DFL); + MU_ASSERT(clock_gettime(CLOCK_MONOTONIC, &t1) == 0, "clock end"); + elapsed_ms = (t1.tv_sec - t0.tv_sec) * 1000L + + (t1.tv_nsec - t0.tv_nsec) / 1000000L; + tool_shell_set_config(NULL); + config_free(cfg); + unlink(cfg_path); + MU_ASSERT(r == 0, "trickle command returns"); + MU_ASSERT(strstr(buf, "timed out") != NULL, "trickle command reports timeout"); + MU_ASSERT(elapsed_ms < 3000, "trickle command stopped near the 1s timeout"); + MU_ASSERT(sleep_argv_did_not_grow("9999", before), + "timeout path killed the background sleep 9999"); +} + static void test_shell_caps_output_kills_background_sleep(void) { const tool_t *t = tool_shell_get(); @@ -317,6 +373,7 @@ int main(void) MU_RUN(test_shell_invalid_json); MU_RUN(test_shell_missing_command); MU_RUN(test_shell_caps_output_without_hanging); + MU_RUN(test_shell_trickle_output_hits_timeout); MU_RUN(test_shell_caps_output_kills_background_sleep); printf("%d tests run, %d failed\n", tests_run, tests_failed); return tests_failed ? 1 : 0;