Disallow more unsafe string->path conversions allowed by path append operators

ryanofsky · ryanofsky · commit 67ca71e5b7ec · 2022-03-03T15:02:11.000-05:00
Add more fs::path operator/ and operator+ overloads to prevent unsafe
string-&gt;path conversions on Windows that would cause strings to be
decoded according to the current Windows locale &amp; code page instead of
the correct string encoding.

Update application code to deal with loss of implicit string-&gt;path
conversions by calling fs::u8path or fs::PathFromString explicitly, or
by just changing variable types from std::string to fs::path to avoid
conversions altoghther, or make them happen earlier.

In all cases, there's no change in behavior either (1) because strings
only contained ASCII characters and would be decoded the same regardless
of what encoding was used, or (2) because of the 1:1 mapping between
paths and strings using the PathToString and PathFromString functions.
diff --git a/src/addrdb.cpp b/src/addrdb.cpp
@@ -53,7 +53,7 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
     std::string tmpfn = strprintf("%s.%04x", prefix, randv);
 
     // open temp output file, and associate with CAutoFile
-    fs::path pathTmp = gArgs.GetDataDirNet() / tmpfn;
+    fs::path pathTmp = gArgs.GetDataDirNet() / fs::u8path(tmpfn);
     FILE *file = fsbridge::fopen(pathTmp, "wb");
     CAutoFile fileout(file, SER_DISK, version);
     if (fileout.IsNull()) {
diff --git a/src/flatfile.cpp b/src/flatfile.cpp
@@ -27,7 +27,7 @@ std::string FlatFilePos::ToString() const
 
 fs::path FlatFileSeq::FileName(const FlatFilePos& pos) const
 {
-    return m_dir / strprintf("%s%05u.dat", m_prefix, pos.nFile);
+    return m_dir / fs::u8path(strprintf("%s%05u.dat", m_prefix, pos.nFile));
 }
 
 FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only)
diff --git a/src/fs.h b/src/fs.h
@@ -78,11 +78,35 @@ static inline auto quoted(const std::string& s)
 }
 
 // Allow safe path append operations.
-static inline path operator+(path p1, path p2)
+static inline path operator/(path p1, path p2)
 {
-    p1 += std::move(p2);
+    p1 /= std::move(p2);
     return p1;
 }
