From c9ab39c8c6b3c496d349301345c8449e087c8f80 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 1 Aug 2018 00:10:16 +0300 Subject: [PATCH] Added validation of ODBC connection string [#CLICKHOUSE-3861] --- dbms/src/Common/ErrorCodes.cpp | 2 +- dbms/src/Dictionaries/CMakeLists.txt | 3 + .../src/Dictionaries/ODBCDictionarySource.cpp | 3 +- dbms/src/Dictionaries/tests/CMakeLists.txt | 2 + .../tests/validate-odbc-connection-string.cpp | 24 ++ .../validate-odbc-connection-string.reference | 36 +++ .../tests/validate-odbc-connection-string.sh | 38 +++ .../validateODBCConnectionString.cpp | 224 ++++++++++++++++++ .../validateODBCConnectionString.h | 21 ++ dbms/src/Storages/StorageODBC.cpp | 3 +- dbms/src/TableFunctions/TableFunctionODBC.cpp | 1 + 11 files changed, 354 insertions(+), 3 deletions(-) create mode 100644 dbms/src/Dictionaries/tests/CMakeLists.txt create mode 100644 dbms/src/Dictionaries/tests/validate-odbc-connection-string.cpp create mode 100644 dbms/src/Dictionaries/tests/validate-odbc-connection-string.reference create mode 100755 dbms/src/Dictionaries/tests/validate-odbc-connection-string.sh create mode 100644 dbms/src/Dictionaries/validateODBCConnectionString.cpp create mode 100644 dbms/src/Dictionaries/validateODBCConnectionString.h diff --git a/dbms/src/Common/ErrorCodes.cpp b/dbms/src/Common/ErrorCodes.cpp index a789a60275..9794e32949 100644 --- a/dbms/src/Common/ErrorCodes.cpp +++ b/dbms/src/Common/ErrorCodes.cpp @@ -378,7 +378,7 @@ namespace ErrorCodes extern const int FEATURE_IS_NOT_ENABLED_AT_BUILD_TIME = 401; extern const int CANNOT_IOSETUP = 402; extern const int INVALID_JOIN_ON_EXPRESSION = 403; - + extern const int BAD_ODBC_CONNECTION_STRING = 404; extern const int KEEPER_EXCEPTION = 999; extern const int POCO_EXCEPTION = 1000; diff --git a/dbms/src/Dictionaries/CMakeLists.txt b/dbms/src/Dictionaries/CMakeLists.txt index e69de29bb2..6517235664 100644 --- a/dbms/src/Dictionaries/CMakeLists.txt +++ b/dbms/src/Dictionaries/CMakeLists.txt @@ -0,0 +1,3 @@ +if (ENABLE_TESTS) + add_subdirectory (tests) +endif () diff --git a/dbms/src/Dictionaries/ODBCDictionarySource.cpp b/dbms/src/Dictionaries/ODBCDictionarySource.cpp index 0d5176c2bb..03c354d4bc 100644 --- a/dbms/src/Dictionaries/ODBCDictionarySource.cpp +++ b/dbms/src/Dictionaries/ODBCDictionarySource.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -39,7 +40,7 @@ ODBCDictionarySource::ODBCDictionarySource(const DictionaryStructure & dict_stru { auto session = std::make_shared( config.getString(config_prefix + ".connector", "ODBC"), - config.getString(config_prefix + ".connection_string")); + validateODBCConnectionString(config.getString(config_prefix + ".connection_string"))); /// Default POCO value is 1024. Set property manually to make possible reading of longer strings. session->setProperty("maxFieldSize", Poco::Any(field_size)); diff --git a/dbms/src/Dictionaries/tests/CMakeLists.txt b/dbms/src/Dictionaries/tests/CMakeLists.txt new file mode 100644 index 0000000000..f0a4cf4ab6 --- /dev/null +++ b/dbms/src/Dictionaries/tests/CMakeLists.txt @@ -0,0 +1,2 @@ +add_executable (validate-odbc-connection-string validate-odbc-connection-string.cpp) +target_link_libraries (validate-odbc-connection-string dbms) diff --git a/dbms/src/Dictionaries/tests/validate-odbc-connection-string.cpp b/dbms/src/Dictionaries/tests/validate-odbc-connection-string.cpp new file mode 100644 index 0000000000..766a709d8f --- /dev/null +++ b/dbms/src/Dictionaries/tests/validate-odbc-connection-string.cpp @@ -0,0 +1,24 @@ +#include +#include +#include + + +using namespace DB; + +int main(int argc, char ** argv) +try +{ + if (argc < 2) + { + std::cerr << "Usage: validate-odbc-connection-string 'ConnectionString'\n"; + return 1; + } + + std::cout << validateODBCConnectionString(argv[1]) << '\n'; + return 0; +} +catch (...) +{ + std::cerr << getCurrentExceptionMessage(false) << "\n"; + return 2; +} diff --git a/dbms/src/Dictionaries/tests/validate-odbc-connection-string.reference b/dbms/src/Dictionaries/tests/validate-odbc-connection-string.reference new file mode 100644 index 0000000000..aae9f91a40 --- /dev/null +++ b/dbms/src/Dictionaries/tests/validate-odbc-connection-string.reference @@ -0,0 +1,36 @@ +Code: 404, e.displayText() = DB::Exception: ODBC connection string cannot be empty, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter doesn't have value, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: DSN parameter is mandatory for ODBC connection string, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter doesn't have value, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: DSN parameter is mandatory for ODBC connection string, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter value is unescaped and contains illegal character, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: DSN parameter is mandatory for ODBC connection string, e.what() = DB::Exception +DSN={hello};ABC={de[f}; +DSN={hello};ABC={de}}f}; +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter value is unescaped and contains illegal character, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter is escaped but there is no closing curly brace, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: Unexpected character found after parameter value in ODBC connection string, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: Unexpected character found after parameter value in ODBC connection string, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: Unexpected character found after parameter value in ODBC connection string, e.what() = DB::Exception +DSN={hello};ABC={de}}f}; +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter is escaped but there is no closing curly brace, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter is escaped but there is no closing curly brace, e.what() = DB::Exception +DSN={hello};ABC={ }; +DSN={hello};ABC={ }; +Code: 404, e.displayText() = DB::Exception: Unexpected character found after parameter value in ODBC connection string, e.what() = DB::Exception +DSN={hello world};ABC={ }; +Code: 404, e.displayText() = DB::Exception: Unexpected character found after parameter value in ODBC connection string, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter name doesn't begin with valid identifier character, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter name doesn't begin with valid identifier character, e.what() = DB::Exception +DSN={hello world};ABC={ };_={}; +DSN={hello world};ABC={ };_={}; +DSN={hello world};ABC={ };_={}; +DSN={hello world};ABC={ };_={}}}; +DSN={hello world};ABC={ };_={...................................................................}; +DSN={hello world};ABC={ };_={....................................................................................}; +Code: 404, e.displayText() = DB::Exception: ODBC connection string has too long keyword or value, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string has forbidden parameter, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string has forbidden parameter, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string has forbidden parameter, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: Duplicate parameter found in ODBC connection string, e.what() = DB::Exception +Code: 404, e.displayText() = DB::Exception: ODBC connection string parameter name doesn't begin with valid identifier character, e.what() = DB::Exception diff --git a/dbms/src/Dictionaries/tests/validate-odbc-connection-string.sh b/dbms/src/Dictionaries/tests/validate-odbc-connection-string.sh new file mode 100755 index 0000000000..09baaa3587 --- /dev/null +++ b/dbms/src/Dictionaries/tests/validate-odbc-connection-string.sh @@ -0,0 +1,38 @@ +#!/bin/sh + +./validate-odbc-connection-string '' 2>&1 +./validate-odbc-connection-string 'abc' 2>&1 +./validate-odbc-connection-string 'abc=' 2>&1 +./validate-odbc-connection-string 'ab"c=' 2>&1 +./validate-odbc-connection-string 'abc=def' 2>&1 +./validate-odbc-connection-string 'abc=de[f' 2>&1 +./validate-odbc-connection-string 'abc={de[f}' 2>&1 +./validate-odbc-connection-string 'abc={de[f};dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={de}}f};dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc=de}}f};dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={de}}f;dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={de}f;dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={de}f;dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={de}f};dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={de}}f};dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={de}}f;dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={de}} ;dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={ } ;dsn=hello' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn=hello ' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn=hello world ' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ...' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;...' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;=' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_=' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_= ' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_= {}' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_= {}}}' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_= {...................................................................}' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_= {....................................................................................}' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_= {.....................................................................................................}' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_= {...}; FILEDSN=x' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_= {...}; FileDsn = x' 2>&1 +./validate-odbc-connection-string 'abc={ } ; dsn = {hello world} ;_= {...}; Driver=x' 2>&1 +./validate-odbc-connection-string 'abc={}; abc=def' 2>&1 +./validate-odbc-connection-string 'abc={};;' 2>&1 diff --git a/dbms/src/Dictionaries/validateODBCConnectionString.cpp b/dbms/src/Dictionaries/validateODBCConnectionString.cpp new file mode 100644 index 0000000000..1ba648b86d --- /dev/null +++ b/dbms/src/Dictionaries/validateODBCConnectionString.cpp @@ -0,0 +1,224 @@ +#include +#include +#include +#include +#include +#include +#include + + +namespace DB +{ + +namespace ErrorCodes +{ + extern const int BAD_ODBC_CONNECTION_STRING; +} + + +std::string validateODBCConnectionString(const std::string & connection_string) +{ + /// Connection string is the list of name, value pairs. + /// name and value are separated by '='. + /// names are case insensitive. + /// name=value pairs are sepated by ';'. + /// ASCII whitespace characters are skipped before and after delimiters. + /// value may be optionally enclosed by {} + /// in enclosed value, } is escaped as }}. + /// + /// Example: PWD={a}}b} means that password is a}b + /// + /// https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqldriverconnect-function?view=sql-server-2017#comments + + /// unixODBC have fixed size buffers on stack and has buffer overflow bugs. + /// We will limit string sizes to small values. + + static constexpr size_t MAX_ELEMENT_SIZE = 100; + static constexpr size_t MAX_CONNECTION_STRING_SIZE = 1000; + + if (connection_string.empty()) + throw Exception("ODBC connection string cannot be empty", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + const char * pos = connection_string.data(); + const char * end = pos + connection_string.size(); + + auto skip_whitespaces = [&] + { + while (pos < end && isWhitespaceASCII(*pos)) + { + if (*pos != ' ') + throw Exception("ODBC connection string parameter contains unusual whitespace character", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + ++pos; + } + }; + + auto read_name = [&] + { + const char * begin = pos; + + if (pos < end && isValidIdentifierBegin(*pos)) + ++pos; + else + throw Exception("ODBC connection string parameter name doesn't begin with valid identifier character", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + while (pos < end && isWordCharASCII(*pos)) + ++pos; + + return std::string(begin, pos); + }; + + auto read_plain_value = [&] + { + const char * begin = pos; + + while (pos < end && *pos != ';' && !isWhitespaceASCII(*pos)) + { + signed char c = *pos; + if (c < 32 || strchr("[]{}(),;?*=!@'\"", c) != nullptr) + throw Exception("ODBC connection string parameter value is unescaped and contains illegal character", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + ++pos; + } + + return std::string(begin, pos); + }; + + auto read_escaped_value = [&] + { + std::string res; + + if (pos < end && *pos == '{') + ++pos; + else + throw Exception("ODBC connection string parameter value doesn't begin with opening curly brace", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + while (pos < end) + { + if (*pos == '}') + { + ++pos; + if (pos >= end || *pos != '}') + return res; + } + + if (*pos == 0) + throw Exception("ODBC connection string parameter value contains ASCII NUL character", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + res += *pos; + ++pos; + } + + throw Exception("ODBC connection string parameter is escaped but there is no closing curly brace", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + }; + + auto read_value = [&] + { + if (pos >= end) + return std::string{}; + + if (*pos == '{') + return read_escaped_value(); + else + return read_plain_value(); + }; + + std::map parameters; + + while (pos < end) + { + skip_whitespaces(); + std::string name = read_name(); + skip_whitespaces(); + + Poco::toUpperInPlace(name); + if (name == "FILEDSN" || name == "SAVEFILE" || name == "DRIVER") + throw Exception("ODBC connection string has forbidden parameter", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + if (pos >= end) + throw Exception("ODBC connection string parameter doesn't have value", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + if (*pos == '=') + ++pos; + else + throw Exception("ODBC connection string parameter doesn't have value", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + skip_whitespaces(); + std::string value = read_value(); + skip_whitespaces(); + + if (name.size() > MAX_ELEMENT_SIZE || value.size() > MAX_ELEMENT_SIZE) + throw Exception("ODBC connection string has too long keyword or value", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + if (!parameters.emplace(name, value).second) + throw Exception("Duplicate parameter found in ODBC connection string", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + if (pos >= end) + break; + + if (*pos == ';') + ++pos; + else + throw Exception("Unexpected character found after parameter value in ODBC connection string", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + } + + /// Reconstruct the connection string. + + auto it = parameters.find("DSN"); + + if (parameters.end() == it) + throw Exception("DSN parameter is mandatory for ODBC connection string", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + std::string dsn = it->second; + + if (dsn.empty()) + throw Exception("DSN parameter cannot be empty in ODBC connection string", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + parameters.erase(it); + + std::string reconstructed_connection_string; + + auto write_value = [&](const std::string & value) + { + reconstructed_connection_string += '{'; + + const char * pos = value.data(); + const char * end = pos + value.size(); + while (true) + { + const char * next_pos = find_first_symbols<'}'>(pos, end); + + if (next_pos == end) + { + reconstructed_connection_string.append(pos, next_pos - pos); + break; + } + else + { + reconstructed_connection_string.append(pos, next_pos - pos); + reconstructed_connection_string.append("}}"); + pos = next_pos + 1; + } + } + + reconstructed_connection_string += '}'; + }; + + auto write_element = [&](const std::string & name, const std::string & value) + { + reconstructed_connection_string.append(name); + reconstructed_connection_string += '='; + write_value(value); + reconstructed_connection_string += ';'; + }; + + /// Place DSN first because that's more safe. + write_element("DSN", dsn); + for (const auto & elem : parameters) + write_element(elem.first, elem.second); + + if (reconstructed_connection_string.size() >= MAX_CONNECTION_STRING_SIZE) + throw Exception("ODBC connection string is too long", ErrorCodes::BAD_ODBC_CONNECTION_STRING); + + return reconstructed_connection_string; +} + +} diff --git a/dbms/src/Dictionaries/validateODBCConnectionString.h b/dbms/src/Dictionaries/validateODBCConnectionString.h new file mode 100644 index 0000000000..f0f93b1de6 --- /dev/null +++ b/dbms/src/Dictionaries/validateODBCConnectionString.h @@ -0,0 +1,21 @@ +#pragma once + +#include + + +namespace DB +{ + +/** Passing arbitary connection string to ODBC Driver Manager is insecure, for the following reasons: + * 1. Driver Manager like unixODBC has multiple bugs like buffer overflow. + * 2. Driver Manager can interpret some parameters as a path to library for dlopen or a file to read, + * thus allows arbitary remote code execution. + * + * This function will throw exception if connection string has insecure parameters. + * It may also modify connection string to harden it. + * + * Note that it is intended for ANSI (not multibyte) variant of connection string. + */ +std::string validateODBCConnectionString(const std::string & connection_string); + +} diff --git a/dbms/src/Storages/StorageODBC.cpp b/dbms/src/Storages/StorageODBC.cpp index cbda47a924..2361597e04 100644 --- a/dbms/src/Storages/StorageODBC.cpp +++ b/dbms/src/Storages/StorageODBC.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include @@ -29,7 +30,7 @@ StorageODBC::StorageODBC( { pool = createAndCheckResizePocoSessionPool([&] { - return std::make_shared("ODBC", connection_string); + return std::make_shared("ODBC", validateODBCConnectionString(connection_string)); }); } diff --git a/dbms/src/TableFunctions/TableFunctionODBC.cpp b/dbms/src/TableFunctions/TableFunctionODBC.cpp index 75f7314648..7d27415607 100644 --- a/dbms/src/TableFunctions/TableFunctionODBC.cpp +++ b/dbms/src/TableFunctions/TableFunctionODBC.cpp @@ -9,6 +9,7 @@ #include #include #include + #include #include #include -- GitLab