From b35f28e200841396285b0759a251d96cadf307b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 30 Aug 2026 13:51:02 +0200 Subject: [PATCH] Remote ISO: don't serve the whole filesystem when no folder is set In LOCAL_FOLDER share mode, LocalFromRemotePath ended with return Path(g_Config.sRemoteISOSharedDir) / decoded; sRemoteISOSharedDir defaults to empty and nothing requires the user to pick a folder before pressing "Share Games (Server)". Path::operator/ doesn't insert a separator when the component already starts with one, so with an empty base it returns the component verbatim - "GET /etc/passwd" resolved to Path("/etc/passwd"), which is non-empty and went straight to DiscHandler. The backslash, "/.." and "//" filters never fired, because no traversal is needed to get there. That is an unauthenticated arbitrary file read for anything that can reach the port. Refuse to resolve anything when no shared directory is configured, and check that the joined path actually stays inside it. HandleListing needs the same guard: it called GetFilesInDir on the empty path, which on Windows becomes FindFirstFile("\*") - a listing of the root of the current drive. Also log a warning when the server starts in this state, so "nothing is shared" doesn't look like a mysterious failure. The empty-base behavior of Path::operator/ is surprising enough to be worth pinning down, so TestPath now asserts it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2 --- Core/WebServer.cpp | 31 ++++++++++++++++++++++++++++++- unittest/UnitTest.cpp | 6 ++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/Core/WebServer.cpp b/Core/WebServer.cpp index e2b99c5f35..ccbc64bb38 100644 --- a/Core/WebServer.cpp +++ b/Core/WebServer.cpp @@ -215,6 +215,16 @@ static Path LocalFromRemotePath(std::string_view path) { return Path(); case RemoteISOShareType::LOCAL_FOLDER: { + // Nothing stops the user from starting the server without ever picking a folder, and an empty + // base is NOT harmless here: Path("") / "/etc/passwd" is just Path("/etc/passwd"), since + // operator/ doesn't insert a separator when the component already starts with one. That would + // serve the entire filesystem to anyone who can reach the port, and none of the checks below + // would fire, because no traversal is needed to get there. + const Path sharedDir(g_Config.sRemoteISOSharedDir); + if (sharedDir.empty()) { + return Path(); + } + std::string decoded = ServerUriDecode(path); if (decoded.empty() || decoded.front() != '/') { @@ -229,7 +239,14 @@ static Path LocalFromRemotePath(std::string_view path) { if (decoded.find("/..") != std::string::npos) { return Path(); } - return Path(g_Config.sRemoteISOSharedDir) / decoded; + + // Belt and braces: whatever the path manipulation above ends up doing, the result has to stay + // inside the shared directory. + const Path localPath = sharedDir / decoded; + if (!localPath.StartsWith(sharedDir)) { + return Path(); + } + return localPath; } default: return Path(); @@ -316,6 +333,12 @@ static void HandleListing(const http::ServerRequest &request) { std::string resource(request.resource()); Path localDir = LocalFromRemotePath(resource); + if (localDir.empty()) { + // Refused (not configured, or traversal attempt). Don't hand an empty path to + // GetFilesInDir - on Windows that turns into FindFirstFile("\\*"), i.e. a listing + // of the root of the current drive. + break; + } File::GetFilesInDir(localDir, &entries); for (const auto &entry : entries) { @@ -851,6 +874,12 @@ static void WebServerThread() { // Only adds flags. bool StartWebServer(WebServerFlags flags) { + if ((flags & WebServerFlags::DISCS) && (RemoteISOShareType)g_Config.iRemoteISOShareType == RemoteISOShareType::LOCAL_FOLDER && g_Config.sRemoteISOSharedDir.empty()) { + // Not fatal - LocalFromRemotePath refuses to resolve anything in this state, so the server just + // won't serve any files. Worth a log line so it doesn't look like a mysterious failure. + WARN_LOG(Log::Loader, "Remote ISO sharing is set to share a local folder, but no folder is set - nothing will be shared."); + } + std::lock_guard guard(serverStatusLock); switch (serverStatus) { case ServerStatus::RUNNING: diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index 08621578d1..9b9913cdb7 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -2228,6 +2228,12 @@ static bool TestPath() { Path path3 = path2 / "foo/bar"; EXPECT_EQ_STR(path3.WithExtraExtension(".txt").ToString(), std::string("/asdf/jkl/foo/bar.txt")); + // An empty base does NOT anchor anything - the component is simply taken as-is. Anything + // joining a user-configured directory with a request-supplied component has to check the base + // itself (see LocalFromRemotePath in Core/WebServer.cpp, where this was a filesystem-wide leak). + EXPECT_EQ_STR((Path("") / "/etc/passwd").ToString(), std::string("/etc/passwd")); + EXPECT_EQ_INT((Path("") / "/etc/passwd").empty(), false); + EXPECT_EQ_STR(Path("foo.bar/hello").GetFileExtension(), std::string()); EXPECT_EQ_STR(Path("foo.bar/hello.txt").WithReplacedExtension(".txt", ".html").ToString(), std::string("foo.bar/hello.html"));