+static inline path operator/(path p1, std::filesystem::path p2)
+{
+    p1 /= std::move(p2);
+    return p1;
+}
+static inline path operator/(path p1, const char* p2)
+{
+    p1 /= p2;
+    return p1;
+}
+static inline path operator+(path p1, const char* p2)
+{
+    p1 += p2;
+    return p1;
+}
+static inline path operator+(path p1, path::value_type p2)
+{
+    p1 += p2;
+    return p1;
+}
+
+// Disallow unsafe path append operations.
+template<typename T> static inline path operator/(path p1, T p2) = delete;
+template<typename T> static inline path operator+(path p1, T p2) = delete;
 
 // Disallow implicit std::string conversion for copy_file
 // to avoid locale-dependent encoding on Windows.
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
@@ -100,7 +100,7 @@ BlockFilterIndex::BlockFilterIndex(BlockFilterType filter_type,
     const std::string& filter_name = BlockFilterTypeName(filter_type);
     if (filter_name.empty()) throw std::invalid_argument("unknown filter_type");
 
-    fs::path path = gArgs.GetDataDirNet() / "indexes" / "blockfilter" / filter_name;
+    fs::path path = gArgs.GetDataDirNet() / "indexes" / "blockfilter" / fs::u8path(filter_name);
     fs::create_directories(path);
 
     m_name = filter_name + " block filter index";
diff --git a/src/net.cpp b/src/net.cpp
@@ -3097,7 +3097,7 @@ void CaptureMessage(const CAddress& addr, const std::string& msg_type, const Spa
     std::string clean_addr = addr.ToString();
     std::replace(clean_addr.begin(), clean_addr.end(), ':', '_');
 
-    fs::path base_path = gArgs.GetDataDirNet() / "message_capture" / clean_addr;
+    fs::path base_path = gArgs.GetDataDirNet() / "message_capture" / fs::u8path(clean_addr);
     fs::create_directories(base_path);
 
     fs::path path = base_path / (is_incoming ? "msgs_recv.dat" : "msgs_sent.dat");
diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp
@@ -586,7 +586,7 @@ fs::path static GetAutostartFilePath()
     std::string chain = gArgs.GetChainName();
     if (chain == CBaseChainParams::MAIN)
         return GetAutostartDir() / "bitcoin.desktop";
-    return GetAutostartDir() / strprintf("bitcoin-%s.desktop", chain);
+    return GetAutostartDir() / fs::u8path(strprintf("bitcoin-%s.desktop", chain));
 }
 
 bool GetStartOnSystemStartup()
diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h
@@ -30,7 +30,7 @@ CreateAndActivateUTXOSnapshot(node::NodeContext& node, const fs::path root, F ma
     //
     int height;
     WITH_LOCK(::cs_main, height = node.chainman->ActiveHeight());
-    fs::path snapshot_path = root / tfm::format("test_snapshot.%d.dat", height);
+    fs::path snapshot_path = root / fs::u8path(tfm::format("test_snapshot.%d.dat", height));
     FILE* outfile{fsbridge::fopen(snapshot_path, "wb")};
     CAutoFile auto_outfile{outfile, SER_DISK, CLIENT_VERSION};
 
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
@@ -2014,7 +2014,7 @@ BOOST_AUTO_TEST_CASE(test_ParseFixedPoint)
     BOOST_CHECK(!ParseFixedPoint("31.999999999999999999999", 3, &amount));
 }
 
-static void TestOtherThread(fs::path dirname, std::string lockname, bool *result)
+static void TestOtherThread(fs::path dirname, fs::path lockname, bool *result)
 {
     *result = LockDirectory(dirname, lockname);
 }
@@ -2024,7 +2024,7 @@ static constexpr char LockCommand = 'L';
 static constexpr char UnlockCommand = 'U';
 static constexpr char ExitCommand = 'X';
 
-[[noreturn]] static void TestOtherProcess(fs::path dirname, std::string lockname, int fd)
+[[noreturn]] static void TestOtherProcess(fs::path dirname, fs::path lockname, int fd)
 {
     char ch;
     while (true) {
@@ -2055,7 +2055,7 @@ static constexpr char ExitCommand = 'X';
 BOOST_AUTO_TEST_CASE(test_LockDirectory)
 {
     fs::path dirname = m_args.GetDataDirBase() / "lock_dir";
-    const std::string lockname = ".lock";
+    const fs::path lockname = ".lock";
 #ifndef WIN32
     // Revert SIGCHLD to default, otherwise boost.test will catch and fail on
     // it: there is BOOST_TEST_IGNORE_SIGCHLD but that only works when defined
diff --git a/src/util/getuniquepath.cpp b/src/util/getuniquepath.cpp
@@ -9,6 +9,6 @@
 fs::path GetUniquePath(const fs::path& base)
 {
     FastRandomContext rnd;
-    fs::path tmpFile = base / HexStr(rnd.randbytes(8));
+    fs::path tmpFile = base / fs::u8path(HexStr(rnd.randbytes(8)));
     return tmpFile;
-}
+}
diff --git a/src/util/system.cpp b/src/util/system.cpp
@@ -95,7 +95,7 @@ static Mutex cs_dir_locks;
  */
 static std::map<std::string, std::unique_ptr<fsbridge::FileLock>> dir_locks GUARDED_BY(cs_dir_locks);
 
-bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only)
+bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only)
 {
     LOCK(cs_dir_locks);
     fs::path pathLockFile = directory / lockfile_name;
@@ -119,7 +119,7 @@ bool LockDirectory(const fs::path& directory, const std::string lockfile_name, b
     return true;
 }
 
-void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name)
+void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name)
 {
     LOCK(cs_dir_locks);
     dir_locks.erase(fs::PathToString(directory / lockfile_name));
diff --git a/src/util/system.h b/src/util/system.h
@@ -76,8 +76,8 @@ void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length);
  */
 [[nodiscard]] bool RenameOver(fs::path src, fs::path dest);
 
-bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only=false);
-void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name);
+bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only=false);
+void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name);
 bool DirIsWritable(const fs::path& directory);
 bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes = 0);
 
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -1428,7 +1428,7 @@ CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams)
 }
 
 CoinsViews::CoinsViews(
-    std::string ldb_name,
+    fs::path ldb_name,
     size_t cache_size_bytes,
     bool in_memory,
     bool should_wipe) : m_dbview(
@@ -1456,7 +1456,7 @@ void CChainState::InitCoinsDB(
     size_t cache_size_bytes,
     bool in_memory,
     bool should_wipe,
-    std::string leveldb_name)
+    fs::path leveldb_name)
 {
     if (m_from_snapshot_blockhash) {
         leveldb_name += "_" + m_from_snapshot_blockhash->ToString();
diff --git a/src/validation.h b/src/validation.h
@@ -427,7 +427,7 @@ class CoinsViews {
     //! state to disk, which should not be done until the health of the database is verified.
     //!
     //! All arguments forwarded onto CCoinsViewDB.
-    CoinsViews(std::string ldb_name, size_t cache_size_bytes, bool in_memory, bool should_wipe);
+    CoinsViews(fs::path ldb_name, size_t cache_size_bytes, bool in_memory, bool should_wipe);
 
     //! Initialize the CCoinsViewCache member.
     void InitCache() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
@@ -521,7 +521,7 @@ class CChainState
         size_t cache_size_bytes,
         bool in_memory,
         bool should_wipe,
-        std::string leveldb_name = "chainstate");
+        fs::path leveldb_name = "chainstate");
 
     //! Initialize the in-memory coins cache (to be done after the health of the on-disk database
     //! is verified).
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
@@ -260,7 +260,7 @@ BerkeleyBatch::SafeDbt::operator Dbt*()
 bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
 {
     fs::path walletDir = env->Directory();
-    fs::path file_path = walletDir / strFile;
+    fs::path file_path = walletDir / m_filename;
 
     LogPrintf("Using BerkeleyDB version %s\n", BerkeleyDatabaseVersion());
     LogPrintf("Using wallet %s\n", fs::PathToString(file_path));
@@ -274,6 +274,7 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
         assert(m_refcount == 0);
 
         Db db(env->dbenv.get(), 0);
+        const std::string strFile = fs::PathToString(m_filename);
         int result = db.verify(strFile.c_str(), nullptr, nullptr, 0);
         if (result != 0) {
             errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), fs::quoted(fs::PathToString(file_path)));
@@ -296,11 +297,11 @@ BerkeleyDatabase::~BerkeleyDatabase()
 {
     if (env) {
         LOCK(cs_db);
-        env->CloseDb(strFile);
+        env->CloseDb(m_filename);
         assert(!m_db);
-        size_t erased = env->m_databases.erase(strFile);
+        size_t erased = env->m_databases.erase(m_filename);
         assert(erased == 1);
-        env->m_fileids.erase(strFile);
+        env->m_fileids.erase(m_filename);
     }
 }
 
@@ -312,7 +313,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, b
     fFlushOnClose = fFlushOnCloseIn;
     env = database.env.get();
     pdb = database.m_db.get();
-    strFile = database.strFile;
+    strFile = fs::PathToString(database.m_filename);
     if (!Exists(std::string("version"))) {
         bool fTmp = fReadOnly;
         fReadOnly = false;
@@ -332,6 +333,8 @@ void BerkeleyDatabase::Open()
             throw std::runtime_error("BerkeleyDatabase: Failed to open database environment.");
 
         if (m_db == nullptr) {
+            const std::string strFile = fs::PathToString(m_filename);
+
             int ret;
             std::unique_ptr<Db> pdb_temp = std::make_unique<Db>(env->dbenv.get(), 0);
 
@@ -406,11 +409,11 @@ void BerkeleyBatch::Close()
         Flush();
 }
 
-void BerkeleyEnvironment::CloseDb(const std::string& strFile)
+void BerkeleyEnvironment::CloseDb(const fs::path& filename)
 {
     {
         LOCK(cs_db);
-        auto it = m_databases.find(strFile);
+        auto it = m_databases.find(filename);
         assert(it != m_databases.end());
         BerkeleyDatabase& database = it->second.get();
         if (database.m_db) {
@@ -433,12 +436,12 @@ void BerkeleyEnvironment::ReloadDbEnv()
         return true;
     });
 
-    std::vector<std::string> filenames;
+    std::vector<fs::path> filenames;
     for (auto it : m_databases) {
         filenames.push_back(it.first);
     }
     // Close the individual Db's
-    for (const std::string& filename : filenames) {
+    for (const fs::path& filename : filenames) {
         CloseDb(filename);
     }
     // Reset the environment
@@ -453,10 +456,11 @@ bool BerkeleyDatabase::Rewrite(const char* pszSkip)
     while (true) {
         {
             LOCK(cs_db);
+            const std::string strFile = fs::PathToString(m_filename);
             if (m_refcount <= 0) {
                 // Flush log data to the dat file
-                env->CloseDb(strFile);
-                env->CheckpointLSN(strFile);
+                env->CloseDb(m_filename);
+                env->CheckpointLSN(m_filename);
                 m_refcount = -1;
 
                 bool fSuccess = true;
@@ -507,7 +511,7 @@ bool BerkeleyDatabase::Rewrite(const char* pszSkip)
                     }
                     if (fSuccess) {
                         db.Close();
-                        env->CloseDb(strFile);
+                        env->CloseDb(m_filename);
                         if (pdbCopy->close(0))
                             fSuccess = false;
                     } else {
@@ -543,13 +547,14 @@ void BerkeleyEnvironment::Flush(bool fShutdown)
         LOCK(cs_db);
         bool no_dbs_accessed = true;
         for (auto& db_it : m_databases) {
-            std::string strFile = db_it.first;
+            const fs::path& filename = db_it.first;
+            const std::string strFile = fs::PathToString(filename);
             int nRefCount = db_it.second.get().m_refcount;
             if (nRefCount < 0) continue;
             LogPrint(BCLog::WALLETDB, "BerkeleyEnvironment::Flush: Flushing %s (refcount = %d)...\n", strFile, nRefCount);
             if (nRefCount == 0) {
                 // Move log data to the dat file
-                CloseDb(strFile);
+                CloseDb(filename);
                 LogPrint(BCLog::WALLETDB, "BerkeleyEnvironment::Flush: %s checkpoint\n", strFile);
                 dbenv->txn_checkpoint(0, 0, 0);
                 LogPrint(BCLog::WALLETDB, "BerkeleyEnvironment::Flush: %s detach\n", strFile);
@@ -589,11 +594,12 @@ bool BerkeleyDatabase::PeriodicFlush()
     // Don't flush if there haven't been any batch writes for this database.
     if (m_refcount < 0) return false;
 
+    const std::string strFile = fs::PathToString(m_filename);
     LogPrint(BCLog::WALLETDB, "Flushing %s\n", strFile);
     int64_t nStart = GetTimeMillis();
 
     // Flush wallet file so it's self contained
-    env->CloseDb(strFile);
+    env->CloseDb(m_filename);
     env->CheckpointLSN(strFile);
     m_refcount = -1;
 
@@ -611,15 +617,16 @@ bool BerkeleyDatabase::Backup(const std::string& strDest) const
             if (m_refcount <= 0)
             {
                 // Flush log data to the dat file
-                env->CloseDb(strFile);
-                env->CheckpointLSN(strFile);
+                env->CloseDb(m_filename);
+                env->CheckpointLSN(m_filename);
 
                 // Copy wallet file
-                fs::path pathSrc = env->Directory() / strFile;
+                fs::path pathSrc = env->Directory() / m_filename;
                 fs::path pathDest(fs::PathFromString(strDest));
                 if (fs::is_directory(pathDest))
-                    pathDest /= fs::PathFromString(strFile);
+                    pathDest /= m_filename;
 
+                const std::string strFile = fs::PathToString(m_filename);
                 try {
                     if (fs::exists(pathDest) && fs::equivalent(pathSrc, pathDest)) {
                         LogPrintf("cannot backup to wallet source file %s\n", fs::PathToString(pathDest));
@@ -830,7 +837,7 @@ std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, con
     std::unique_ptr<BerkeleyDatabase> db;
     {
         LOCK(cs_db); // Lock env.m_databases until insert in BerkeleyDatabase constructor
-        std::string data_filename = fs::PathToString(data_file.filename());
+        fs::path data_filename = data_file.filename();
         std::shared_ptr<BerkeleyEnvironment> env = GetBerkeleyEnv(data_file.parent_path());
         if (env->m_databases.count(data_filename)) {
             error = Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", fs::PathToString(env->Directory() / data_filename)));
diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h
diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp
diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp
diff --git a/src/wallet/test/init_test_fixture.cpp b/src/wallet/test/init_test_fixture.cpp

Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ std::string FlatFilePos::ToString() const`
`27`	`27`
`28`	`28`	`fs::path FlatFileSeq::FileName(const FlatFilePos& pos) const`
`29`	`29`	`{`
`30`		`- return m_dir / strprintf("%s%05u.dat", m_prefix, pos.nFile);`
	`30`	`+ return m_dir / fs::u8path(strprintf("%s%05u.dat", m_prefix, pos.nFile));`
`31`	`31`	`}`
`32`	`32`
`33`	`33`	`FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only)`
Original file line number	Diff line number	Diff line change
`@@ -586,7 +586,7 @@ fs::path static GetAutostartFilePath()`
`586`	`586`	`std::string chain = gArgs.GetChainName();`
`587`	`587`	`if (chain == CBaseChainParams::MAIN)`
`588`	`588`	`return GetAutostartDir() / "bitcoin.desktop";`
`589`		`- return GetAutostartDir() / strprintf("bitcoin-%s.desktop", chain);`
	`589`	`+ return GetAutostartDir() / fs::u8path(strprintf("bitcoin-%s.desktop", chain));`
`590`	`590`	`}`
`591`	`591`
`592`	`592`	`bool GetStartOnSystemStartup()`
Original file line number	Diff line number	Diff line change
`@@ -2014,7 +2014,7 @@ BOOST_AUTO_TEST_CASE(test_ParseFixedPoint)`
`2014`	`2014`	`BOOST_CHECK(!ParseFixedPoint("31.999999999999999999999", 3, &amount));`
`2015`	`2015`	`}`
`2016`	`2016`
`2017`		`-static void TestOtherThread(fs::path dirname, std::string lockname, bool *result)`
	`2017`	`+static void TestOtherThread(fs::path dirname, fs::path lockname, bool *result)`
`2018`	`2018`	`{`
`2019`	`2019`	`*result = LockDirectory(dirname, lockname);`
`2020`	`2020`	`}`
`@@ -2024,7 +2024,7 @@ static constexpr char LockCommand = 'L';`
`2024`	`2024`	`static constexpr char UnlockCommand = 'U';`
`2025`	`2025`	`static constexpr char ExitCommand = 'X';`
`2026`	`2026`
`2027`		`-[[noreturn]] static void TestOtherProcess(fs::path dirname, std::string lockname, int fd)`
	`2027`	`+[[noreturn]] static void TestOtherProcess(fs::path dirname, fs::path lockname, int fd)`
`2028`	`2028`	`{`
`2029`	`2029`	`char ch;`
`2030`	`2030`	`while (true) {`
`@@ -2055,7 +2055,7 @@ static constexpr char ExitCommand = 'X';`
`2055`	`2055`	`BOOST_AUTO_TEST_CASE(test_LockDirectory)`
`2056`	`2056`	`{`
`2057`	`2057`	`fs::path dirname = m_args.GetDataDirBase() / "lock_dir";`
`2058`		`- const std::string lockname = ".lock";`
	`2058`	`+ const fs::path lockname = ".lock";`
`2059`	`2059`	`#ifndef WIN32`
`2060`	`2060`	`// Revert SIGCHLD to default, otherwise boost.test will catch and fail on`
`2061`	`2061`	`// it: there is BOOST_TEST_IGNORE_SIGCHLD but that only works when defined`
Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,6 @@`
`9`	`9`	`fs::path GetUniquePath(const fs::path& base)`
`10`	`10`	`{`
`11`	`11`	`FastRandomContext rnd;`
`12`		`- fs::path tmpFile = base / HexStr(rnd.randbytes(8));`
	`12`	`+ fs::path tmpFile = base / fs::u8path(HexStr(rnd.randbytes(8)));`
`13`	`13`	`return tmpFile;`
`14`		`-}`
	`14`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ static Mutex cs_dir_locks;`
`95`	`95`	`*/`
`96`	`96`	`static std::map<std::string, std::unique_ptr<fsbridge::FileLock>> dir_locks GUARDED_BY(cs_dir_locks);`
`97`	`97`
`98`		`-bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only)`
	`98`	`+bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only)`
`99`	`99`	`{`
`100`	`100`	`LOCK(cs_dir_locks);`
`101`	`101`	`fs::path pathLockFile = directory / lockfile_name;`
`@@ -119,7 +119,7 @@ bool LockDirectory(const fs::path& directory, const std::string lockfile_name, b`
`119`	`119`	`return true;`
`120`	`120`	`}`
`121`	`121`
`122`		`-void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name)`
	`122`	`+void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name)`
`123`	`123`	`{`
`124`	`124`	`LOCK(cs_dir_locks);`
`125`	`125`	`dir_locks.erase(fs::PathToString(directory / lockfile_name));`