From d54bc6857f198d2d85a20b2e5321b196f73582d6 Mon Sep 17 00:00:00 2001 From: Martin Natano Date: Mon, 2 May 2022 22:27:19 +0200 Subject: [PATCH] Prevent access to /Save/Preferences.ini from lua Directly writing to the file would circumvent preference protections, compromising security. --- src/RageFile.cpp | 13 ++++- src/RageFileManager.cpp | 105 ++++++++++++++++++---------------------- src/RageFileManager.h | 9 ++++ src/StepMania.cpp | 3 ++ 4 files changed, 71 insertions(+), 59 deletions(-) diff --git a/src/RageFile.cpp b/src/RageFile.cpp index 32dd3af825..c0642aa127 100644 --- a/src/RageFile.cpp +++ b/src/RageFile.cpp @@ -8,6 +8,7 @@ #include "global.h" #include "RageFileBasic.h" #include "RageFile.h" +#include "RageLog.h" #include "RageUtil.h" #include "RageFileDriver.h" @@ -351,7 +352,17 @@ public: static int Open( T* p, lua_State *L ) { - lua_pushboolean( L, p->Open( SArg(1), IArg(2) ) ); + const RString path = SArg(1); + int mode = IArg(2); + + if ((mode & RageFile::WRITE) && FILEMAN->IsPathProtected(path)) + { + LOG->Warn("Writing to %s is not allowed", path.c_str()); + lua_pushboolean(L, false); + return 1; + } + + lua_pushboolean( L, p->Open(path, mode) ); return 1; } diff --git a/src/RageFileManager.cpp b/src/RageFileManager.cpp index 55418b68c1..2e504a7736 100644 --- a/src/RageFileManager.cpp +++ b/src/RageFileManager.cpp @@ -197,6 +197,12 @@ bool RageFileManager::Unzip(const std::string &zipPath, std::string targetPath, std::string filepath = targetPath + filename; + if (FILEMAN->IsPathProtected(filepath)) + { + LOG->Warn("Overwriting %s is not allowed", filepath.c_str()); + continue; + } + if (info.m_is_directory) { CreateDir(filepath); @@ -227,42 +233,37 @@ bool RageFileManager::Unzip(const std::string &zipPath, std::string targetPath, return success; } -/* Wait for the given driver to become unreferenced, and remove it from the list - * to get exclusive access to it. Returns false if the driver is no longer available - * (somebody else got it first). */ -#if 0 -static bool GrabDriver( RageFileDriver *pDriver ) +static void NormalizePath( RString &sPath ) { - g_Mutex->Lock(); - - for(;;) + FixSlashesInPlace( sPath ); + CollapsePath( sPath, true ); + if (sPath.size() == 0) { - unsigned i; - for( i = 0; i < g_pDrivers.size(); ++i ) - { - if( g_pDrivers[i]->m_pDriver == pDriver ) - { - break; - } - } - if( i == g_pDrivers.size() ) - { - g_Mutex->Unlock(); - return false; - } - - if( g_pDrivers[i]->m_iRefs == 0 ) - { - g_pDrivers.erase( g_pDrivers.begin()+i ); - return true; - } - - /* The driver is in use. Wait for somebody to release a driver, and - * try again. */ - g_Mutex->Wait(); + sPath = '/'; + } + else if (sPath[0] != '/') + { + sPath = '/' + sPath; } } -#endif + +void RageFileManager::ProtectPath(const std::string& path) +{ + RString normalizedPath(path); + NormalizePath(normalizedPath); + normalizedPath.MakeLower(); + + m_protectedPaths.insert(normalizedPath); +} + +bool RageFileManager::IsPathProtected(const std::string& path) +{ + RString normalizedPath(path); + NormalizePath(normalizedPath); + normalizedPath.MakeLower(); + + return m_protectedPaths.count(normalizedPath) > 0; +} // Mountpoints as directories cause a problem. If "Themes/default" is a mountpoint, and // doesn't exist anywhere else, then GetDirListing("Themes/*") must return "default". The @@ -492,20 +493,6 @@ RString LoadedDriver::GetPath( const RString &sPath ) const return sRet; } -static void NormalizePath( RString &sPath ) -{ - FixSlashesInPlace( sPath ); - CollapsePath( sPath, true ); - if (sPath.size() == 0) - { - sPath = '/'; - } - else if (sPath[0] != '/') - { - sPath = '/' + sPath; - } -} - bool ilt( const RString &a, const RString &b ) { return a.CompareNoCase(b) < 0; } bool ieq( const RString &a, const RString &b ) { return a.CompareNoCase(b) == 0; } void RageFileManager::GetDirListing( const RString &sPath_, vector &AddTo, bool bOnlyDirs, bool bReturnPathToo ) @@ -1277,7 +1264,20 @@ unsigned int GetHashForDirectory( const RString &sDir ) class LunaRageFileManager: public Luna { public: - static int Copy(T* p, lua_State *L){ lua_pushboolean(L, p->Copy(SArg(1), SArg(2))); return 1; } + static int Copy(T* p, lua_State *L){ + const std::string fromPath = SArg(1); + const std::string toPath = SArg(2); + + if (p->IsPathProtected(toPath)) + { + LOG->Warn("Overwriting %s is not allowed", toPath.c_str()); + lua_pushboolean(L, false); + return 1; + } + + lua_pushboolean(L, p->Copy(fromPath, toPath)); + return 1; + } static int DoesFileExist( T* p, lua_State *L ){ lua_pushboolean( L, p->DoesFileExist(SArg(1)) ); return 1; } static int GetFileSizeBytes( T* p, lua_State *L ){ lua_pushnumber( L, p->GetFileSizeInBytes(SArg(1)) ); return 1; } static int GetHashForFile( T* p, lua_State *L ){ lua_pushnumber( L, p->GetFileHash(SArg(1)) ); return 1; } @@ -1302,16 +1302,6 @@ public: LuaHelpers::CreateTableFromArray(vDirs, L); return 1; } - /* - static int GetDirListingRecursive( T* p, lua_State *L ) - { - vector vDirs; - // (directory, match, addto) - GetDirListingRecursive( SArg(1), SArg(2), vDirs ); - LuaHelpers::CreateTableFromArray(vDirs, L); - return 1; - } - */ static int Unzip(T* p, lua_State *L) { std::string zipPath = SArg(1); @@ -1337,7 +1327,6 @@ public: ADD_METHOD( GetFileSizeBytes ); ADD_METHOD( GetHashForFile ); ADD_METHOD( GetDirListing ); - //ADD_METHOD( GetDirListingRecursive ); ADD_METHOD( Unzip ); } }; diff --git a/src/RageFileManager.h b/src/RageFileManager.h index 5dbedb1062..bf6138764a 100644 --- a/src/RageFileManager.h +++ b/src/RageFileManager.h @@ -1,5 +1,8 @@ #ifndef RAGE_FILE_MANAGER_H #define RAGE_FILE_MANAGER_H + +#include + /** @brief Constants for working with the RageFileManager. */ namespace RageFileManagerUtil { @@ -74,12 +77,18 @@ public: bool Unzip(const std::string &zipPath, std::string targetPath, int strip); + // path protection + void ProtectPath(const std::string& path); + bool IsPathProtected(const std::string& path); + // Lua void PushSelf( lua_State *L ); private: RageFileBasic *OpenForReading( const RString &sPath, int iMode, int &iError ); RageFileBasic *OpenForWriting( const RString &sPath, int iMode, int &iError ); + + std::unordered_set m_protectedPaths; }; extern RageFileManager *FILEMAN; diff --git a/src/StepMania.cpp b/src/StepMania.cpp index 5541318c3f..7affd25b89 100644 --- a/src/StepMania.cpp +++ b/src/StepMania.cpp @@ -1002,6 +1002,9 @@ int sm_main(int argc, char* argv[]) // Almost everything uses this to read and write files. Load this early. FILEMAN = new RageFileManager( argv[0] ); + FILEMAN->ProtectPath(SpecialFiles::DEFAULTS_INI_PATH); + FILEMAN->ProtectPath(SpecialFiles::STATIC_INI_PATH); + FILEMAN->ProtectPath(SpecialFiles::PREFERENCES_INI_PATH); FILEMAN->MountInitialFilesystems(); bool bPortable = DoesFileExist("/Portable.ini");