From 9b54e97fd4b5fba0de7f1c2e9f481fd44eab9682 Mon Sep 17 00:00:00 2001 From: mundur Date: Mon, 27 Jul 2026 10:53:58 +0000 Subject: [PATCH] Reject path traversal in mount destinations during jail staging Mount destinations were joined as newroot+"/"+dst and walked with mkdirat/openat without rejecting "."/".." components or following symlinks safely. An attacker who can influence mount dst (CLI, config, or prefix_dst_env) could create directories, symlinks, or mounts outside the intended staging root on the host before pivot_root. Add isSafeContainmentPath(), validate destinations in both legacy and new mount APIs, use O_NOFOLLOW while walking parents, and include a standalone regression test. --- mnt_legacy.cc | 16 ++++ mnt_newapi.cc | 15 ++++ tests/path_containment_test.cc | 156 +++++++++++++++++++++++++++++++++ util.cc | 31 ++++++- util.h | 3 + 5 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 tests/path_containment_test.cc diff --git a/mnt_legacy.cc b/mnt_legacy.cc index 4cc9d1a..69b5e14 100644 --- a/mnt_legacy.cc +++ b/mnt_legacy.cc @@ -82,6 +82,13 @@ static mount_t prepareMountPoint(const nsjail::MountPt& proto) { } mpt.dst += proto.dst(); + if (!util::isSafeContainmentPath(mpt.dst)) { + LOG_E("Mount destination escapes containment via '.'/'..'/NUL: %s", QC(mpt.dst)); + /* Keep the unsafe dst so later mount steps fail closed rather than + * treating a cleared path as the jail root. */ + return mpt; + } + mpt.flags = proto.rw() ? 0 : (uintptr_t)MS_RDONLY; if (proto.is_bind()) { mpt.flags |= MS_BIND | MS_REC | MS_PRIVATE; @@ -196,7 +203,16 @@ static bool mountWithDynamicContent( static bool mountSinglePoint(mount_t* mpt, const char* newroot, const char* tmpdir) { LOG_D("Mounting (legacy): %s", mnt::describeMountPt(*mpt->mpt).c_str()); + if (!util::isSafeContainmentPath(mpt->dst)) { + LOG_E("Mount destination escapes containment via '.'/'..'/NUL: %s", QC(mpt->dst)); + return false; + } + const std::string dstpath = std::string(newroot) + "/" + mpt->dst; + if (!util::isSafeContainmentPath(dstpath)) { + LOG_E("Resolved mount path escapes containment: %s", QC(dstpath)); + return false; + } std::string srcpath = mpt->src.empty() ? "none" : mpt->src; if (!util::createDirRecursively(dstpath.c_str())) { diff --git a/mnt_newapi.cc b/mnt_newapi.cc index d96623d..c0cf6a5 100644 --- a/mnt_newapi.cc +++ b/mnt_newapi.cc @@ -199,6 +199,10 @@ static bool createDirAt(int dir_fd, const char* path, mode_t mode) { if (!path[0]) { return true; } + if (!util::isSafeContainmentPath(path)) { + LOG_E("Mount destination escapes containment via '.'/'..'/NUL: '%s'", path); + return false; + } std::string cumulative; for (const auto& component : util::strSplit(path, '/')) { @@ -412,8 +416,12 @@ static bool mountSinglePointAt(mount_t* mpt, int root_fd) { LOG_D("Mounting (new API): %s", mnt::describeMountPt(*mpt->mpt).c_str()); const char* rel_dst = util::stripLeadingSlashes(mpt->dst.c_str()); + /* Empty / only-slashes dst means the jail root (represented as "."). */ if (!rel_dst[0]) { rel_dst = "."; + } else if (!util::isSafeContainmentPath(rel_dst)) { + LOG_E("Mount destination escapes containment via '.'/'..'/NUL: %s", QC(mpt->dst)); + return false; } const char* last_slash = strrchr(rel_dst, '/'); @@ -504,6 +512,13 @@ static mount_t prepareMountPoint(const nsjail::MountPt& proto) { } mpt.dst += proto.dst(); + if (!util::isSafeContainmentPath(mpt.dst)) { + LOG_E("Mount destination escapes containment via '.'/'..'/NUL: %s", QC(mpt.dst)); + /* Keep the unsafe dst so later mount steps fail closed rather than + * treating a cleared path as the jail root. */ + return mpt; + } + mpt.flags = proto.rw() ? 0 : (uintptr_t)MS_RDONLY; if (proto.is_bind()) { mpt.flags |= MS_BIND | MS_REC | MS_PRIVATE; diff --git a/tests/path_containment_test.cc b/tests/path_containment_test.cc new file mode 100644 index 0000000..ce76f38 --- /dev/null +++ b/tests/path_containment_test.cc @@ -0,0 +1,156 @@ +/* + * Regression tests for mount-destination path traversal hardening. + * + * Standalone harness (mirrors util::isSafeContainmentPath / + * createDirRecursively policy) so it can run without linking full nsjail. + * + * g++ -std=c++20 -O1 -o path_containment_test tests/path_containment_test.cc + * ./path_containment_test + */ + +#include +#include +#include +#include +#include +#include +#include + +namespace { + +std::vector strSplit(const std::string& str, char delim) { + std::vector vec; + std::string word; + for (char c : str) { + if (c == delim) { + vec.push_back(word); + word.clear(); + } else { + word.push_back(c); + } + } + vec.push_back(word); + return vec; +} + +bool isSafeContainmentPath(const std::string& path) { + if (path.empty()) { + return true; + } + if (path.find('\0') != std::string::npos) { + return false; + } + for (const auto& component : strSplit(path, '/')) { + if (component.empty()) { + continue; + } + if (component == "." || component == "..") { + return false; + } + } + return true; +} + +bool createDirRecursivelySafe(const char* dir) { + if (dir[0] != '/') { + return false; + } + if (!isSafeContainmentPath(dir)) { + return false; + } + + int prev_dir_fd = open("/", O_RDONLY | O_CLOEXEC | O_DIRECTORY); + if (prev_dir_fd == -1) { + return false; + } + + char path[4096]; + if (snprintf(path, sizeof(path), "%s", dir) >= (int)sizeof(path)) { + close(prev_dir_fd); + return false; + } + char* curr = path; + for (;;) { + while (*curr == '/') { + curr++; + } + char* next = strchr(curr, '/'); + if (next == nullptr) { + close(prev_dir_fd); + return true; + } + *next = '\0'; + if (mkdirat(prev_dir_fd, curr, 0755) == -1 && errno != EEXIST) { + close(prev_dir_fd); + return false; + } + int dir_fd = openat(prev_dir_fd, curr, O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW); + if (dir_fd == -1) { + close(prev_dir_fd); + return false; + } + close(prev_dir_fd); + prev_dir_fd = dir_fd; + curr = next + 1; + } +} + +} // namespace + +static void expect_safe(const char* p, bool want) { + bool got = isSafeContainmentPath(p); + if (got != want) { + fprintf(stderr, "FAIL isSafeContainmentPath(%s) = %d want %d\n", p, got, want); + exit(1); + } +} + +int main() { + expect_safe("", true); + expect_safe("/", true); + expect_safe("/usr/lib", true); + expect_safe("/tmp/nsjail.root/home/user", true); + expect_safe("usr/lib", true); + expect_safe("/usr//lib", true); + + expect_safe("..", false); + expect_safe("/..", false); + expect_safe("/../", false); + expect_safe("/tmp/../etc", false); + expect_safe("/tmp/nsjail.root/../../etc/passwd", false); + expect_safe("/foo/./bar", false); + expect_safe("./foo", false); + expect_safe("foo/../../bar", false); + + std::string with_nul = std::string("/tmp/foo") + '\0' + "bar"; + if (isSafeContainmentPath(with_nul)) { + fprintf(stderr, "FAIL NUL component accepted\n"); + return 1; + } + + (void)system("rm -rf /tmp/nsj_path_test /tmp/nsj_path_escaped"); + mkdir("/tmp/nsj_path_test", 0755); + const char* escape = + "/tmp/nsj_path_test/root/../../nsj_path_escaped/evil_dir/leaf"; + if (createDirRecursivelySafe(escape)) { + fprintf(stderr, "FAIL createDirRecursively accepted traversal path\n"); + return 1; + } + if (access("/tmp/nsj_path_escaped", F_OK) == 0) { + fprintf(stderr, "FAIL escape directory was created\n"); + return 1; + } + + const char* ok = "/tmp/nsj_path_test/root/home/user/docs/leaf"; + if (!createDirRecursivelySafe(ok)) { + fprintf(stderr, "FAIL createDirRecursively rejected safe path\n"); + return 1; + } + if (access("/tmp/nsj_path_test/root/home/user/docs", F_OK) != 0) { + fprintf(stderr, "FAIL safe parents were not created\n"); + return 1; + } + + printf("OK path_containment_test passed\n"); + return 0; +} diff --git a/util.cc b/util.cc index e09986c..d7a42ab 100644 --- a/util.cc +++ b/util.cc @@ -232,11 +232,35 @@ bool writeBufToFile( return true; } +bool isSafeContainmentPath(const std::string& path) { + /* Empty / root-only paths refer to the containment root itself. */ + if (path.empty()) { + return true; + } + /* Embedded NUL would truncate C-string APIs and hide trailing components. */ + if (path.find('\0') != std::string::npos) { + return false; + } + for (const auto& component : strSplit(path, '/')) { + if (component.empty()) { + continue; + } + if (component == "." || component == "..") { + return false; + } + } + return true; +} + bool createDirRecursively(const char* dir) { if (dir[0] != '/') { LOG_W("The directory path must start with '/': '%s' provided", dir); return false; } + if (!isSafeContainmentPath(dir)) { + LOG_W("Refusing path with '.'/'..'/NUL components: '%s'", dir); + return false; + } int prev_dir_fd = TEMP_FAILURE_RETRY(open("/", O_RDONLY | O_CLOEXEC | O_DIRECTORY)); if (prev_dir_fd == -1) { @@ -271,9 +295,12 @@ bool createDirRecursively(const char* dir) { } } - int dir_fd = TEMP_FAILURE_RETRY(openat(prev_dir_fd, curr, O_DIRECTORY | O_CLOEXEC)); + /* O_NOFOLLOW: do not walk through symlinks that escape the intended tree. */ + int dir_fd = TEMP_FAILURE_RETRY( + openat(prev_dir_fd, curr, O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW)); if (dir_fd == -1) { - PLOG_W("openat('%d', %s, O_DIRECTORY | O_CLOEXEC)", prev_dir_fd, QC(curr)); + PLOG_W("openat('%d', %s, O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW)", prev_dir_fd, + QC(curr)); close(prev_dir_fd); return false; } diff --git a/util.h b/util.h index 92acfd4..e1f4421 100644 --- a/util.h +++ b/util.h @@ -60,6 +60,9 @@ int recvFd(int sock); bool writeBufToFile( const char* filename, const void* buf, size_t len, int open_flags, bool log_errors = true); bool createDirRecursively(const char* dir); +/* Reject ".", ".." and embedded NUL in mount destinations so they cannot + * escape the jail staging root via path traversal. */ +bool isSafeContainmentPath(const std::string& path); std::string* StrAppend(std::string* str, const char* format, ...) __attribute__((format(printf, 2, 3))); std::string StrPrintf(const char* format, ...) __attribute__((format(printf, 1, 2)));