Merge pull request #6247 from yandex/fix-malicious-replica

Fixed the case when malicious ClickHouse replica can force clickhouse-server to write to arbitrary path.
ClickHouse · Aug 1, 2019 · eeeab85 · eeeab85
2 parents 591af05 + 12f6b75
commit eeeab85
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 2 deletions.
diff --git a/dbms/src/Common/ErrorCodes.cpp b/dbms/src/Common/ErrorCodes.cpp
@@ -440,6 +440,7 @@ namespace ErrorCodes
     extern const int CANNOT_FCNTL = 463;
     extern const int CANNOT_PARSE_ELF = 464;
     extern const int CANNOT_PARSE_DWARF = 465;
+    extern const int INSECURE_PATH = 466;
 
     extern const int KEEPER_EXCEPTION = 999;
     extern const int POCO_EXCEPTION = 1000;

diff --git a/dbms/src/Storages/MergeTree/DataPartsExchange.cpp b/dbms/src/Storages/MergeTree/DataPartsExchange.cpp
@@ -27,6 +27,7 @@ namespace ErrorCodes
     extern const int CANNOT_WRITE_TO_OSTREAM;
     extern const int CHECKSUM_DOESNT_MATCH;
     extern const int UNKNOWN_TABLE;
+    extern const int INSECURE_PATH;
 }
 
 namespace DataPartsExchange
@@ -200,7 +201,7 @@ MergeTreeData::MutableDataPartPtr Fetcher::fetchPart(
     String tmp_prefix = tmp_prefix_.empty() ? TMP_PREFIX : tmp_prefix_;
 
     String relative_part_path = String(to_detached ? "detached/" : "") + tmp_prefix + part_name;
-    String absolute_part_path = data.getFullPath() + relative_part_path + "/";
+    String absolute_part_path = Poco::Path(data.getFullPath() + relative_part_path + "/").absolute().toString();
     Poco::File part_file(absolute_part_path);
 
     if (part_file.exists())
@@ -225,7 +226,15 @@ MergeTreeData::MutableDataPartPtr Fetcher::fetchPart(
         readStringBinary(file_name, in);
         readBinary(file_size, in);
 
-        WriteBufferFromFile file_out(absolute_part_path + file_name);
+        /// File must be inside "absolute_part_path" directory.
+        /// Otherwise malicious ClickHouse replica may force us to write to arbitrary path.
+        String absolute_file_path = Poco::Path(absolute_part_path + file_name).absolute().toString();
+        if (!startsWith(absolute_file_path, absolute_part_path))
+            throw Exception("File path (" + absolute_file_path + ") doesn't appear to be inside part path (" + absolute_part_path + ")."
+                " This may happen if we are trying to download part from malicious replica or logical error.",
+                ErrorCodes::INSECURE_PATH);
+
+        WriteBufferFromFile file_out(absolute_file_path);
         HashingWriteBuffer hashing_out(file_out);
         copyData(in, hashing_out, file_size, blocker.getCounter());