From 688a549e7c24be08f84cf342e0fc5d3b468e5482 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 15 Sep 2026 11:31:20 +0000 Subject: [PATCH] fix(file): reject dangling-symlink writes outside workspace write_file treated a dangling workspace symlink as "ancestor is in bounds" and fopen("w") followed it onto the host. Camera capture used the same ancestor walk. Open with O_NOFOLLOW and fail closed on unresolvable links. Co-authored-by: esadrianno --- CHANGELOG.md | 1 + src/hardware/hardware_camera.c | 5 ++++ src/tools/file.c | 33 +++++++++++++++++++++--- tests/test_file.c | 46 ++++++++++++++++++++++++++++++++++ tests/test_hardware_camera.c | 31 +++++++++++++++++++++++ 5 files changed, 112 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 901fb38..3729341 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ All notable changes to ShellClaw are documented here. Format follows [Keep a Cha ### Security - Camera auto-output keeps the exclusive `mkstemp` inode (no unlink + `${tmpl}.jpg` sibling). - Reject I2C `bus` outside 0–255 at the tool JSON boundary. +- `write_file` and camera capture reject dangling workspace symlinks (`O_NOFOLLOW` / `lstat`) so they cannot create host files outside the workspace. --- diff --git a/src/hardware/hardware_camera.c b/src/hardware/hardware_camera.c index 5fbf95d..03272da 100644 --- a/src/hardware/hardware_camera.c +++ b/src/hardware/hardware_camera.c @@ -115,6 +115,11 @@ static int path_inside_workspace(const char *path) return 0; if (realpath(path, resolved) != NULL) return resolved_under_workspace(resolved, ws_resolved); + { + struct stat lst; + 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); diff --git a/src/tools/file.c b/src/tools/file.c index e8170b1..a433b07 100644 --- a/src/tools/file.c +++ b/src/tools/file.c @@ -10,6 +10,8 @@ #include "core/config.h" #include "cJSON.h" #include +#include +#include #include #include #include @@ -50,6 +52,13 @@ static int path_within_workspace(const char *path, char *resolved, size_t resolv if (resolved[ws_len] != '\0' && resolved[ws_len] != '/') return 0; return 1; } + { + struct stat lst; + /* Dangling (or otherwise unresolvable) symlink: do not treat the + * workspace ancestor as sufficient — fopen/open would follow it. */ + if (lstat(path, &lst) == 0 && S_ISLNK(lst.st_mode)) + return 0; + } char path_copy[PATH_MAX]; snprintf(path_copy, sizeof(path_copy), "%s", path); for (;;) { @@ -104,6 +113,7 @@ 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 { @@ -124,10 +134,25 @@ static int file_write(const char *path, const char *content, char *result_buf, s memcpy(safe_path + res_len + 1, base, base_len + 1); } } - FILE *f = fopen(safe_path, "w"); - if (!f) { - snprintf(result_buf, max_len, "{\"error\":\"cannot write file\"}"); - return -1; + { + int flags = O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC; + int fd; + 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; + } } if (content) { size_t len = strlen(content); diff --git a/tests/test_file.c b/tests/test_file.c index b4087ac..ff2786e 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -196,6 +196,51 @@ static void test_symlink_escape_rejected(void) rmdir(tmpdir); } +/* + * write_file used the first existing ancestor when realpath failed. A dangling + * symlink in the workspace therefore passed the prefix check, and fopen("w") + * followed it and created/truncated a host file outside the workspace. + */ +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 +248,7 @@ 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_file_write_dangling_symlink_rejected); printf("%d tests run, %d failed\n", tests_run, tests_failed); return tests_failed ? 1 : 0; } diff --git a/tests/test_hardware_camera.c b/tests/test_hardware_camera.c index 3e27175..6ab41b6 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]; @@ -234,6 +235,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]; @@ -606,6 +636,7 @@ int main(void) RUN(test_unsafe_output_path_rejected()); RUN(test_output_path_outside_workspace_rejected()); 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());