diff --git a/Core/WebServer.cpp b/Core/WebServer.cpp index 9482e2d62c..1b4df8f1ee 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) { @@ -869,6 +892,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"));