From 6ebd6c0f16cf15bb455b81ca571ef88bac381655 Mon Sep 17 00:00:00 2001 From: Andrei Karas Date: Tue, 28 Feb 2017 00:54:28 +0300 Subject: Add path sanitization in virtfsdir. --- src/fs/paths.cpp | 14 ++++++++++++++ src/fs/paths.h | 2 ++ src/fs/virtfsdir.cpp | 37 +++++++++++++++++++++++++----------- src/fs/virtfsdir.h | 20 ++++++++++---------- src/fs/virtfsdir_unittest.cc | 45 ++++++++++++++++++++++++++++++-------------- 5 files changed, 83 insertions(+), 35 deletions(-) diff --git a/src/fs/paths.cpp b/src/fs/paths.cpp index aa1c453b0..8a9969461 100644 --- a/src/fs/paths.cpp +++ b/src/fs/paths.cpp @@ -27,6 +27,7 @@ #include "fs/paths.h" #include "fs/virtfs.h" +#include "utils/checkutils.h" #include "utils/stringutils.h" #ifdef USE_X11 @@ -101,6 +102,19 @@ bool checkPath(const std::string &path) && path.find("\\..") == std::string::npos; } +void prepareFsPath(std::string &path) +{ + std::string path2 = path; + sanitizePath(path); +// can be enabled for debugging +// if (path != path2) +// { +// reportAlways("Path can be improved: '%s' -> '%s'", +// path2.c_str(), +// path0.c_str()); +// } +} + std::string &fixDirSeparators(std::string &str) { if (dirSeparator[0] == '/') diff --git a/src/fs/paths.h b/src/fs/paths.h index 48a6eeea8..be431f9c3 100644 --- a/src/fs/paths.h +++ b/src/fs/paths.h @@ -31,6 +31,8 @@ bool isRealPath(const std::string &str) A_WARN_UNUSED; bool checkPath(const std::string &path) A_WARN_UNUSED; +void prepareFsPath(std::string &path); + std::string &fixDirSeparators(std::string &str); std::string removeLast(const std::string &str) A_WARN_UNUSED; diff --git a/src/fs/virtfsdir.cpp b/src/fs/virtfsdir.cpp index 605ad2aac..6673072aa 100644 --- a/src/fs/virtfsdir.cpp +++ b/src/fs/virtfsdir.cpp @@ -60,7 +60,7 @@ namespace VirtFsDir { namespace { - static VirtFile *openFile(const std::string &restrict filename, + static VirtFile *openFile(std::string filename, const int mode) { if (checkPath(filename) == false) @@ -69,6 +69,7 @@ namespace VirtFsDir filename.c_str()); return nullptr; } + prepareFsPath(filename); VirtDirEntry *const entry = searchEntryByPath(filename); if (entry == nullptr) return nullptr; @@ -111,10 +112,11 @@ namespace VirtFsDir return nullptr; } - bool addToSearchPathSilent(const std::string &newDir, + bool addToSearchPathSilent(std::string newDir, const Append append, const SkipError skipError) { + prepareFsPath(newDir); if (skipError == SkipError_false && Files::existsLocal(newDir) == false) { @@ -152,9 +154,10 @@ namespace VirtFsDir return true; } - bool addToSearchPath(const std::string &newDir, + bool addToSearchPath(std::string newDir, const Append append) { + prepareFsPath(newDir); if (Files::existsLocal(newDir) == false) { reportAlways("VirtFsDir::addToSearchPath directory not exists: %s", @@ -193,6 +196,7 @@ namespace VirtFsDir bool removeFromSearchPathSilent(std::string oldDir) { + prepareFsPath(oldDir); if (oldDir.find(".zip") != std::string::npos) { reportAlways("Called removeFromSearchPath with zip archive"); @@ -219,6 +223,7 @@ namespace VirtFsDir bool removeFromSearchPath(std::string oldDir) { + prepareFsPath(oldDir); if (oldDir.find(".zip") != std::string::npos) { reportAlways("Called removeFromSearchPath with zip archive"); @@ -269,7 +274,9 @@ namespace VirtFsDir mBaseDir = getRealPath(getFileDir(name)); #endif // defined(__native_client__) + prepareFsPath(mBaseDir); mUserDir = getHomePath(); + prepareFsPath(mUserDir); initFuncs(&funcs); } @@ -294,8 +301,9 @@ namespace VirtFsDir return mUserDir.c_str(); } - std::string getRealDir(const std::string &restrict filename) + std::string getRealDir(std::string filename) { + prepareFsPath(filename); if (checkPath(filename) == false) { reportAlways("VirtFsDir::exists invalid path: %s", @@ -312,8 +320,9 @@ namespace VirtFsDir return std::string(); } - bool exists(const std::string &restrict name) + bool exists(std::string name) { + prepareFsPath(name); if (checkPath(name) == false) { reportAlways("VirtFsDir::exists invalid path: %s", @@ -329,8 +338,9 @@ namespace VirtFsDir return false; } - VirtList *enumerateFiles(const std::string &restrict dirName) + VirtList *enumerateFiles(std::string dirName) { + prepareFsPath(dirName); VirtList *const list = new VirtList; if (checkPath(dirName) == false) { @@ -382,8 +392,9 @@ namespace VirtFsDir return list; } - bool isDirectory(const std::string &restrict dirName) + bool isDirectory(std::string dirName) { + prepareFsPath(dirName); if (checkPath(dirName) == false) { reportAlways("VirtFsDir::isDirectory invalid path: %s", @@ -407,8 +418,9 @@ namespace VirtFsDir return false; } - bool isSymbolicLink(const std::string &restrict name) + bool isSymbolicLink(std::string name) { + prepareFsPath(name); if (checkPath(name) == false) { reportAlways("VirtFsDir::isSymbolicLink invalid path: %s", @@ -443,16 +455,18 @@ namespace VirtFsDir return openFile(filename, O_WRONLY | O_CREAT | O_APPEND); } - bool setWriteDir(const std::string &restrict newDir) + bool setWriteDir(std::string newDir) { + prepareFsPath(newDir); mWriteDir = newDir; if (findLast(mWriteDir, std::string(dirSeparator)) == false) mWriteDir += dirSeparator; return true; } - bool mkdir(const std::string &restrict dirname) + bool mkdir(std::string dirname) { + prepareFsPath(dirname); if (mWriteDir.empty()) { reportAlways("VirtFsDir::mkdir write dir is empty"); @@ -461,8 +475,9 @@ namespace VirtFsDir return mkdir_r((mWriteDir + dirname).c_str()) != -1; } - bool remove(const std::string &restrict filename) + bool remove(std::string filename) { + prepareFsPath(filename); if (mWriteDir.empty()) { reportAlways("VirtFsDir::remove write dir is empty"); diff --git a/src/fs/virtfsdir.h b/src/fs/virtfsdir.h index 0f36b1241..8ea825d82 100644 --- a/src/fs/virtfsdir.h +++ b/src/fs/virtfsdir.h @@ -40,9 +40,9 @@ namespace VirtFsDir VirtDirEntry *searchEntryByPath(const std::string &restrict path); const char *getBaseDir(); const char *getUserDir(); - bool addToSearchPath(const std::string &newDir, + bool addToSearchPath(std::string newDir, const Append append); - bool addToSearchPathSilent(const std::string &newDir, + bool addToSearchPathSilent(std::string newDir, const Append append, const SkipError skipError); bool removeFromSearchPath(std::string oldDir); @@ -51,18 +51,18 @@ namespace VirtFsDir void initFuncs(VirtFsFuncs *restrict const ptr); void deinit(); std::vector &getEntries(); - bool exists(const std::string &restrict name); - VirtList *enumerateFiles(const std::string &restrict dir) RETURNS_NONNULL; - bool isDirectory(const std::string &restrict dirName); - bool isSymbolicLink(const std::string &restrict name); + bool exists(std::string name); + VirtList *enumerateFiles(std::string dir) RETURNS_NONNULL; + bool isDirectory(std::string dirName); + bool isSymbolicLink(std::string name); void freeList(VirtList *restrict const handle); VirtFile *openRead(const std::string &restrict filename); VirtFile *openWrite(const std::string &restrict filename); VirtFile *openAppend(const std::string &restrict filename); - bool setWriteDir(const std::string &restrict newDir); - std::string getRealDir(const std::string &restrict filename); - bool mkdir(const std::string &restrict dirName); - bool remove(const std::string &restrict filename); + bool setWriteDir(std::string newDir); + std::string getRealDir(std::string filename); + bool mkdir(std::string dirName); + bool remove(std::string filename); void permitLinks(const bool val); const char *getLastError(); int64_t read(VirtFile *restrict const handle, diff --git a/src/fs/virtfsdir_unittest.cc b/src/fs/virtfsdir_unittest.cc index 1f162a6e9..589b10e4c 100644 --- a/src/fs/virtfsdir_unittest.cc +++ b/src/fs/virtfsdir_unittest.cc @@ -93,7 +93,7 @@ TEST_CASE("VirtFsDir addToSearchPath") SECTION("simple 4") { - REQUIRE(VirtFsDir::addToSearchPathSilent("dir1", + REQUIRE(VirtFsDir::addToSearchPathSilent("dir1\\", Append_true, SkipError_true)); REQUIRE(VirtFsDir::addToSearchPathSilent("dir2", @@ -105,7 +105,7 @@ TEST_CASE("VirtFsDir addToSearchPath") REQUIRE(VirtFsDir::getEntries().size() == 2); REQUIRE(VirtFsDir::getEntries()[0]->mRootDir == "dir1/"); REQUIRE(VirtFsDir::getEntries()[1]->mRootDir == "dir2/"); - REQUIRE(VirtFsDir::getEntries()[0]->mUserDir == "dir1"); + REQUIRE(VirtFsDir::getEntries()[0]->mUserDir == "dir1/"); REQUIRE(VirtFsDir::getEntries()[1]->mUserDir == "dir2"); } @@ -169,6 +169,7 @@ TEST_CASE("VirtFsDir removeFromSearchPath") SECTION("simple 1") { REQUIRE_THROWS(VirtFsDir::removeFromSearchPath("dir1")); + REQUIRE_THROWS(VirtFsDir::removeFromSearchPath("dir1/")); } SECTION("simple 2") @@ -185,7 +186,7 @@ TEST_CASE("VirtFsDir removeFromSearchPath") REQUIRE(VirtFsDir::addToSearchPathSilent("dir1", Append_true, SkipError_true)); - REQUIRE(VirtFsDir::addToSearchPathSilent("dir2/dir3", + REQUIRE(VirtFsDir::addToSearchPathSilent("dir2//dir3", Append_true, SkipError_true)); REQUIRE(VirtFsDir::addToSearchPathSilent("dir3", @@ -239,26 +240,26 @@ TEST_CASE("VirtFsDir exists") { VirtFsDir::init("."); logger = new Logger(); - VirtFsDir::addToSearchPathSilent("data", + VirtFsDir::addToSearchPathSilent("data/", Append_false, SkipError_false); - VirtFsDir::addToSearchPathSilent("../data", + VirtFsDir::addToSearchPathSilent("..\\data", Append_false, SkipError_false); - REQUIRE(VirtFsDir::exists("test/units.xml") == true); - REQUIRE(VirtFsDir::exists("test/units123.xml") == false); + REQUIRE(VirtFsDir::exists("test//units.xml") == true); + REQUIRE(VirtFsDir::exists("test/\\units123.xml") == false); REQUIRE(VirtFsDir::exists("tesQ/units.xml") == false); REQUIRE(VirtFsDir::exists("units.xml") == false); - VirtFsDir::addToSearchPathSilent("data/test", + VirtFsDir::addToSearchPathSilent("data//test", Append_false, SkipError_false); - VirtFsDir::addToSearchPathSilent("../data/test", + VirtFsDir::addToSearchPathSilent("..//data\\test", Append_false, SkipError_false); - REQUIRE(VirtFsDir::exists("test/units.xml") == true); + REQUIRE(VirtFsDir::exists("test\\units.xml") == true); REQUIRE(VirtFsDir::exists("test/units123.xml") == false); REQUIRE(VirtFsDir::exists("tesQ/units.xml") == false); REQUIRE(VirtFsDir::exists("units.xml") == true); @@ -266,7 +267,7 @@ TEST_CASE("VirtFsDir exists") VirtFsDir::removeFromSearchPathSilent("data/test"); VirtFsDir::removeFromSearchPathSilent("../data/test"); - REQUIRE(VirtFsDir::exists("test/units.xml") == true); + REQUIRE(VirtFsDir::exists("test\\units.xml") == true); REQUIRE(VirtFsDir::exists("test/units123.xml") == false); REQUIRE(VirtFsDir::exists("tesQ/units.xml") == false); REQUIRE(VirtFsDir::exists("units.xml") == false); @@ -320,12 +321,20 @@ TEST_CASE("VirtFsDir getRealDir") REQUIRE(VirtFsDir::getRealDir("test") == "data"); REQUIRE(VirtFsDir::getRealDir("test/test.txt") == "data"); + REQUIRE(VirtFsDir::getRealDir("test\\test.txt") == + "data"); + REQUIRE(VirtFsDir::getRealDir("test//test.txt") == + "data"); } else { REQUIRE(VirtFsDir::getRealDir("test") == "../data"); REQUIRE(VirtFsDir::getRealDir("test/test.txt") == "../data"); + REQUIRE(VirtFsDir::getRealDir("test\\test.txt") == + "../data"); + REQUIRE(VirtFsDir::getRealDir("test//test.txt") == + "../data"); } REQUIRE(VirtFsDir::getRealDir("zzz") == ""); @@ -340,6 +349,8 @@ TEST_CASE("VirtFsDir getRealDir") REQUIRE(VirtFsDir::getRealDir("test") == "data"); REQUIRE(VirtFsDir::getRealDir("test/test.txt") == "data"); + REQUIRE(VirtFsDir::getRealDir("test\\test.txt") == + "data"); REQUIRE(VirtFsDir::getRealDir("test.txt") == "data/test"); } @@ -348,6 +359,8 @@ TEST_CASE("VirtFsDir getRealDir") REQUIRE(VirtFsDir::getRealDir("test") == "../data"); REQUIRE(VirtFsDir::getRealDir("test/test.txt") == "../data"); + REQUIRE(VirtFsDir::getRealDir("test\\test.txt") == + "../data"); REQUIRE(VirtFsDir::getRealDir("test.txt") == "../data/test"); } @@ -412,13 +425,13 @@ TEST_CASE("VirtFsDir enumerateFiles1") VirtFsDir::freeList(list); VirtFsDir::permitLinks(true); - list = VirtFsDir::enumerateFiles("test"); + list = VirtFsDir::enumerateFiles("test/"); removeTemp(list->names); REQUIRE(list->names.size() == cnt2); VirtFsDir::freeList(list); VirtFsDir::permitLinks(false); - list = VirtFsDir::enumerateFiles("test"); + list = VirtFsDir::enumerateFiles("test\\"); removeTemp(list->names); REQUIRE(list->names.size() == cnt1); VirtFsDir::freeList(list); @@ -509,7 +522,7 @@ TEST_CASE("VirtFsDir isDirectory") REQUIRE(VirtFsDir::isDirectory("test//dir1") == true); REQUIRE(VirtFsDir::isDirectory("test//dir1/") == true); REQUIRE(VirtFsDir::isDirectory("test//dir1//") == true); - REQUIRE(VirtFsDir::isDirectory("test/dir1/") == true); + REQUIRE(VirtFsDir::isDirectory("test\\dir1/") == true); REQUIRE(VirtFsDir::isDirectory("test/dir1//") == true); REQUIRE(VirtFsDir::isDirectory("testQ") == false); REQUIRE(VirtFsDir::isDirectory("testQ/") == false); @@ -531,6 +544,7 @@ TEST_CASE("VirtFsDir isDirectory") REQUIRE(VirtFsDir::isDirectory("test") == true); REQUIRE(VirtFsDir::isDirectory("testQ") == false); REQUIRE(VirtFsDir::isDirectory("test/dir1") == true); + REQUIRE(VirtFsDir::isDirectory("test\\dir1") == true); VirtFsDir::removeFromSearchPathSilent("data/test"); VirtFsDir::removeFromSearchPathSilent("../data/test"); @@ -567,6 +581,9 @@ TEST_CASE("VirtFsDir openRead") file = VirtFsDir::openRead("test/units.xml"); REQUIRE(file != nullptr); VirtFsDir::close(file); + file = VirtFsDir::openRead("test\\units.xml"); + REQUIRE(file != nullptr); + VirtFsDir::close(file); file = VirtFsDir::openRead("test/units123.xml"); REQUIRE(file == nullptr); file = VirtFsDir::openRead("tesQ/units.xml"); -- cgit v1.2.3-70-g09d2