diff --git a/CHANGELOG.md b/CHANGELOG.md index cc2e71b..c2a9eb4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to ShellClaw are documented here. Format follows [Keep a Cha ## [Unreleased] ### Fixed +- Unsandboxed `shell` no longer blocks forever in `waitpid` after the output cap fills; leftover children (including background grandchildren) are SIGKILL'd via the command process group, and truncated capture is NUL-terminated (#69). - `write_file` maps to the intended path instead of the first existing ancestor, so a nested path cannot truncate a workspace file treated as a directory or overwrite a same-named file in a parent (#67). Leaf workspace symlinks (dangling or an in-workspace alias) are rejected (`lstat` + `O_NOFOLLOW`) instead of creating host files outside the workspace (#90). - `write_file` persists via unique temp (`mkstemp`)+fsync+rename so a failed write cannot wipe an existing workspace file and a sibling `path.tmp` is not truncated (#78). - Camera capture fails closed when `workspace_only` is on with an empty `workspace_path`, and rejects leaf symlink outputs (#91, #90). diff --git a/src/tools/shell.c b/src/tools/shell.c index 89f125d..5638e59 100644 --- a/src/tools/shell.c +++ b/src/tools/shell.c @@ -19,12 +19,14 @@ #include "sandbox/sandbox.h" #include "sandbox/allowlist.h" #include "cJSON.h" +#include #include #include #include #include #include #include +#include #include #define DEFAULT_TIMEOUT_SEC 60 @@ -56,6 +58,50 @@ static int fallback_is_blocked(const char *cmd) static const config_t *g_shell_cfg; +static void kill_command_tree(pid_t pid) +{ + if (kill(-pid, SIGKILL) != 0) + (void)kill(pid, SIGKILL); +} + +/* Always SIGKILL the process group first so background grandchildren cannot + * leak after the shell has already exited (SIGPIPE, or `cmd &`). Then reap + * the tracked pid. Matching the hang fix for #69 / #96. */ +static int reap_running_child(pid_t pid) +{ + int status = 0; + int wr; + int retries; + struct timespec ts; + kill_command_tree(pid); + wr = waitpid(pid, &status, WNOHANG); + if (wr != 0) + return 0; + for (retries = 0; retries < 40; retries++) { + wr = waitpid(pid, &status, WNOHANG); + if (wr != 0) + return 1; + ts.tv_sec = 0; + ts.tv_nsec = 50 * 1000 * 1000; + (void)nanosleep(&ts, NULL); + } + (void)waitpid(pid, &status, 0); + return 1; +} + +static void append_unsandboxed_output(char *result_buf, size_t max_len, size_t *total, + const char *chunk, size_t n) +{ + size_t add = n; + if (*total + add >= max_len - 1) + add = max_len - 1 - *total; + if (add == 0) + return; + memcpy(result_buf + *total, chunk, add); + *total += add; + result_buf[*total] = '\0'; +} + /* ------------------------------------------------------------------ */ /* Unsandboxed execution (fork + poll + waitpid) */ /* ------------------------------------------------------------------ */ @@ -68,7 +114,6 @@ static int run_unsandboxed(const char *command, int timeout_sec, size_t total = 0; int timed_out = 0; int elapsed_ms = 0; - int status; char buf[256]; if (pipe(pipefd) != 0) { snprintf(result_buf, max_len, "{\"error\":\"pipe failed\"}"); @@ -86,9 +131,11 @@ static int run_unsandboxed(const char *command, int timeout_sec, dup2(pipefd[1], STDOUT_FILENO); dup2(pipefd[1], STDERR_FILENO); close(pipefd[1]); + (void)setpgid(0, 0); execl("/bin/sh", "sh", "-c", command, (char *)NULL); _exit(127); } + (void)setpgid(pid, pid); close(pipefd[1]); result_buf[0] = '\0'; while (total < max_len - 1 && elapsed_ms < timeout_sec * 1000) { @@ -102,28 +149,27 @@ static int run_unsandboxed(const char *command, int timeout_sec, rem = timeout_sec * 1000 - elapsed_ms; if (rem > 5000) rem = 5000; r = poll(&pfd, 1, rem); - if (r < 0) break; + if (r < 0) { + if (errno == EINTR) continue; + break; + } if (r == 0) { elapsed_ms += rem; if (elapsed_ms >= timeout_sec * 1000) { timed_out = 1; - kill(pid, SIGKILL); + kill_command_tree(pid); break; } continue; } - n = read(pipefd[0], buf, sizeof(buf) - 1); + n = read(pipefd[0], buf, sizeof(buf)); if (n <= 0) break; - buf[n] = '\0'; - { - size_t add = (size_t)n; - if (total + add >= max_len - 1) add = max_len - 1 - total; - memcpy(result_buf + total, buf, add + 1); - total += add; - } + append_unsandboxed_output(result_buf, max_len, &total, buf, (size_t)n); } + result_buf[total] = '\0'; close(pipefd[0]); - waitpid(pid, &status, 0); + if (reap_running_child(pid)) + timed_out = 1; if (timed_out && total < max_len - 32) snprintf(result_buf + total, max_len - total, "\n[Command timed out]"); return 0; diff --git a/tests/test_shell.c b/tests/test_shell.c index 8b43d94..ceb9551 100644 --- a/tests/test_shell.c +++ b/tests/test_shell.c @@ -2,12 +2,20 @@ * @file test_shell.c * @brief Unit tests for shell tool: safe commands, blocklist, timeout. */ +#define _POSIX_C_SOURCE 200809L #include "tools/tool.h" #include "tools/shell.h" #include "core/config.h" +#include +#include +#include +#include #include +#include #include +#include +#include static int tests_run = 0; static int tests_failed = 0; @@ -68,6 +76,195 @@ static void test_shell_missing_command(void) MU_ASSERT(r == -1, "missing command returns -1"); } +static void output_cap_hang_watchdog(int sig) +{ + (void)sig; + fprintf(stderr, "FAIL: unsandboxed shell hung after filling the output cap\n"); + _exit(2); +} + +static int buf_has_nul(const char *buf, size_t n) +{ + size_t i; + for (i = 0; i < n; i++) { + if (buf[i] == '\0') + return 1; + } + return 0; +} + +static int argv0_is_sleep(const char *arg0) +{ + const char *base; + if (!arg0 || !arg0[0]) + return 0; + base = strrchr(arg0, '/'); + base = base ? base + 1 : arg0; + return strcmp(base, "sleep") == 0; +} + +static int cmdline_is_sleep_marker(const char *buf, size_t n, const char *marker) +{ + size_t i = 0; + const char *arg1; + if (n == 0 || !marker) + return 0; + while (i < n && buf[i] != '\0') + i++; + if (i >= n || i + 1 >= n) + return 0; + if (!argv0_is_sleep(buf)) + return 0; + arg1 = buf + i + 1; + return strcmp(arg1, marker) == 0; +} + +static int count_sleep_argv_proc(const char *marker) +{ + DIR *dir; + struct dirent *ent; + int count = 0; + dir = opendir("/proc"); + if (!dir) + return -1; + while ((ent = readdir(dir)) != NULL) { + char path[64]; + char buf[256]; + ssize_t n; + int fd; + if (!isdigit((unsigned char)ent->d_name[0])) + continue; + if (snprintf(path, sizeof(path), "/proc/%s/cmdline", ent->d_name) + >= (int)sizeof(path)) + continue; + fd = open(path, O_RDONLY); + if (fd < 0) + continue; + n = read(fd, buf, sizeof(buf) - 1); + close(fd); + if (n <= 0) + continue; + buf[n] = '\0'; + if (cmdline_is_sleep_marker(buf, (size_t)n + 1, marker)) + count++; + } + closedir(dir); + return count; +} + +static int ps_line_is_sleep_marker(char *line, const char *marker) +{ + char *s = line; + char *nl; + char *base; + size_t marker_len; + nl = strchr(line, '\n'); + if (nl) + *nl = '\0'; + while (*s == ' ' || *s == '\t') + s++; + base = strrchr(s, '/'); + base = base ? base + 1 : s; + marker_len = strlen(marker); + if (strncmp(base, "sleep ", 6) != 0) + return 0; + return strcmp(base + 6, marker) == 0 && marker_len > 0; +} + +static int count_sleep_argv_ps(const char *marker) +{ + FILE *fp; + char line[256]; + int count = 0; + fp = popen("ps -axo args=", "r"); + if (!fp) + return -1; + while (fgets(line, sizeof(line), fp) != NULL) { + if (ps_line_is_sleep_marker(line, marker)) + count++; + } + (void)pclose(fp); + return count; +} + +/* Linux CI: /proc cmdline. Darwin (no /proc): ps args. argv0 must be sleep. */ +static int count_sleep_argv(const char *marker) +{ + int n = count_sleep_argv_proc(marker); + if (n >= 0) + return n; + return count_sleep_argv_ps(marker); +} + +static int sleep_argv_did_not_grow(const char *marker, int before) +{ + struct timespec ts; + int i; + for (i = 0; i < 20; i++) { + int now = count_sleep_argv(marker); + if (now >= 0 && now <= before) + return 1; + ts.tv_sec = 0; + ts.tv_nsec = 50 * 1000 * 1000; + (void)nanosleep(&ts, NULL); + } + return 0; +} + +static void test_shell_caps_output_without_hanging(void) +{ + const tool_t *t = tool_shell_get(); + char buf[64]; + int r; + int before; + tool_shell_set_config(NULL); + memset(buf, 'B', sizeof(buf)); + before = count_sleep_argv("9999"); + MU_ASSERT(before >= 0, "can count sleep 9999 processes"); + signal(SIGALRM, output_cap_hang_watchdog); + alarm(5); + /* Fill the 64-byte cap, then sleep so the child stays alive without + * writing (SIGPIPE will not reap it). Unsandboxed waitpid used to block + * forever on this path (#69). + */ + r = t->execute("{\"command\":\"printf '%080d' 0; sleep 9999\"}", buf, + sizeof(buf)); + alarm(0); + signal(SIGALRM, SIG_DFL); + MU_ASSERT(r == 0, "capped shell command returns"); + MU_ASSERT(buf[0] == '0', "capped output starts with truncated zeros"); + MU_ASSERT(buf_has_nul(buf, sizeof(buf)), "capped output is NUL-terminated"); + MU_ASSERT(sleep_argv_did_not_grow("9999", before), + "sequential sleep 9999 did not leak"); +} + +static void test_shell_caps_output_kills_background_sleep(void) +{ + const tool_t *t = tool_shell_get(); + char buf[64]; + int r; + int before; + tool_shell_set_config(NULL); + memset(buf, 'B', sizeof(buf)); + before = count_sleep_argv("9998"); + MU_ASSERT(before >= 0, "can count sleep 9998 processes"); + signal(SIGALRM, output_cap_hang_watchdog); + alarm(5); + /* Shell can exit after printf while the background sleep stays in the + * process group. Skipping kill_command_tree when waitpid already reaped + * the shell leaked that grandchild (#96). + */ + r = t->execute("{\"command\":\"trap '' HUP; sleep 9998 & printf '%080d' 0\"}", + buf, sizeof(buf)); + alarm(0); + signal(SIGALRM, SIG_DFL); + MU_ASSERT(r == 0, "background capped shell command returns"); + MU_ASSERT(buf[0] == '0', "background capped output is truncated zeros"); + MU_ASSERT(buf_has_nul(buf, sizeof(buf)), "background capped output is NUL-terminated"); + MU_ASSERT(sleep_argv_did_not_grow("9998", before), + "background sleep 9998 did not leak"); +} + int main(void) { MU_RUN(test_shell_blocked_rm_rf); @@ -75,6 +272,8 @@ int main(void) MU_RUN(test_shell_ls_succeeds); MU_RUN(test_shell_invalid_json); MU_RUN(test_shell_missing_command); + MU_RUN(test_shell_caps_output_without_hanging); + 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; }