From 080c174f7d791adef7d1e334d438c5fd12ef06d0 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Sereno Date: Mon, 21 Sep 2026 01:27:48 -0300 Subject: [PATCH 1/4] fix(file): write to the intended path and reject dangling symlinks Ancestor lookup is membership only: do not fopen the resolved parent, which truncated a workspace file treated as a directory. Dangling symlinks fail closed via lstat and O_NOFOLLOW instead of creating host files outside the workspace. Refs: #67, #90 --- CHANGELOG.md | 1 + src/tools/file.c | 158 ++++++++++++++++++++++++++++++++++------------ tests/test_file.c | 134 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 254 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57ce925..6c61e4a 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 +- `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). Dangling workspace symlinks are rejected (`lstat` + `O_NOFOLLOW`) instead of creating host files outside the workspace (#90). - Discord Gateway RX grows for the trailing NUL so two 64 KiB libwebsockets fragments cannot write one byte past the heap block (typical READY payloads). - WebChat inbound WS `rx_buffer_size` is `WS_RX_BUFFER_SIZE` (`WS_TEXT_MAX` plus JSON envelope) so dashboard messages are not split across 256-byte RECEIVE callbacks and dropped. - WebChat WebSocket sends now accept agent replies up to 32 KiB (`WS_TEXT_MAX`, matching `RESPONSE_BUF_SIZE`) instead of silently dropping payloads above 8 KiB. Dest buffers are `WS_TEXT_BUF_SIZE` so a max-length payload keeps its NUL; a too-large frame is logged instead of skipped with `<`. diff --git a/src/tools/file.c b/src/tools/file.c index e8170b1..4219fed 100644 --- a/src/tools/file.c +++ b/src/tools/file.c @@ -2,6 +2,9 @@ * @file file.c * @brief File tool: read_file, write_file, list_dir with workspace boundary check. */ +#if defined(__APPLE__) +#define _DARWIN_C_SOURCE +#endif #define _POSIX_C_SOURCE 200809L #define _GNU_SOURCE @@ -10,6 +13,8 @@ #include "core/config.h" #include "cJSON.h" #include +#include +#include #include #include #include @@ -31,49 +36,134 @@ void tool_file_set_config(const config_t *cfg) g_file_cfg = cfg; } +static int resolved_is_under_workspace(const char *resolved) +{ + char ws_resolved[PATH_MAX]; + const char *workspace; + size_t ws_len; + + if (!resolved || !g_file_cfg) return 0; + workspace = config_workspace_path(g_file_cfg); + if (!workspace || workspace[0] == '\0') return 0; + if (realpath(workspace, ws_resolved) == NULL) return 0; + ws_len = strlen(ws_resolved); + if (strncmp(resolved, ws_resolved, ws_len) != 0) return 0; + if (resolved[ws_len] != '\0' && resolved[ws_len] != '/') return 0; + return 1; +} + +static int path_is_dangling_symlink(const char *path) +{ + struct stat lst; + + if (!path) return 0; + if (lstat(path, &lst) != 0) return 0; + return S_ISLNK(lst.st_mode) ? 1 : 0; +} + static int path_within_workspace(const char *path, char *resolved, size_t resolved_size) { + const char *workspace; + char path_copy[PATH_MAX]; + if (!path || path[0] == '\0') return 0; if (!g_file_cfg || !config_workspace_only(g_file_cfg)) { snprintf(resolved, resolved_size, "%s", path); return 1; } - const char *workspace = config_workspace_path(g_file_cfg); - if (!workspace || workspace[0] == '\0') { - return 0; /* Deny: cannot validate without workspace path */ - } - char ws_resolved[PATH_MAX]; - if (realpath(workspace, ws_resolved) == NULL) return 0; - if (realpath(path, resolved) != NULL) { - size_t ws_len = strlen(ws_resolved); - if (strncmp(resolved, ws_resolved, ws_len) != 0) return 0; - if (resolved[ws_len] != '\0' && resolved[ws_len] != '/') return 0; - return 1; - } - char path_copy[PATH_MAX]; + workspace = config_workspace_path(g_file_cfg); + if (!workspace || workspace[0] == '\0') + return 0; + if (realpath(path, resolved) != NULL) + return resolved_is_under_workspace(resolved); + /* Dangling symlink: ancestor prefix is not enough — open() would follow it. */ + if (path_is_dangling_symlink(path)) + return 0; snprintf(path_copy, sizeof(path_copy), "%s", path); for (;;) { char *dir = dirname(path_copy); if (!dir || dir[0] == '\0') break; - if (realpath(dir, resolved) != NULL) { - size_t ws_len = strlen(ws_resolved); - if (strncmp(resolved, ws_resolved, ws_len) != 0) return 0; - if (resolved[ws_len] != '\0' && resolved[ws_len] != '/') return 0; - return 1; - } + if (realpath(dir, resolved) != NULL) + return resolved_is_under_workspace(resolved); if (strcmp(dir, ".") == 0 || strcmp(dir, "/") == 0) break; snprintf(path_copy, sizeof(path_copy), "%s", dir); } return 0; } +/* + * Membership via ancestor is not the write target. Using that ancestor as + * fopen() would truncate a file treated as a directory (#67). + */ +static int resolve_workspace_write_path(const char *path, char *safe_path, size_t cap) +{ + char parent[PATH_MAX]; + char path_copy[PATH_MAX]; + struct stat st; + const char *base; + char *dir; + int n; + + if (path_is_dangling_symlink(path)) + return 0; + if (realpath(path, safe_path) != NULL) { + if (stat(safe_path, &st) != 0 || !S_ISREG(st.st_mode)) return 0; + return resolved_is_under_workspace(safe_path); + } + if (snprintf(path_copy, sizeof(path_copy), "%s", path) >= (int)sizeof(path_copy)) + return 0; + dir = dirname(path_copy); + if (!dir || realpath(dir, parent) == NULL) return 0; + if (stat(parent, &st) != 0 || !S_ISDIR(st.st_mode)) return 0; + if (!resolved_is_under_workspace(parent)) return 0; + base = strrchr(path, '/'); + base = base ? base + 1 : path; + if (base[0] == '\0' || strcmp(base, ".") == 0 || strcmp(base, "..") == 0) + return 0; + n = snprintf(safe_path, cap, "%s/%s", parent, base); + return n > 0 && (size_t)n < cap; +} + +static int open_write_nofollow(const char *safe_path, int ws_only, + char *result_buf, size_t max_len, FILE **out) +{ + int flags = O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC; + int fd; + FILE *f; + + if (ws_only) + flags |= O_NOFOLLOW; + fd = open(safe_path, flags, 0644); + if (fd < 0) { + if (ws_only && errno == ELOOP) + snprintf(result_buf, max_len, "{\"error\":\"path outside workspace\"}"); + else + snprintf(result_buf, max_len, "{\"error\":\"cannot write file\"}"); + return -1; + } + f = fdopen(fd, "w"); + if (!f) { + close(fd); + snprintf(result_buf, max_len, "{\"error\":\"cannot write file\"}"); + return -1; + } + *out = f; + return 0; +} + static int file_read(const char *path, char *result_buf, size_t max_len) { char resolved[PATH_MAX]; + int ws_only; if (!path_within_workspace(path, resolved, sizeof(resolved))) { snprintf(result_buf, max_len, "{\"error\":\"path outside workspace\"}"); return -1; } + ws_only = g_file_cfg ? config_workspace_only(g_file_cfg) : 0; + if (ws_only && realpath(path, resolved) == NULL) { + snprintf(result_buf, max_len, "{\"error\":\"cannot open file\"}"); + return -1; + } FILE *f = fopen(resolved, "rb"); if (!f) { snprintf(result_buf, max_len, "{\"error\":\"cannot open file\"}"); @@ -104,31 +194,15 @@ static int file_write(const char *path, const char *content, char *result_buf, s } int ws_only = g_file_cfg ? config_workspace_only(g_file_cfg) : 0; char safe_path[PATH_MAX]; + FILE *f; if (!ws_only) { snprintf(safe_path, sizeof(safe_path), "%s", path); - } else { - struct stat st; - if (stat(resolved, &st) == 0 && S_ISREG(st.st_mode)) { - snprintf(safe_path, sizeof(safe_path), "%s", resolved); - } else { - const char *base = strrchr(path, '/'); - base = base ? base + 1 : path; - size_t res_len = strlen(resolved); - size_t base_len = strlen(base); - if (res_len + 1 + base_len >= sizeof(safe_path)) { - snprintf(result_buf, max_len, "{\"error\":\"path too long\"}"); - return -1; - } - memcpy(safe_path, resolved, res_len); - safe_path[res_len] = '/'; - memcpy(safe_path + res_len + 1, base, base_len + 1); - } - } - FILE *f = fopen(safe_path, "w"); - if (!f) { + } else if (!resolve_workspace_write_path(path, safe_path, sizeof(safe_path))) { snprintf(result_buf, max_len, "{\"error\":\"cannot write file\"}"); return -1; } + if (open_write_nofollow(safe_path, ws_only, result_buf, max_len, &f) != 0) + return -1; if (content) { size_t len = strlen(content); if (fwrite(content, 1, len, f) != len) { @@ -145,10 +219,16 @@ static int file_write(const char *path, const char *content, char *result_buf, s static int file_list(const char *path, char *result_buf, size_t max_len) { char resolved[PATH_MAX]; + int ws_only; if (!path_within_workspace(path, resolved, sizeof(resolved))) { snprintf(result_buf, max_len, "{\"error\":\"path outside workspace\"}"); return -1; } + ws_only = g_file_cfg ? config_workspace_only(g_file_cfg) : 0; + if (ws_only && realpath(path, resolved) == NULL) { + snprintf(result_buf, max_len, "{\"error\":\"cannot list directory\"}"); + return -1; + } DIR *d = opendir(resolved); if (!d) { snprintf(result_buf, max_len, "{\"error\":\"cannot list directory\"}"); diff --git a/tests/test_file.c b/tests/test_file.c index b4087ac..dd4f173 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -196,6 +196,137 @@ static void test_symlink_escape_rejected(void) rmdir(tmpdir); } +static int slurp_file(const char *path, char *buf, size_t cap) +{ + FILE *f = fopen(path, "rb"); + size_t n; + if (!f) return -1; + n = fread(buf, 1, cap - 1, f); + fclose(f); + buf[n] = '\0'; + return 0; +} + +static void test_write_does_not_truncate_file_used_as_directory(void) +{ + char tmpdir[PATH_MAX]; + char victim[PATH_MAX]; + char nested[PATH_MAX]; + char config_path[PATH_MAX]; + char args[PATH_MAX + 128]; + char buf[256]; + char kept[64]; + config_t *cfg; + const tool_t *t; + FILE *f; + int r; + + snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_filedir_%d", (int)getpid()); + if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) return; + snprintf(victim, sizeof(victim), "%s/important.md", tmpdir); + f = fopen(victim, "w"); + MU_ASSERT(f != NULL, "create victim file"); + fputs("KEEP", f); + fclose(f); + cfg = make_ws_config(tmpdir, config_path, sizeof(config_path)); + MU_ASSERT(cfg != NULL, "file-as-dir: load config"); + tool_file_set_config(cfg); + t = tool_file_get(); + snprintf(nested, sizeof(nested), "%s/important.md/nested.txt", tmpdir); + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"PWNED\"}", nested); + r = t->execute(args, buf, sizeof(buf)); + MU_ASSERT(r == -1, "write through file-as-directory is rejected"); + MU_ASSERT(slurp_file(victim, kept, sizeof(kept)) == 0, "victim still readable"); + MU_ASSERT(strcmp(kept, "KEEP") == 0, "victim content preserved"); + snprintf(args, sizeof(args), "{\"operation\":\"read_file\",\"path\":\"%s\"}", nested); + r = t->execute(args, buf, sizeof(buf)); + MU_ASSERT(r == -1, "read through file-as-directory is rejected"); + MU_ASSERT(strstr(buf, "KEEP") == NULL, "read does not leak victim contents"); + config_free(cfg); + unlink(config_path); + unlink(victim); + rmdir(tmpdir); +} + +static void test_write_does_not_collapse_missing_parent_onto_basename(void) +{ + char tmpdir[PATH_MAX]; + char victim[PATH_MAX]; + char nested[PATH_MAX]; + char config_path[PATH_MAX]; + char args[PATH_MAX + 128]; + char buf[256]; + char kept[64]; + config_t *cfg; + const tool_t *t; + FILE *f; + int r; + + snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_missdir_%d", (int)getpid()); + if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) return; + snprintf(victim, sizeof(victim), "%s/notes.md", tmpdir); + f = fopen(victim, "w"); + MU_ASSERT(f != NULL, "create same-basename victim"); + fputs("KEEP", f); + fclose(f); + cfg = make_ws_config(tmpdir, config_path, sizeof(config_path)); + MU_ASSERT(cfg != NULL, "missing-parent: load config"); + tool_file_set_config(cfg); + t = tool_file_get(); + snprintf(nested, sizeof(nested), "%s/missing_dir/notes.md", tmpdir); + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"PWNED\"}", nested); + r = t->execute(args, buf, sizeof(buf)); + MU_ASSERT(r == -1, "write with missing parent is rejected"); + MU_ASSERT(slurp_file(victim, kept, sizeof(kept)) == 0, "workspace notes.md still readable"); + MU_ASSERT(strcmp(kept, "KEEP") == 0, "missing parent does not overwrite same basename"); + config_free(cfg); + unlink(config_path); + unlink(victim); + rmdir(tmpdir); +} + +static void test_file_write_dangling_symlink_rejected(void) +{ + char tmpdir[PATH_MAX]; + char outside[PATH_MAX]; + char link_path[PATH_MAX]; + char config_path[PATH_MAX]; + char args[PATH_MAX + 128]; + char buf[256]; + config_t *cfg; + const tool_t *t; + struct stat st; + int r; + + snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_dangle_%d", (int)getpid()); + if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) return; + snprintf(outside, sizeof(outside), "/tmp/sc_file_pwned_%d", (int)getpid()); + unlink(outside); + snprintf(link_path, sizeof(link_path), "%s/leak", tmpdir); + unlink(link_path); + if (symlink(outside, link_path) != 0) { + rmdir(tmpdir); + return; + } + cfg = make_ws_config(tmpdir, config_path, sizeof(config_path)); + MU_ASSERT(cfg != NULL, "dangling symlink: load config"); + tool_file_set_config(cfg); + t = tool_file_get(); + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"pwned\"}", + link_path); + r = t->execute(args, buf, sizeof(buf)); + MU_ASSERT(r == -1, "write through dangling symlink rejected"); + MU_ASSERT(stat(outside, &st) != 0, "host path outside workspace not created"); + config_free(cfg); + unlink(config_path); + unlink(link_path); + unlink(outside); + rmdir(tmpdir); +} + int main(void) { MU_RUN(test_file_read_write_list); @@ -203,6 +334,9 @@ int main(void) MU_RUN(test_file_outside_workspace_rejected); MU_RUN(test_path_traversal_rejected); MU_RUN(test_symlink_escape_rejected); + MU_RUN(test_write_does_not_truncate_file_used_as_directory); + MU_RUN(test_write_does_not_collapse_missing_parent_onto_basename); + MU_RUN(test_file_write_dangling_symlink_rejected); printf("%d tests run, %d failed\n", tests_run, tests_failed); return tests_failed ? 1 : 0; } From 723cd548aa3f555132e3f9abcb9749807fa05903 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Sereno Date: Mon, 21 Sep 2026 01:29:42 -0300 Subject: [PATCH 2/4] fix(file): persist workspace writes with atomic replace Write to path.tmp, fsync, then rename over the live file so a failed open or write cannot O_TRUNC an existing workspace document. Refs: #78 --- CHANGELOG.md | 1 + src/tools/file.c | 86 +++++++++++++++++++++++++++++++++-------------- tests/test_file.c | 59 ++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c61e4a..863de91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to ShellClaw are documented here. Format follows [Keep a Cha ### Fixed - `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). Dangling workspace symlinks are rejected (`lstat` + `O_NOFOLLOW`) instead of creating host files outside the workspace (#90). +- `write_file` persists via temp+fsync+rename so a failed write cannot wipe an existing workspace file (#78). - Discord Gateway RX grows for the trailing NUL so two 64 KiB libwebsockets fragments cannot write one byte past the heap block (typical READY payloads). - WebChat inbound WS `rx_buffer_size` is `WS_RX_BUFFER_SIZE` (`WS_TEXT_MAX` plus JSON envelope) so dashboard messages are not split across 256-byte RECEIVE callbacks and dropped. - WebChat WebSocket sends now accept agent replies up to 32 KiB (`WS_TEXT_MAX`, matching `RESPONSE_BUF_SIZE`) instead of silently dropping payloads above 8 KiB. Dest buffers are `WS_TEXT_BUF_SIZE` so a max-length payload keeps its NUL; a too-large frame is logged instead of skipped with `<`. diff --git a/src/tools/file.c b/src/tools/file.c index 4219fed..7dcd501 100644 --- a/src/tools/file.c +++ b/src/tools/file.c @@ -13,7 +13,6 @@ #include "core/config.h" #include "cJSON.h" #include -#include #include #include #include @@ -124,30 +123,71 @@ static int resolve_workspace_write_path(const char *path, char *safe_path, size_ return n > 0 && (size_t)n < cap; } -static int open_write_nofollow(const char *safe_path, int ws_only, - char *result_buf, size_t max_len, FILE **out) +static void discard_file_tmp(int fd, char *tmp_path) { + if (fd >= 0) + (void)close(fd); + if (tmp_path) { + unlink(tmp_path); + free(tmp_path); + } +} + +static int write_all(int fd, const char *buf, size_t len) +{ + size_t off = 0; + + while (off < len) { + ssize_t n = write(fd, buf + off, len - off); + if (n <= 0) + return -1; + off += (size_t)n; + } + return 0; +} + +/* + * Temp+rename so O_TRUNC cannot wipe the live workspace file before the + * new bytes are durable (ENOSPC, EFBIG, or a non-writable parent dir). + */ +static int write_file_atomic(const char *path, const char *content, int ws_only) +{ + size_t path_len; + char *tmp_path; int flags = O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC; int fd; - FILE *f; + if (!path || !content) + return -1; + path_len = strlen(path); + tmp_path = malloc(path_len + 8); + if (!tmp_path) + return -1; + snprintf(tmp_path, path_len + 8, "%s.tmp", path); if (ws_only) flags |= O_NOFOLLOW; - fd = open(safe_path, flags, 0644); + fd = open(tmp_path, flags, 0644); if (fd < 0) { - if (ws_only && errno == ELOOP) - snprintf(result_buf, max_len, "{\"error\":\"path outside workspace\"}"); - else - snprintf(result_buf, max_len, "{\"error\":\"cannot write file\"}"); + free(tmp_path); return -1; } - f = fdopen(fd, "w"); - if (!f) { - close(fd); - snprintf(result_buf, max_len, "{\"error\":\"cannot write file\"}"); + if (write_all(fd, content, strlen(content)) != 0) { + discard_file_tmp(fd, tmp_path); + return -1; + } + if (fsync(fd) != 0) { + discard_file_tmp(fd, tmp_path); + return -1; + } + if (close(fd) != 0) { + discard_file_tmp(-1, tmp_path); + return -1; + } + if (rename(tmp_path, path) != 0) { + discard_file_tmp(-1, tmp_path); return -1; } - *out = f; + free(tmp_path); return 0; } @@ -188,30 +228,24 @@ static int file_read(const char *path, char *result_buf, size_t max_len) static int file_write(const char *path, const char *content, char *result_buf, size_t max_len) { char resolved[PATH_MAX]; + int ws_only; + char safe_path[PATH_MAX]; + if (!path_within_workspace(path, resolved, sizeof(resolved))) { snprintf(result_buf, max_len, "{\"error\":\"path outside workspace\"}"); return -1; } - int ws_only = g_file_cfg ? config_workspace_only(g_file_cfg) : 0; - char safe_path[PATH_MAX]; - FILE *f; + ws_only = g_file_cfg ? config_workspace_only(g_file_cfg) : 0; if (!ws_only) { snprintf(safe_path, sizeof(safe_path), "%s", path); } else if (!resolve_workspace_write_path(path, safe_path, sizeof(safe_path))) { snprintf(result_buf, max_len, "{\"error\":\"cannot write file\"}"); return -1; } - if (open_write_nofollow(safe_path, ws_only, result_buf, max_len, &f) != 0) + if (write_file_atomic(safe_path, content ? content : "", ws_only) != 0) { + snprintf(result_buf, max_len, "{\"error\":\"write failed\"}"); return -1; - if (content) { - size_t len = strlen(content); - if (fwrite(content, 1, len, f) != len) { - fclose(f); - snprintf(result_buf, max_len, "{\"error\":\"write failed\"}"); - return -1; - } } - fclose(f); snprintf(result_buf, max_len, "{\"status\":\"ok\"}"); return 0; } diff --git a/tests/test_file.c b/tests/test_file.c index dd4f173..2706d8c 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -327,6 +327,64 @@ static void test_file_write_dangling_symlink_rejected(void) rmdir(tmpdir); } +static void test_file_write_failure_preserves_existing(void) +{ + char tmpdir[PATH_MAX]; + char notes_path[PATH_MAX]; + char config_path[PATH_MAX]; + char args[PATH_MAX + 128]; + char buf[256]; + config_t *cfg; + const tool_t *t; + int write_ret; + + /* + * Directory without write permission: creating path.tmp fails, but + * fopen/open O_TRUNC on the existing file still succeeds. Atomic + * replace must leave the original body intact. + */ + snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_nowrite_%d", (int)getpid()); + if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) return; + snprintf(notes_path, sizeof(notes_path), "%s/notes.md", tmpdir); + cfg = make_ws_config(tmpdir, config_path, sizeof(config_path)); + MU_ASSERT(cfg != NULL, "nowrite: load config"); + tool_file_set_config(cfg); + t = tool_file_get(); + + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"original notes that must survive\"}", + notes_path); + MU_ASSERT(t->execute(args, buf, sizeof(buf)) == 0, "seed original file"); + + MU_ASSERT(chmod(tmpdir, 0555) == 0, "make workspace dir non-writable"); + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"should not land\"}", + notes_path); + write_ret = t->execute(args, buf, sizeof(buf)); + MU_ASSERT(chmod(tmpdir, 0755) == 0, "restore workspace dir mode"); + + MU_ASSERT(write_ret != 0, "write into non-writable dir fails"); + snprintf(args, sizeof(args), "{\"operation\":\"read_file\",\"path\":\"%s\"}", notes_path); + MU_ASSERT(t->execute(args, buf, sizeof(buf)) == 0, "read after failed write"); + MU_ASSERT(strstr(buf, "original notes that must survive") != NULL, + "failed write must not wipe original"); + + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"recovered after chmod\"}", + notes_path); + MU_ASSERT(t->execute(args, buf, sizeof(buf)) == 0, "write recovers after chmod"); + snprintf(args, sizeof(args), "{\"operation\":\"read_file\",\"path\":\"%s\"}", notes_path); + MU_ASSERT(t->execute(args, buf, sizeof(buf)) == 0, "read recovered file"); + MU_ASSERT(strstr(buf, "recovered after chmod") != NULL, "recovered content present"); + + config_free(cfg); + unlink(config_path); + unlink(notes_path); + snprintf(notes_path, sizeof(notes_path), "%s/notes.md.tmp", tmpdir); + unlink(notes_path); + rmdir(tmpdir); +} + int main(void) { MU_RUN(test_file_read_write_list); @@ -337,6 +395,7 @@ int main(void) MU_RUN(test_write_does_not_truncate_file_used_as_directory); MU_RUN(test_write_does_not_collapse_missing_parent_onto_basename); MU_RUN(test_file_write_dangling_symlink_rejected); + MU_RUN(test_file_write_failure_preserves_existing); printf("%d tests run, %d failed\n", tests_run, tests_failed); return tests_failed ? 1 : 0; } From 867008bee110d29c47fa797b6585cf15b78a08f2 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Sereno Date: Mon, 21 Sep 2026 01:30:43 -0300 Subject: [PATCH 3/4] fix(camera): fail closed on empty workspace_path and dangling capture paths workspace_only with an empty root no longer treats every path as allowed. Dangling symlink capture outputs are rejected instead of creating host files outside the workspace. Refs: #91, #90 --- CHANGELOG.md | 1 + src/hardware/hardware_camera.c | 20 ++++++++++-- src/hardware/hardware_camera.h | 10 ++++-- src/tools/hardware_tools.c | 10 ++++-- tests/test_hardware_camera.c | 57 ++++++++++++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 863de91..15bcac1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ All notable changes to ShellClaw are documented here. Format follows [Keep a Cha ### Fixed - `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). Dangling workspace symlinks are rejected (`lstat` + `O_NOFOLLOW`) instead of creating host files outside the workspace (#90). - `write_file` persists via temp+fsync+rename so a failed write cannot wipe an existing workspace file (#78). +- Camera capture fails closed when `workspace_only` is on with an empty `workspace_path`, and rejects dangling symlink outputs (#91, #90). - Discord Gateway RX grows for the trailing NUL so two 64 KiB libwebsockets fragments cannot write one byte past the heap block (typical READY payloads). - WebChat inbound WS `rx_buffer_size` is `WS_RX_BUFFER_SIZE` (`WS_TEXT_MAX` plus JSON envelope) so dashboard messages are not split across 256-byte RECEIVE callbacks and dropped. - WebChat WebSocket sends now accept agent replies up to 32 KiB (`WS_TEXT_MAX`, matching `RESPONSE_BUF_SIZE`) instead of silently dropping payloads above 8 KiB. Dest buffers are `WS_TEXT_BUF_SIZE` so a max-length payload keeps its NUL; a too-large frame is logged instead of skipped with `<`. diff --git a/src/hardware/hardware_camera.c b/src/hardware/hardware_camera.c index 5fbf95d..f7e57f9 100644 --- a/src/hardware/hardware_camera.c +++ b/src/hardware/hardware_camera.c @@ -32,6 +32,8 @@ typedef enum camera_cli_kind { static int s_camera_ready; static char s_workspace[PATH_MAX]; +/** Non-zero when workspace_only is on (set_workspace with non-NULL). */ +static int s_workspace_enforced; static hardware_camera_spawn_fn s_test_spawn; static char *s_last_argv[HARDWARE_CAMERA_ARGV_MAX]; static char s_last_argv_storage[HARDWARE_CAMERA_ARGV_MAX][ARG_BUF_SZ]; @@ -108,13 +110,20 @@ static int path_inside_workspace(const char *path) char ws_resolved[PATH_MAX]; char resolved[PATH_MAX]; char path_copy[PATH_MAX]; + struct stat lst; - if (s_workspace[0] == '\0' || !path || path[0] == '\0') + /* workspace_only off (set_workspace(NULL)): no containment. */ + if (!s_workspace_enforced) return 1; + /* Enforced but missing/empty root or empty path: deny (file.c parity). */ + if (s_workspace[0] == '\0' || !path || path[0] == '\0') + return 0; if (realpath(s_workspace, ws_resolved) == NULL) return 0; if (realpath(path, resolved) != NULL) return resolved_under_workspace(resolved, ws_resolved); + if (lstat(path, &lst) == 0 && S_ISLNK(lst.st_mode)) + return 0; snprintf(path_copy, sizeof(path_copy), "%s", path); for (;;) { char *dir = dirname(path_copy); @@ -417,7 +426,13 @@ int hardware_camera_init(void) void hardware_camera_set_workspace(const char *workspace) { - if (!workspace || workspace[0] == '\0') { + if (!workspace) { + s_workspace[0] = '\0'; + s_workspace_enforced = 0; + return; + } + s_workspace_enforced = 1; + if (workspace[0] == '\0') { s_workspace[0] = '\0'; return; } @@ -436,6 +451,7 @@ void hardware_camera_shutdown(void) s_test_spawn = NULL; s_camera_ready = 0; s_workspace[0] = '\0'; + s_workspace_enforced = 0; s_spawn_timeout_ms = HARDWARE_CAMERA_SPAWN_TIMEOUT_MS; s_last_argv_count = 0; memset(s_last_argv, 0, sizeof(s_last_argv)); diff --git a/src/hardware/hardware_camera.h b/src/hardware/hardware_camera.h index 48bb014..67156f6 100644 --- a/src/hardware/hardware_camera.h +++ b/src/hardware/hardware_camera.h @@ -51,14 +51,18 @@ int hardware_camera_capture(board_id_t board, const char *camera_type, /** * Bind the file-tool workspace root used for caller-supplied capture paths. - * NULL or empty disables the check (auto temp files and unit tests). + * NULL disables containment (workspace_only off). Non-NULL enables enforcement; + * an empty string fails closed (deny caller paths) — same as write_file when + * workspace_path is missing/empty under workspace_only. + * + * Example: hardware_camera_set_workspace("") denies /tmp/out.jpg; NULL allows it. */ void hardware_camera_set_workspace(const char *workspace); /** * Return 1 if @p path may be used as a caller-supplied capture output. - * Auto temp (NULL/empty) is always allowed. When a workspace is bound, - * the path must resolve under that root (same policy as write_file). + * Auto temp (NULL/empty) is always allowed. When enforcement is on, the path + * must resolve under the bound root (same policy as write_file). */ int hardware_camera_output_allowed(const char *path); diff --git a/src/tools/hardware_tools.c b/src/tools/hardware_tools.c index 1667e15..9289be9 100644 --- a/src/tools/hardware_tools.c +++ b/src/tools/hardware_tools.c @@ -100,10 +100,14 @@ static const size_t HARDWARE_TOOL_COUNT = void tool_hardware_set_config(const config_t *cfg) { g_hw_cfg = cfg; - if (cfg && config_workspace_only(cfg)) - hardware_camera_set_workspace(config_workspace_path(cfg)); - else + if (cfg && config_workspace_only(cfg)) { + const char *ws = config_workspace_path(cfg); + + /* Pass "" when path is missing so camera fails closed like write_file. */ + hardware_camera_set_workspace(ws ? ws : ""); + } else { hardware_camera_set_workspace(NULL); + } } size_t tool_hardware_get_all(const tool_t **out, size_t max_count) diff --git a/tests/test_hardware_camera.c b/tests/test_hardware_camera.c index 3e27175..46fb2c1 100644 --- a/tests/test_hardware_camera.c +++ b/tests/test_hardware_camera.c @@ -7,6 +7,7 @@ #include "hardware/hardware_camera.h" #include #include +#include #include static char s_spawn_out_path[256]; @@ -214,6 +215,31 @@ static int test_output_path_outside_workspace_rejected(void) return 0; } +static int test_empty_workspace_enforced_denies_outside(void) +{ + char result[256]; + char err[128]; + const char *outside = "/tmp/shellclaw_cam_empty_ws_escape.jpg"; + + unlink(outside); + /* Non-NULL empty string: workspace_only on, path missing/empty. */ + hardware_camera_set_workspace(""); + ASSERT(hardware_camera_output_allowed(outside) == 0); + RUN(setup_mock()); + ASSERT(hardware_camera_capture(BOARD_JETSON_ORIN_NANO, "csi", "640x480", 75, 0, 0, + outside, result, sizeof(result), err, + sizeof(err)) == -1); + ASSERT(strstr(err, "workspace") != NULL); + ASSERT(s_spawn_called == 0); + ASSERT(access(outside, F_OK) != 0); + teardown(); + hardware_camera_init(); + hardware_camera_set_workspace(NULL); + ASSERT(hardware_camera_output_allowed(outside) == 1); + hardware_camera_shutdown(); + return 0; +} + static int test_output_path_inside_workspace_allowed(void) { char result[256]; @@ -234,6 +260,35 @@ static int test_output_path_inside_workspace_allowed(void) return 0; } +static int test_output_dangling_symlink_rejected(void) +{ + char result[256]; + char err[128]; + char ws[128]; + char link_path[256]; + char outside[256]; + struct stat st; + + ASSERT(test_runner_mkdtemp_path("shellclaw_cam_dangle", ws, sizeof(ws)) == 0); + snprintf(outside, sizeof(outside), "/tmp/sc_cam_pwned_%d.jpg", (int)getpid()); + unlink(outside); + snprintf(link_path, sizeof(link_path), "%s/shot.jpg", ws); + ASSERT(symlink(outside, link_path) == 0); + hardware_camera_set_workspace(ws); + RUN(setup_mock()); + ASSERT(hardware_camera_capture(BOARD_JETSON_ORIN_NANO, "csi", "640x480", 75, 0, 0, + link_path, result, sizeof(result), err, + sizeof(err)) == -1); + ASSERT(strstr(err, "workspace") != NULL); + ASSERT(s_spawn_called == 0); + ASSERT(stat(outside, &st) != 0); + teardown(); + unlink(link_path); + unlink(outside); + rmdir(ws); + return 0; +} + static int test_output_path_traversal_rejected(void) { char result[256]; @@ -605,7 +660,9 @@ int main(void) RUN(test_capture_argument_validation()); RUN(test_unsafe_output_path_rejected()); RUN(test_output_path_outside_workspace_rejected()); + RUN(test_empty_workspace_enforced_denies_outside()); RUN(test_output_path_inside_workspace_allowed()); + RUN(test_output_dangling_symlink_rejected()); RUN(test_output_path_traversal_rejected()); RUN(test_resolution_injection_rejected()); RUN(test_camera_type_injection_rejected()); From 088ef193c61ed5495cc85de23c7853953cc056b0 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Sereno Date: Mon, 21 Sep 2026 02:33:52 -0300 Subject: [PATCH 4/4] fix(file): unique write sidecar and honest leaf-symlink reject Use mkstemp in the parent directory so write_file cannot O_TRUNC a sibling path.tmp. Rename the helper and changelog to match fail-closed rejection of every leaf symlink, including in-workspace aliases. Refs: #94 --- CHANGELOG.md | 6 +-- src/tools/file.c | 56 +++++++++++++++------------- tests/test_file.c | 93 +++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 123 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15bcac1..b43fa0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,9 @@ All notable changes to ShellClaw are documented here. Format follows [Keep a Cha ## [Unreleased] ### Fixed -- `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). Dangling workspace symlinks are rejected (`lstat` + `O_NOFOLLOW`) instead of creating host files outside the workspace (#90). -- `write_file` persists via temp+fsync+rename so a failed write cannot wipe an existing workspace file (#78). -- Camera capture fails closed when `workspace_only` is on with an empty `workspace_path`, and rejects dangling symlink outputs (#91, #90). +- `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). - Discord Gateway RX grows for the trailing NUL so two 64 KiB libwebsockets fragments cannot write one byte past the heap block (typical READY payloads). - WebChat inbound WS `rx_buffer_size` is `WS_RX_BUFFER_SIZE` (`WS_TEXT_MAX` plus JSON envelope) so dashboard messages are not split across 256-byte RECEIVE callbacks and dropped. - WebChat WebSocket sends now accept agent replies up to 32 KiB (`WS_TEXT_MAX`, matching `RESPONSE_BUF_SIZE`) instead of silently dropping payloads above 8 KiB. Dest buffers are `WS_TEXT_BUF_SIZE` so a max-length payload keeps its NUL; a too-large frame is logged instead of skipped with `<`. diff --git a/src/tools/file.c b/src/tools/file.c index 7dcd501..2fd08dd 100644 --- a/src/tools/file.c +++ b/src/tools/file.c @@ -51,7 +51,7 @@ static int resolved_is_under_workspace(const char *resolved) return 1; } -static int path_is_dangling_symlink(const char *path) +static int path_is_symlink(const char *path) { struct stat lst; @@ -75,8 +75,8 @@ static int path_within_workspace(const char *path, char *resolved, size_t resolv return 0; if (realpath(path, resolved) != NULL) return resolved_is_under_workspace(resolved); - /* Dangling symlink: ancestor prefix is not enough — open() would follow it. */ - if (path_is_dangling_symlink(path)) + /* Leaf symlink: ancestor prefix is not enough — open() would follow it. */ + if (path_is_symlink(path)) return 0; snprintf(path_copy, sizeof(path_copy), "%s", path); for (;;) { @@ -103,7 +103,7 @@ static int resolve_workspace_write_path(const char *path, char *safe_path, size_ char *dir; int n; - if (path_is_dangling_symlink(path)) + if (path_is_symlink(path)) return 0; if (realpath(path, safe_path) != NULL) { if (stat(safe_path, &st) != 0 || !S_ISREG(st.st_mode)) return 0; @@ -123,14 +123,12 @@ static int resolve_workspace_write_path(const char *path, char *safe_path, size_ return n > 0 && (size_t)n < cap; } -static void discard_file_tmp(int fd, char *tmp_path) +static void discard_file_tmp(int fd, const char *tmp_path) { if (fd >= 0) (void)close(fd); - if (tmp_path) { - unlink(tmp_path); - free(tmp_path); - } + if (tmp_path && tmp_path[0] != '\0') + (void)unlink(tmp_path); } static int write_all(int fd, const char *buf, size_t len) @@ -147,28 +145,35 @@ static int write_all(int fd, const char *buf, size_t len) } /* - * Temp+rename so O_TRUNC cannot wipe the live workspace file before the - * new bytes are durable (ENOSPC, EFBIG, or a non-writable parent dir). + * Unique temp+rename so O_TRUNC cannot wipe the live file, and a sibling + * named path.tmp is not used as the sidecar (ENOSPC, EFBIG, or a + * non-writable parent dir). mkstemp uses O_EXCL, so a planted symlink at + * the random name cannot be followed (the path.tmp #90 shape). */ -static int write_file_atomic(const char *path, const char *content, int ws_only) +static int write_file_atomic(const char *path, const char *content) { - size_t path_len; - char *tmp_path; - int flags = O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC; + char path_copy[PATH_MAX]; + char tmp_path[PATH_MAX]; + char *dir; int fd; + int n; if (!path || !content) return -1; - path_len = strlen(path); - tmp_path = malloc(path_len + 8); - if (!tmp_path) + if (snprintf(path_copy, sizeof(path_copy), "%s", path) >= (int)sizeof(path_copy)) + return -1; + dir = dirname(path_copy); + if (!dir || dir[0] == '\0') + return -1; + n = snprintf(tmp_path, sizeof(tmp_path), "%s/.sc-write-XXXXXX", dir); + if (n < 0 || (size_t)n >= sizeof(tmp_path)) return -1; - snprintf(tmp_path, path_len + 8, "%s.tmp", path); - if (ws_only) - flags |= O_NOFOLLOW; - fd = open(tmp_path, flags, 0644); - if (fd < 0) { - free(tmp_path); + fd = mkstemp(tmp_path); + if (fd < 0) + return -1; + (void)fcntl(fd, F_SETFD, FD_CLOEXEC); + if (fchmod(fd, 0644) != 0) { + discard_file_tmp(fd, tmp_path); return -1; } if (write_all(fd, content, strlen(content)) != 0) { @@ -187,7 +192,6 @@ static int write_file_atomic(const char *path, const char *content, int ws_only) discard_file_tmp(-1, tmp_path); return -1; } - free(tmp_path); return 0; } @@ -242,7 +246,7 @@ static int file_write(const char *path, const char *content, char *result_buf, s snprintf(result_buf, max_len, "{\"error\":\"cannot write file\"}"); return -1; } - if (write_file_atomic(safe_path, content ? content : "", ws_only) != 0) { + if (write_file_atomic(safe_path, content ? content : "") != 0) { snprintf(result_buf, max_len, "{\"error\":\"write failed\"}"); return -1; } diff --git a/tests/test_file.c b/tests/test_file.c index 2706d8c..a21bb2e 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -339,9 +339,9 @@ static void test_file_write_failure_preserves_existing(void) int write_ret; /* - * Directory without write permission: creating path.tmp fails, but - * fopen/open O_TRUNC on the existing file still succeeds. Atomic - * replace must leave the original body intact. + * Directory without write permission: creating the mkstemp sidecar + * fails, but fopen/open O_TRUNC on the existing file still succeeds. + * Atomic replace must leave the original body intact. */ snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_nowrite_%d", (int)getpid()); if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) return; @@ -385,6 +385,91 @@ static void test_file_write_failure_preserves_existing(void) rmdir(tmpdir); } +static void test_write_does_not_clobber_sibling_tmp(void) +{ + char tmpdir[PATH_MAX]; + char notes_path[PATH_MAX]; + char sibling_tmp[PATH_MAX]; + char config_path[PATH_MAX]; + char args[PATH_MAX + 128]; + char buf[256]; + char kept[64]; + config_t *cfg; + const tool_t *t; + FILE *f; + + snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_sibtmp_%d", (int)getpid()); + if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) return; + snprintf(notes_path, sizeof(notes_path), "%s/notes.md", tmpdir); + snprintf(sibling_tmp, sizeof(sibling_tmp), "%s/notes.md.tmp", tmpdir); + f = fopen(sibling_tmp, "w"); + MU_ASSERT(f != NULL, "create sibling notes.md.tmp"); + fputs("SIBLING KEEP", f); + fclose(f); + cfg = make_ws_config(tmpdir, config_path, sizeof(config_path)); + MU_ASSERT(cfg != NULL, "sibling tmp: load config"); + tool_file_set_config(cfg); + t = tool_file_get(); + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"NEW NOTES\"}", + notes_path); + MU_ASSERT(t->execute(args, buf, sizeof(buf)) == 0, "write notes.md succeeds"); + MU_ASSERT(slurp_file(notes_path, kept, sizeof(kept)) == 0, "notes.md readable"); + MU_ASSERT(strcmp(kept, "NEW NOTES") == 0, "notes.md has new content"); + MU_ASSERT(slurp_file(sibling_tmp, kept, sizeof(kept)) == 0, "sibling tmp still readable"); + MU_ASSERT(strcmp(kept, "SIBLING KEEP") == 0, "write must not O_TRUNC notes.md.tmp"); + config_free(cfg); + unlink(config_path); + unlink(notes_path); + unlink(sibling_tmp); + rmdir(tmpdir); +} + +static void test_write_in_workspace_symlink_alias_rejected(void) +{ + char tmpdir[PATH_MAX]; + char notes_path[PATH_MAX]; + char alias_path[PATH_MAX]; + char config_path[PATH_MAX]; + char args[PATH_MAX + 128]; + char buf[256]; + char kept[64]; + config_t *cfg; + const tool_t *t; + FILE *f; + + snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_alias_%d", (int)getpid()); + if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) return; + snprintf(notes_path, sizeof(notes_path), "%s/notes.md", tmpdir); + snprintf(alias_path, sizeof(alias_path), "%s/alias.md", tmpdir); + f = fopen(notes_path, "w"); + MU_ASSERT(f != NULL, "create notes.md"); + fputs("KEEP", f); + fclose(f); + unlink(alias_path); + if (symlink(notes_path, alias_path) != 0 && symlink("notes.md", alias_path) != 0) { + unlink(notes_path); + rmdir(tmpdir); + return; + } + cfg = make_ws_config(tmpdir, config_path, sizeof(config_path)); + MU_ASSERT(cfg != NULL, "alias: load config"); + tool_file_set_config(cfg); + t = tool_file_get(); + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"PWNED\"}", + alias_path); + MU_ASSERT(t->execute(args, buf, sizeof(buf)) == -1, + "write through in-workspace symlink alias is rejected"); + MU_ASSERT(slurp_file(notes_path, kept, sizeof(kept)) == 0, "notes.md still readable"); + MU_ASSERT(strcmp(kept, "KEEP") == 0, "alias write must not change notes.md"); + config_free(cfg); + unlink(config_path); + unlink(alias_path); + unlink(notes_path); + rmdir(tmpdir); +} + int main(void) { MU_RUN(test_file_read_write_list); @@ -396,6 +481,8 @@ int main(void) MU_RUN(test_write_does_not_collapse_missing_parent_onto_basename); MU_RUN(test_file_write_dangling_symlink_rejected); MU_RUN(test_file_write_failure_preserves_existing); + MU_RUN(test_write_does_not_clobber_sibling_tmp); + MU_RUN(test_write_in_workspace_symlink_alias_rejected); printf("%d tests run, %d failed\n", tests_run, tests_failed); return tests_failed ? 1 : 0; }