From bc9d620155fca38b2a495fe3080ef085e9179f41 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 31 Jul 2019 21:21:13 +0300 Subject: [PATCH 1/2] Fixed the case when malicious ClickHouse replica can force clickhouse-server to write to arbitrary path --- dbms/src/Common/ErrorCodes.cpp | 1 + dbms/src/Storages/MergeTree/DataPartsExchange.cpp | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/dbms/src/Common/ErrorCodes.cpp b/dbms/src/Common/ErrorCodes.cpp index 5cb6b7e0a37..bb1636b7871 100644 --- 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 index 42c668c9fcb..ee7de070776 100644 --- 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 @@ -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 file_absolute_path = Poco::Path(absolute_part_path + file_name).absolute().toString(); + if (!startsWith(file_absolute_path, absolute_part_path)) + throw Exception("File path doesn't appear to be inside part path." + " This may happen if we are trying to download part from malicious replica or logical error.", + ErrorCodes::INSECURE_PATH); + + WriteBufferFromFile file_out(file_absolute_path); HashingWriteBuffer hashing_out(file_out); copyData(in, hashing_out, file_size, blocker.getCounter()); From 12f6b75284d4dd8b786671852dad915f9bdc5dee Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 1 Aug 2019 05:34:58 +0300 Subject: [PATCH 2/2] Fixed error --- dbms/src/Storages/MergeTree/DataPartsExchange.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dbms/src/Storages/MergeTree/DataPartsExchange.cpp b/dbms/src/Storages/MergeTree/DataPartsExchange.cpp index ee7de070776..80e521b32c1 100644 --- a/dbms/src/Storages/MergeTree/DataPartsExchange.cpp +++ b/dbms/src/Storages/MergeTree/DataPartsExchange.cpp @@ -201,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()) @@ -228,13 +228,13 @@ MergeTreeData::MutableDataPartPtr Fetcher::fetchPart( /// File must be inside "absolute_part_path" directory. /// Otherwise malicious ClickHouse replica may force us to write to arbitrary path. - String file_absolute_path = Poco::Path(absolute_part_path + file_name).absolute().toString(); - if (!startsWith(file_absolute_path, absolute_part_path)) - throw Exception("File path doesn't appear to be inside part 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(file_absolute_path); + WriteBufferFromFile file_out(absolute_file_path); HashingWriteBuffer hashing_out(file_out); copyData(in, hashing_out, file_size, blocker.getCounter());