diff --git a/core/src/io/sparse_file.cpp b/core/src/io/sparse_file.cpp index 45e2175..fb98210 100644 --- a/core/src/io/sparse_file.cpp +++ b/core/src/io/sparse_file.cpp @@ -74,7 +74,12 @@ Result SparseFile::open(std::string_view path, const OpenOptions &opts) { return ErrorInfo(Error::internal, "SparseFile already open"); std::string p(path); - int flags = O_WRONLY | O_CREAT | O_CLOEXEC; + // O_NOFOLLOW: the final component of a download target must never be a symlink, on + // create or on resume. DAEMON canonicalises the path and checks it against the allowed + // roots before start(), but a symlink swapped in afterwards would redirect our writes + // outside those roots (daemon/docs/safepath-adversarial.md leans on this open closing + // that TOCTOU window). A symlinked leaf fails here with ELOOP -> Error::path_rejected. + int flags = O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW; if (opts.truncate_existing) flags |= O_TRUNC; diff --git a/core/tests/io/sparse_file_test.cpp b/core/tests/io/sparse_file_test.cpp index 1a4b469..ba385d1 100644 --- a/core/tests/io/sparse_file_test.cpp +++ b/core/tests/io/sparse_file_test.cpp @@ -117,6 +117,23 @@ VT_TEST(sparse_open_bad_path_is_path_rejected) { VT_CHECK(!f.is_open()); } +VT_TEST(sparse_symlinked_target_is_rejected) { + // A symlink swapped in as the final path component after DAEMON's canonicalise-and-check + // must not be followed: the open is O_NOFOLLOW, so it fails with ELOOP -> path_rejected + // rather than redirecting our writes through the link. + TempPath link; // the download target the caller hands us + TempPath target; // where the symlink points (would-be victim, outside allowed roots) + VT_REQUIRE(::symlink(target.path.c_str(), link.path.c_str()) == 0); + + SparseFile f; + auto r = f.open(link.path, {.total_size = 4096}); + VT_REQUIRE(!r.has_value()); + VT_CHECK_EQ(r.error().code, Error::path_rejected); + VT_CHECK(!f.is_open()); + // the link target was never created/written through + VT_CHECK_EQ(::access(target.path.c_str(), F_OK), -1); +} + VT_TEST(sparse_ops_on_closed_file_error) { SparseFile f; VT_CHECK_EQ(f.write_at(0, bytes("x")).error().code, Error::internal); diff --git a/docs/04-engine-design.md b/docs/04-engine-design.md index 7626cfe..b2ab3dc 100644 --- a/docs/04-engine-design.md +++ b/docs/04-engine-design.md @@ -61,8 +61,14 @@ Rules: ## 4. Disk I/O — the buffer setting you asked for -One file, opened once, `O_WRONLY`. Each segment `pwrite()`s at its own absolute offset, so -**there is no reassembly pass and no second write of the whole file.** +One file, opened once, `O_WRONLY | O_NOFOLLOW`. Each segment `pwrite()`s at its own absolute +offset, so **there is no reassembly pass and no second write of the whole file.** + +`O_NOFOLLOW` on the part-file open: DAEMON canonicalises the save path and checks it against +the allowed roots before `start()`, but the final component could be swapped for a symlink +in the window between that check and our open. A symlinked leaf is rejected here (`ELOOP` → +`Error::path_rejected`), not followed — it closes the TOCTOU residual that +`daemon/docs/safepath-adversarial.md` accepts on those grounds. - `posix_fallocate()` the full size up front → contiguous extents, no ENOSPC surprise at 99 %, no fragmentation.