diff --git a/CHANGELOG.md b/CHANGELOG.md index ac72d9d5..d8f45600 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ loosely while pre-1.0 (breaking changes can land on minor bumps). ## [Unreleased] ### Fixed +- **Secret key/token writes do not follow a planted dest symlink.** + `write_key_file` and admin-token generate opened + `~/.arbiter/{openrouter_api_key,search_api_key,admin_token}` with + `O_CREAT|O_TRUNC`, so a dest symlink redirected the secret into the + link target. Open with `O_NOFOLLOW` and require a regular file so a + planted symlink or FIFO cannot exfiltrate the bytes. - **Remote `--connect` base URL query/userinfo.** `normalize_api_base_url` now rejects query strings, fragments, URL userinfo, and control bytes instead of concatenating them into every request (`https://host?x/v1/…` diff --git a/CMakeLists.txt b/CMakeLists.txt index 5474042b..476e0e21 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -169,6 +169,7 @@ set(ARBITER_SOURCES src/remote/api_client.cpp src/remote/sse_turn.cpp src/atomic_file.cpp + src/secret_file.cpp src/loop_manager.cpp src/title_generator.cpp src/cli.cpp @@ -1394,6 +1395,17 @@ if(INDEX_BUILD_TESTS) ) add_test(NAME unit_artifacts COMMAND unit_artifacts) + # Secret dest writes (API keys / admin token): O_NOFOLLOW + regular file. + add_executable(unit_secret_file + tests/test_secret_file.cpp + src/secret_file.cpp + ) + target_include_directories(unit_secret_file PRIVATE + ${CMAKE_SOURCE_DIR}/include + ${CMAKE_SOURCE_DIR}/third_party/doctest + ) + add_test(NAME unit_secret_file COMMAND unit_secret_file) + # MCP layer (JSON-RPC framing, subprocess wrapper, /mcp dispatch). # Live-server tests are intentionally not wired here — manual smoke # via `arbiter --api` covers the playwright path. diff --git a/include/secret_file.h b/include/secret_file.h new file mode 100644 index 00000000..baa4f70c --- /dev/null +++ b/include/secret_file.h @@ -0,0 +1,17 @@ +#pragma once +// arbiter/include/secret_file.h — owner-only dest writes that do not +// follow a planted symlink or other non-regular dest. +// +// Used for ~/.arbiter API-key files and the generated admin token. + +#include + +namespace arbiter { + +// Write `data` to `path` at mode 0600. Opens with O_NOFOLLOW so a +// dest symlink cannot redirect the bytes into its target, and refuses +// after open unless the fd is a regular file (FIFO / device / dir). +// Returns false on I/O failure or a non-regular dest. +bool write_secret_file(const std::string& path, const std::string& data); + +} // namespace arbiter diff --git a/src/cli.cpp b/src/cli.cpp index cc0f9a09..1e1816cc 100644 --- a/src/cli.cpp +++ b/src/cli.cpp @@ -7,6 +7,7 @@ #include "logger.h" #include "orchestrator.h" #include "sandbox.h" +#include "secret_file.h" #include "starters.h" #include "tenant_store.h" #include "tui/tui_design.h" @@ -29,7 +30,6 @@ #include #include -#include #include #include @@ -50,7 +50,8 @@ void signal_handler(int) { g_running = 0; } // tenant tokens. Resolution order on every `arbiter --api` startup: // 1. $ARBITER_ADMIN_TOKEN (overrides the file) // 2. ~/.arbiter/admin_token (file) (owner-only; re-tightens perms) -// 3. generate a fresh one, write it to #2 at mode 0600, print at startup +// 3. generate a fresh one, write it to #2 at mode 0600 (O_NOFOLLOW), +// print at startup // // A `true` in `freshly_generated` tells the caller to announce the new // token prominently — this is the only time the operator sees the @@ -99,20 +100,11 @@ std::string resolve_admin_token(const std::string& config_dir, static const char* hex = "0123456789abcdef"; for (unsigned char c : buf) { tok += hex[c >> 4]; tok += hex[c & 0xF]; } - const int fd = ::open(path.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0600); - if (fd < 0) { + if (!write_secret_file(path, tok + "\n")) { std::cerr << "ERR: could not write " << path << ": " << std::strerror(errno) << "\n"; std::exit(1); } - const std::string out = tok + "\n"; - ssize_t w = ::write(fd, out.data(), out.size()); - ::close(fd); - ::chmod(path.c_str(), 0600); - if (w != static_cast(out.size())) { - std::cerr << "ERR: short write to " << path << "\n"; - std::exit(1); - } freshly_generated = true; return tok; } diff --git a/src/cli_helpers.cpp b/src/cli_helpers.cpp index 3a99c02d..0f2c6b0a 100644 --- a/src/cli_helpers.cpp +++ b/src/cli_helpers.cpp @@ -4,6 +4,7 @@ #include "commands.h" #include "mcp/manager.h" #include "model_catalog.h" +#include "secret_file.h" #include "starters.h" #include "theme.h" #include "tui/opentui/engine.h" @@ -659,26 +660,11 @@ std::string read_secret_line() { return line; } -// Write the key to ~/.arbiter/ with mode 0600. Opens with the -// restrictive mode upfront so the file is never world-readable in between. +// Write the key to ~/.arbiter/ with mode 0600. Opens with +// O_NOFOLLOW so a planted dest symlink cannot exfiltrate the key. bool write_key_file(const std::string& filename, const std::string& key) { const std::string path = get_config_dir() + "/" + filename; - const int fd = ::open(path.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0600); - if (fd < 0) return false; - // open(2) mode is masked by umask — fchmod so the key is never - // briefly group/world-readable. - if (::fchmod(fd, 0600) != 0) { ::close(fd); return false; } - size_t off = 0; - while (off < key.size()) { - const ssize_t n = ::write(fd, key.data() + off, key.size() - off); - if (n <= 0) { ::close(fd); return false; } - off += static_cast(n); - } - ::write(fd, "\n", 1); - ::close(fd); - // Re-apply mode in case the file pre-existed with wider perms. - ::chmod(path.c_str(), 0600); - return true; + return write_secret_file(path, key + "\n"); } // Read a trimmed line. Exits(1) on EOF/Ctrl-D so the wizard can't spin. diff --git a/src/secret_file.cpp b/src/secret_file.cpp new file mode 100644 index 00000000..6e74dd00 --- /dev/null +++ b/src/secret_file.cpp @@ -0,0 +1,54 @@ +#include "secret_file.h" + +#include +#include +#include +#include + +namespace arbiter { + +bool write_secret_file(const std::string& path, const std::string& data) { + // O_NOFOLLOW: a planted dest symlink used to send API keys / adm_ + // tokens into the link target (open(O_CREAT|O_TRUNC) follows). + // O_NONBLOCK: a planted FIFO must not hang the wizard / --api + // startup waiting for a reader — and must not deliver the secret + // if a reader is already attached. + const int fd = ::open(path.c_str(), + O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW | + O_CLOEXEC | O_NONBLOCK, + 0600); + if (fd < 0) return false; + + auto fail = [fd](int err = 0) { + if (err != 0) errno = err; + const int saved = errno; + ::close(fd); + errno = saved; + return false; + }; + + struct stat st{}; + if (::fstat(fd, &st) != 0) return fail(); + if (!S_ISREG(st.st_mode)) return fail(EINVAL); + + // open(2) mode is masked by umask — fchmod so the secret is never + // briefly group/world-readable. Path chmod is not used: it follows + // a dest that is swapped for a symlink after open. + if (::fchmod(fd, 0600) != 0) return fail(); + + size_t off = 0; + while (off < data.size()) { + const ssize_t n = ::write(fd, data.data() + off, data.size() - off); + if (n < 0) { + if (errno == EINTR) continue; + return fail(); + } + if (n == 0) return fail(EIO); + off += static_cast(n); + } + if (::fsync(fd) != 0) return fail(); + ::close(fd); + return true; +} + +} // namespace arbiter diff --git a/tests/test_secret_file.cpp b/tests/test_secret_file.cpp new file mode 100644 index 00000000..46e71c2c --- /dev/null +++ b/tests/test_secret_file.cpp @@ -0,0 +1,106 @@ +// tests/test_secret_file.cpp — API-key / admin-token dest writes must +// not follow a planted symlink or hang on a FIFO. + +#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN +#include "doctest.h" + +#include "secret_file.h" + +#include +#include +#include + +#include +#include +#include +#include + +using namespace arbiter; +namespace fs = std::filesystem; + +namespace { + +std::string read_all(const fs::path& path) { + std::ifstream f(path); + std::ostringstream buf; + buf << f.rdbuf(); + return buf.str(); +} + +fs::path make_temp_dir(const char* tag) { + const auto dir = fs::temp_directory_path() + / (std::string(tag) + "-" + std::to_string(::getpid())); + fs::create_directories(dir); + return dir; +} + +} // namespace + +TEST_CASE("write_secret_file creates a 0600 regular file") { + const auto dir = make_temp_dir("arbiter-secret-create"); + const auto path = dir / "admin_token"; + + REQUIRE(write_secret_file(path.string(), "adm_fresh-token\n")); + CHECK(read_all(path) == "adm_fresh-token\n"); + CHECK(fs::is_regular_file(path)); + CHECK_FALSE(fs::is_symlink(path)); + struct stat st{}; + REQUIRE(::stat(path.c_str(), &st) == 0); + CHECK((st.st_mode & 0777) == 0600); + + fs::remove_all(dir); +} + +TEST_CASE("write_secret_file overwrites an existing regular file") { + const auto dir = make_temp_dir("arbiter-secret-overwrite"); + const auto path = dir / "openrouter_api_key"; + { + std::ofstream f(path); + f << "old-key\n"; + } + ::chmod(path.c_str(), 0644); + + REQUIRE(write_secret_file(path.string(), "sk-new-key\n")); + CHECK(read_all(path) == "sk-new-key\n"); + struct stat st{}; + REQUIRE(::stat(path.c_str(), &st) == 0); + CHECK((st.st_mode & 0777) == 0600); + + fs::remove_all(dir); +} + +TEST_CASE("write_secret_file does not follow a planted dest symlink") { + const auto dir = make_temp_dir("arbiter-secret-dest-link"); + const auto dest = dir / "admin_token"; + const auto victim = dir / "secret.txt"; + { + std::ofstream f(victim); + f << "do-not-clobber"; + } + fs::create_symlink(victim, dest); + + CHECK_FALSE(write_secret_file(dest.string(), "adm_LEAKED_TOKEN\n")); + CHECK(read_all(victim) == "do-not-clobber"); + CHECK(fs::is_symlink(dest)); + + fs::remove_all(dir); +} + +TEST_CASE("write_secret_file refuses a planted dest FIFO without hanging") { + const auto dir = make_temp_dir("arbiter-secret-fifo"); + const auto dest = dir / "search_api_key"; + REQUIRE(::mkfifo(dest.c_str(), 0600) == 0); + + CHECK_FALSE(write_secret_file(dest.string(), "search-secret\n")); + CHECK(fs::is_fifo(dest)); + + fs::remove_all(dir); +} + +TEST_CASE("write_secret_file refuses a directory dest") { + const auto dir = make_temp_dir("arbiter-secret-dir"); + CHECK_FALSE(write_secret_file(dir.string(), "nope\n")); + CHECK(fs::is_directory(dir)); + + fs::remove_all(dir); +}