From 1482610efe91d5ba5eb26751b7aabb8ebb164c23 Mon Sep 17 00:00:00 2001 From: wsnipex Date: Fri, 28 Sep 2018 12:14:05 +0200 Subject: fix CVE-2017-5982 thanks to Rechi for the windows part --- xbmc/network/WebServer.cpp | 5 + .../httprequesthandler/HTTPImageHandler.cpp | 4 +- .../httprequesthandler/HTTPWebinterfaceHandler.cpp | 4 + xbmc/utils/FileUtils.cpp | 109 ++++++++++++++++++++- xbmc/utils/FileUtils.h | 1 + 5 files changed, 120 insertions(+), 3 deletions(-) diff --git a/xbmc/network/WebServer.cpp b/xbmc/network/WebServer.cpp index f3e8d4124e..f63b336b39 100644 --- a/xbmc/network/WebServer.cpp +++ b/xbmc/network/WebServer.cpp @@ -28,6 +28,7 @@ #include "URL.h" #include "Util.h" #include "utils/Base64.h" +#include "utils/FileUtils.h" #include "utils/log.h" #include "utils/Mime.h" #include "utils/StringUtils.h" @@ -734,6 +735,10 @@ int CWebServer::CreateFileDownloadResponse(const std::shared_ptr file = std::make_shared(); std::string filePath = handler->GetResponseFile(); + // access check + if (!CFileUtils::CheckFileAccessAllowed(filePath)) + return SendErrorResponse(request, MHD_HTTP_NOT_FOUND, request.method); + if (!file->Open(filePath, XFILE::READ_NO_CACHE)) { CLog::Log(LOGERROR, "CWebServer[%hu]: Failed to open %s", m_port, filePath.c_str()); diff --git a/xbmc/network/httprequesthandler/HTTPImageHandler.cpp b/xbmc/network/httprequesthandler/HTTPImageHandler.cpp index a0417e6037..75e89b1997 100644 --- a/xbmc/network/httprequesthandler/HTTPImageHandler.cpp +++ b/xbmc/network/httprequesthandler/HTTPImageHandler.cpp @@ -10,6 +10,8 @@ #include "URL.h" #include "filesystem/ImageFile.h" #include "network/WebServer.h" +#include "utils/FileUtils.h" + CHTTPImageHandler::CHTTPImageHandler(const HTTPRequest &request) : CHTTPFileHandler(request) @@ -24,7 +26,7 @@ CHTTPImageHandler::CHTTPImageHandler(const HTTPRequest &request) XFILE::CImageFile imageFile; const CURL pathToUrl(file); - if (imageFile.Exists(pathToUrl)) + if (imageFile.Exists(pathToUrl) && CFileUtils::CheckFileAccessAllowed(file)) { responseStatus = MHD_HTTP_OK; struct __stat64 statBuffer; diff --git a/xbmc/network/httprequesthandler/HTTPWebinterfaceHandler.cpp b/xbmc/network/httprequesthandler/HTTPWebinterfaceHandler.cpp index 32edaefabb..6c091783ab 100644 --- a/xbmc/network/httprequesthandler/HTTPWebinterfaceHandler.cpp +++ b/xbmc/network/httprequesthandler/HTTPWebinterfaceHandler.cpp @@ -13,6 +13,7 @@ #include "addons/Webinterface.h" #include "filesystem/Directory.h" #include "filesystem/File.h" +#include "utils/FileUtils.h" #include "utils/StringUtils.h" #include "utils/URIUtils.h" @@ -61,6 +62,9 @@ int CHTTPWebinterfaceHandler::ResolveUrl(const std::string &url, std::string &pa } } + if (!CFileUtils::CheckFileAccessAllowed(path)) + return MHD_HTTP_NOT_FOUND; + if (!XFILE::CFile::Exists(path)) return MHD_HTTP_NOT_FOUND; diff --git a/xbmc/utils/FileUtils.cpp b/xbmc/utils/FileUtils.cpp index 771864d6de..9036e912a9 100644 --- a/xbmc/utils/FileUtils.cpp +++ b/xbmc/utils/FileUtils.cpp @@ -5,6 +5,7 @@ * SPDX-License-Identifier: GPL-2.0-or-later * See LICENSES/README.md for more information. */ + #include "FileUtils.h" #include "ServiceBroker.h" #include "guilib/GUIKeyboardFactory.h" @@ -13,9 +14,9 @@ #include "JobManager.h" #include "FileOperationJob.h" #include "URIUtils.h" -#include "filesystem/StackDirectory.h" #include "filesystem/MultiPathDirectory.h" -#include +#include "filesystem/SpecialProtocol.h" +#include "filesystem/StackDirectory.h" #include "settings/MediaSourceSettings.h" #include "Util.h" #include "StringUtils.h" @@ -24,6 +25,13 @@ #include "settings/SettingsComponent.h" #include "utils/Variant.h" +#if defined(TARGET_WINDOWS) +#include "platform/win32/WIN32Util.h" +#include "utils/CharsetConverter.h" +#endif + +#include + using namespace XFILE; bool CFileUtils::DeleteItem(const std::string &strPath) @@ -202,3 +210,100 @@ CDateTime CFileUtils::GetModificationDate(const std::string& strFileNameAndPath, } return dateAdded; } + +bool CFileUtils::CheckFileAccessAllowed(const std::string &filePath) +{ + // DENY access to paths matching + const std::vector blacklist = { + "passwords.xml", + "sources.xml", + "guisettings.xml", + "advancedsettings.xml", + "server.key", + "/.ssh/", + }; + // ALLOW kodi paths + const std::vector whitelist = { + CSpecialProtocol::TranslatePath("special://home"), + CSpecialProtocol::TranslatePath("special://xbmc") + }; + + // image urls come in the form of image://... sometimes with a / appended at the end + // strip this off to get the real file path + bool isImage = false; + std::string decodePath = CURL::Decode(filePath); + size_t pos = decodePath.find("image://"); + if (pos != std::string::npos) + { + isImage = true; + decodePath.erase(pos, 8); + URIUtils::RemoveSlashAtEnd(decodePath); + } + + // check blacklist + for (const auto &b : blacklist) + { + if (decodePath.find(b) != std::string::npos) + { + CLog::Log(LOGERROR,"%s denied access to %s", __FUNCTION__, decodePath.c_str()); + return false; + } + } + +#if defined(TARGET_POSIX) + std::string whiteEntry; + char *fullpath = realpath(decodePath.c_str(), nullptr); + + // if this is a locally existing file, check access permissions + if (fullpath) + { + const std::string realPath = fullpath; + free(fullpath); + + // check whitelist + for (const auto &w : whitelist) + { + char *realtemp = realpath(w.c_str(), nullptr); + if (realtemp) + { + whiteEntry = realtemp; + free(realtemp); + } + if (StringUtils::StartsWith(realPath, whiteEntry)) + return true; + } + // check sources with realPath + return CFileUtils::RemoteAccessAllowed(realPath); + } +#elif defined(TARGET_WINDOWS) + CURL url(decodePath); + if (url.GetProtocol().empty()) + { + std::wstring decodePathW; + g_charsetConverter.utf8ToW(decodePath, decodePathW, false); + CWIN32Util::AddExtraLongPathPrefix(decodePathW); + DWORD bufSize = GetFullPathNameW(decodePathW.c_str(), 0, nullptr, nullptr); + if (bufSize > 0) + { + std::wstring fullpathW; + fullpathW.resize(bufSize); + if (GetFullPathNameW(decodePathW.c_str(), bufSize, const_cast(fullpathW.c_str()), nullptr) <= bufSize - 1) + { + CWIN32Util::RemoveExtraLongPathPrefix(fullpathW); + std::string fullpath; + g_charsetConverter.wToUTF8(fullpathW, fullpath, false); + for (const std::string& whiteEntry : whitelist) + { + if (StringUtils::StartsWith(fullpath, whiteEntry)) + return true; + } + return CFileUtils::RemoteAccessAllowed(fullpath); + } + } + } +#endif + // if it isn't a local file, it must be a vfs entry + if (! isImage) + return CFileUtils::RemoteAccessAllowed(decodePath); + return true; +} diff --git a/xbmc/utils/FileUtils.h b/xbmc/utils/FileUtils.h index 13bb162065..fe76e6aef7 100644 --- a/xbmc/utils/FileUtils.h +++ b/xbmc/utils/FileUtils.h @@ -14,6 +14,7 @@ class CFileUtils { public: + static bool CheckFileAccessAllowed(const std::string &filePath); static bool DeleteItem(const CFileItemPtr &item); static bool DeleteItem(const std::string &strPath); static bool RenameFile(const std::string &strFile); -- cgit v1.2.3