Skip to content

Commit 3e564ca

Browse files
authored
fix(replication): reject unsafe fullsync file names (#3483)
Replication fullsync uses the peer's file names to fetch checkpoint files and materialize them in the local sync checkpoint directory. Previously, those names were joined directly with local directories, so traversal components could escape the intended directory during existence checks, temporary file creation, rename, cleanup, or master-side file open. Assisted-by: Codex/GPT 5.5 xhigh
1 parent 47e0893 commit 3e564ca

4 files changed

Lines changed: 94 additions & 0 deletions

File tree

src/cluster/replication.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -847,6 +847,10 @@ ReplicationThread::CBState ReplicationThread::fullSyncReadCB(bufferevent *bev) {
847847
}
848848
std::vector<std::string> need_files = util::Split(std::string(line.get()), ",");
849849
for (const auto &f : need_files) {
850+
if (auto s = engine::Storage::ReplDataManager::ValidateReplFileName(f); !s.IsOK()) {
851+
WARN("[replication] Ignored the invalid fullsync file name '{}': {}", f, s.Msg());
852+
continue;
853+
}
850854
meta.files.emplace_back(f, 0);
851855
}
852856

src/storage/storage.cc

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
#include <iostream>
3535
#include <memory>
3636
#include <random>
37+
#include <string_view>
3738

3839
#include "compact_filter.h"
3940
#include "db_util.h"
@@ -1226,7 +1227,40 @@ Status Storage::ReplDataManager::CleanInvalidFiles(Storage *storage, const std::
12261227
return ret;
12271228
}
12281229

1230+
Status Storage::ReplDataManager::ValidateReplFileName(const std::string &repl_file) {
1231+
if (repl_file.empty()) {
1232+
return {Status::NotOK, "empty replication file name"};
1233+
}
1234+
if (repl_file.front() == '/') {
1235+
return {Status::NotOK, fmt::format("absolute replication file name '{}' is not allowed", repl_file)};
1236+
}
1237+
if (repl_file.back() == '/') {
1238+
return {Status::NotOK, fmt::format("unsafe replication file name '{}' is not allowed", repl_file)};
1239+
}
1240+
1241+
for (size_t begin = 0; begin < repl_file.size();) {
1242+
auto end = repl_file.find('/', begin);
1243+
auto component = std::string_view(repl_file).substr(begin, end - begin);
1244+
if (component.empty() || component == "." || component == "..") {
1245+
return {Status::NotOK, fmt::format("unsafe replication file name '{}' is not allowed", repl_file)};
1246+
}
1247+
if (component.find('\0') != std::string_view::npos || component.find('\\') != std::string_view::npos ||
1248+
component.find(':') != std::string_view::npos) {
1249+
return {Status::NotOK, fmt::format("unsafe replication file name '{}' is not allowed", repl_file)};
1250+
}
1251+
if (end == std::string::npos) break;
1252+
begin = end + 1;
1253+
}
1254+
1255+
return Status::OK();
1256+
}
1257+
12291258
int Storage::ReplDataManager::OpenDataFile(Storage *storage, const std::string &repl_file, uint64_t *file_size) {
1259+
if (auto s = ValidateReplFileName(repl_file); !s.IsOK()) {
1260+
ERROR("[storage] Invalid replication data file '{}': {}", repl_file, s.Msg());
1261+
return NullFD;
1262+
}
1263+
12301264
std::string abs_path = storage->config_->checkpoint_dir + "/" + repl_file;
12311265
auto s = storage->env_->FileExists(abs_path);
12321266
if (!s.ok()) {
@@ -1282,6 +1316,9 @@ Status Storage::ReplDataManager::ParseMetaAndSave(Storage *storage, rocksdb::Bac
12821316
}
12831317

12841318
auto filename = std::string(line.get(), cptr - line.get() - 1);
1319+
if (auto s = ValidateReplFileName(filename); !s.IsOK()) {
1320+
return s;
1321+
}
12851322
while (*(cptr++) != ' ') {
12861323
}
12871324

@@ -1311,6 +1348,11 @@ Status MkdirRecursively(rocksdb::Env *env, const std::string &dir) {
13111348

13121349
std::unique_ptr<rocksdb::WritableFile> Storage::ReplDataManager::NewTmpFile(Storage *storage, const std::string &dir,
13131350
const std::string &repl_file) {
1351+
if (auto s = ValidateReplFileName(repl_file); !s.IsOK()) {
1352+
ERROR("[storage] Invalid replication data file '{}': {}", repl_file, s.Msg());
1353+
return nullptr;
1354+
}
1355+
13141356
std::string tmp_file = dir + "/" + repl_file + ".tmp";
13151357
auto s = storage->env_->FileExists(tmp_file);
13161358
if (s.ok()) {
@@ -1335,6 +1377,10 @@ std::unique_ptr<rocksdb::WritableFile> Storage::ReplDataManager::NewTmpFile(Stor
13351377
}
13361378

13371379
Status Storage::ReplDataManager::SwapTmpFile(Storage *storage, const std::string &dir, const std::string &repl_file) {
1380+
if (auto s = ValidateReplFileName(repl_file); !s.IsOK()) {
1381+
return s;
1382+
}
1383+
13381384
std::string tmp_file = dir + "/" + repl_file + ".tmp";
13391385
std::string orig_file = dir + "/" + repl_file;
13401386

@@ -1349,6 +1395,7 @@ Status Storage::ReplDataManager::SwapTmpFile(Storage *storage, const std::string
13491395
bool Storage::ReplDataManager::FileExists(Storage *storage, const std::string &dir, const std::string &repl_file,
13501396
uint32_t crc) {
13511397
if (storage->IsClosing()) return false;
1398+
if (!ValidateReplFileName(repl_file).IsOK()) return false;
13521399

13531400
auto file_path = dir + "/" + repl_file;
13541401
auto s = storage->env_->FileExists(file_path);

src/storage/storage.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,8 @@ class Storage {
320320
// Full replication data files manager
321321
class ReplDataManager {
322322
public:
323+
static Status ValidateReplFileName(const std::string &repl_file);
324+
323325
// Master side
324326
static Status GetFullReplDataInfo(Storage *storage, std::string *files);
325327
static int OpenDataFile(Storage *storage, const std::string &rel_file, uint64_t *file_size);

tests/cppunit/storage_test.cc

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626

2727
#include <filesystem>
2828
#include <fstream>
29+
#include <string>
30+
#include <vector>
2931

3032
TEST(Storage, CreateBackup) {
3133
std::error_code ec;
@@ -129,3 +131,42 @@ TEST(Storage, RocksDBDictionaryCompressionOptions) {
129131

130132
unlink(path);
131133
}
134+
135+
TEST(Storage, ReplDataManagerRejectsUnsafeFilenames) {
136+
Config config;
137+
config.db_dir = "test_repl_file_validation_dir/db";
138+
config.checkpoint_dir = "test_repl_file_validation_dir/checkpoint";
139+
config.slot_id_encoded = false;
140+
141+
std::error_code ec;
142+
std::filesystem::remove_all("test_repl_file_validation_dir", ec);
143+
ASSERT_FALSE(ec);
144+
145+
engine::Storage storage(&config);
146+
auto base_dir = std::string("test_repl_file_validation_dir/sync_checkpoint");
147+
148+
auto valid_file = engine::Storage::ReplDataManager::NewTmpFile(&storage, base_dir, "meta/1");
149+
ASSERT_TRUE(valid_file);
150+
ASSERT_TRUE(valid_file->Close().ok());
151+
ASSERT_TRUE(engine::Storage::ReplDataManager::SwapTmpFile(&storage, base_dir, "meta/1").IsOK());
152+
EXPECT_TRUE(std::filesystem::exists("test_repl_file_validation_dir/sync_checkpoint/meta/1"));
153+
154+
const std::vector<std::string> unsafe_files = {
155+
"../escape", "a/../../escape", "/tmp/escape", "a//b", "a/",
156+
".", "a/..", "a\\b", "C:/escape", std::string("bad\0name", 8),
157+
};
158+
for (const auto &file : unsafe_files) {
159+
EXPECT_FALSE(engine::Storage::ReplDataManager::ValidateReplFileName(file).IsOK()) << file;
160+
EXPECT_FALSE(engine::Storage::ReplDataManager::NewTmpFile(&storage, base_dir, file)) << file;
161+
EXPECT_FALSE(engine::Storage::ReplDataManager::SwapTmpFile(&storage, base_dir, file).IsOK()) << file;
162+
EXPECT_FALSE(engine::Storage::ReplDataManager::FileExists(&storage, base_dir, file, 0)) << file;
163+
uint64_t file_size = 0;
164+
EXPECT_LT(engine::Storage::ReplDataManager::OpenDataFile(&storage, file, &file_size), 0) << file;
165+
}
166+
167+
EXPECT_FALSE(std::filesystem::exists("test_repl_file_validation_dir/escape.tmp"));
168+
EXPECT_FALSE(std::filesystem::exists("test_repl_file_validation_dir/escape"));
169+
170+
std::filesystem::remove_all("test_repl_file_validation_dir", ec);
171+
ASSERT_FALSE(ec);
172+
}

0 commit comments

Comments
 (0)