diff --git a/CHANGELOG.md b/CHANGELOG.md index 41bf6992..0df31cf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,14 @@ loosely while pre-1.0 (breaking changes can land on minor bumps). ## [Unreleased] -### Fixed +- **MCP registry writes do not follow a planted `.tmp` symlink.** + `save_server_registry` opened `.tmp` with `O_CREAT|O_TRUNC`, so + a symlink at that staging name redirected the write (including + registry `env` secrets) into the link target before `rename` replaced + only the symlink. Open the staging file with `O_NOFOLLOW` / `O_EXCL` + after `unlink` (which does not follow). Dest-symlink `rename` already + replaced the link, not its target. - **`/schedule` calendar dates.** `on YYYY-MM-DD` now rejects impossible - dates (Feb 31, Jun 31, Feb 29 in a non-leap year) instead of letting - `mktime` overflow into the next month and fire on a different day. - **MCP string JSON-RPC ids.** `parse_response` now accepts a decimal-string - **Reconcile rollback no longer wipes the workspace on a failed restore.** - **LaTeX math recursion depth.** `latex_math_to_plain` now stops converting diff --git a/include/mcp/manager.h b/include/mcp/manager.h index c187de08..69a44512 100644 --- a/include/mcp/manager.h +++ b/include/mcp/manager.h @@ -52,7 +52,10 @@ std::vector load_server_registry(const std::string& path); std::string serialize_server_registry(const std::vector& specs); // Atomically write the registry to `path` (mode 0600). Returns false on -// I/O failure. Empty specs writes `{ "servers": {} }`. +// I/O failure. Empty specs writes `{ "servers": {} }`. The staging +// name is opened with O_NOFOLLOW / O_EXCL after unlink (which does not +// follow) so a planted `.tmp` symlink cannot redirect the write +// — registry env blocks may hold secrets. bool save_server_registry(const std::string& path, const std::vector& specs); diff --git a/src/mcp/manager.cpp b/src/mcp/manager.cpp index e47512c7..7e0d55c7 100644 --- a/src/mcp/manager.cpp +++ b/src/mcp/manager.cpp @@ -3,6 +3,7 @@ #include "mcp/manager.h" #include +#include #include #include #include @@ -189,8 +190,17 @@ bool save_server_registry(const std::string& path, // Write tmp with mode 0600 up front — registry env blocks may hold // secrets, and umask-default create + later chmod leaves a window. + // + // Drop a leftover crash file or a planted name first. unlink(2) + // removes a symlink itself, so this cannot clobber the target of + // `.tmp` → elsewhere. open(O_CREAT|O_TRUNC) used to follow + // that link and write secrets into the target before rename + // replaced only the symlink. const std::string tmp = path + ".tmp"; - const int fd = ::open(tmp.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0600); + ::unlink(tmp.c_str()); + const int fd = ::open(tmp.c_str(), + O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC, + 0600); if (fd < 0) return false; // open(2) mode is masked by umask — force 0600 before any bytes land. if (::fchmod(fd, 0600) != 0) { @@ -202,8 +212,14 @@ bool save_server_registry(const std::string& path, bool ok = true; while (ok && off < body.size()) { const ssize_t n = ::write(fd, body.data() + off, body.size() - off); - if (n <= 0) ok = false; - else off += static_cast(n); + if (n < 0) { + if (errno == EINTR) continue; + ok = false; + } else if (n == 0) { + ok = false; + } else { + off += static_cast(n); + } } if (ok) ok = (::fsync(fd) == 0); ::close(fd); diff --git a/tests/test_mcp.cpp b/tests/test_mcp.cpp index e15ebad1..6e493059 100644 --- a/tests/test_mcp.cpp +++ b/tests/test_mcp.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -836,6 +837,101 @@ TEST_CASE("save_server_registry rejects empty argv without throwing") { CHECK_FALSE(save_server_registry("/tmp/arbiter-mcp-should-not-exist.json", {s})); } +TEST_CASE("save_server_registry does not follow a planted path.tmp symlink") { + namespace fs = std::filesystem; + const auto dir = fs::temp_directory_path() + / ("arbiter-mcp-tmp-link-" + std::to_string(::getpid())); + fs::create_directories(dir); + const auto path = (dir / "mcp_servers.json").string(); + const auto victim = (dir / "secret.txt").string(); + { + std::ofstream f(victim); + f << "do-not-clobber"; + } + fs::create_symlink(victim, path + ".tmp"); + + ServerSpec s; + s.name = "playwright"; + s.argv = {"npx"}; + s.env_extra = {"TOKEN=super-secret"}; + REQUIRE(save_server_registry(path, {s})); + + { + std::ifstream f(victim); + std::ostringstream buf; + buf << f.rdbuf(); + CHECK(buf.str() == "do-not-clobber"); + } + CHECK(fs::is_regular_file(path)); + CHECK_FALSE(fs::is_symlink(path)); + CHECK_FALSE(fs::exists(path + ".tmp")); + struct stat st{}; + REQUIRE(::stat(path.c_str(), &st) == 0); + CHECK((st.st_mode & 0777) == 0600); + auto loaded = load_server_registry(path); + REQUIRE(loaded.size() == 1); + REQUIRE(loaded[0].env_extra.size() == 1); + CHECK(loaded[0].env_extra[0] == "TOKEN=super-secret"); + + fs::remove_all(dir); +} + +TEST_CASE("save_server_registry replaces a leftover regular tmp") { + namespace fs = std::filesystem; + const auto dir = fs::temp_directory_path() + / ("arbiter-mcp-tmp-stale-" + std::to_string(::getpid())); + fs::create_directories(dir); + const auto path = (dir / "mcp_servers.json").string(); + { + std::ofstream f(path + ".tmp"); + f << "stale crash leftover"; + } + + ServerSpec s; + s.name = "ok"; + s.argv = {"npx"}; + REQUIRE(save_server_registry(path, {s})); + CHECK_FALSE(fs::exists(path + ".tmp")); + auto loaded = load_server_registry(path); + REQUIRE(loaded.size() == 1); + CHECK(loaded[0].name == "ok"); + + fs::remove_all(dir); +} + +TEST_CASE("save_server_registry rename replaces a dest symlink instead of writing through it") { + namespace fs = std::filesystem; + const auto dir = fs::temp_directory_path() + / ("arbiter-mcp-dest-link-" + std::to_string(::getpid())); + fs::create_directories(dir); + const auto path = (dir / "mcp_servers.json").string(); + const auto victim = (dir / "outside.txt").string(); + { + std::ofstream f(victim); + f << "keep-me"; + } + fs::create_symlink(victim, path); + + ServerSpec s; + s.name = "ok"; + s.argv = {"npx"}; + REQUIRE(save_server_registry(path, {s})); + + { + std::ifstream f(victim); + std::ostringstream buf; + buf << f.rdbuf(); + CHECK(buf.str() == "keep-me"); + } + CHECK(fs::is_regular_file(path)); + CHECK_FALSE(fs::is_symlink(path)); + auto loaded = load_server_registry(path); + REQUIRE(loaded.size() == 1); + CHECK(loaded[0].name == "ok"); + + fs::remove_all(dir); +} + TEST_CASE("serialize_server_registry escapes quotes in names and args") { ServerSpec s; s.name = "weird\"name";