Added validation of ODBC connection string [#CLICKHOUSE-3861]

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;

dbms/src/Dictionaries/CMakeLists.txt

+if (ENABLE_TESTS)
+    add_subdirectory (tests)
+endif ()

dbms/src/Dictionaries/ODBCDictionarySource.cpp

@@ -6,6 +6,7 @@
 #include <Columns/ColumnString.h>
 #include <Dictionaries/ODBCDictionarySource.h>
 #include <Dictionaries/ODBCBlockInputStream.h>
+#include <Dictionaries/validateODBCConnectionString.h>
 #include <Dictionaries/readInvalidateQuery.h>
 #include <Interpreters/Context.h>
 #include <IO/WriteHelpers.h>
@@ -39,7 +40,7 @@ ODBCDictionarySource::ODBCDictionarySource(const DictionaryStructure & dict_stru
     {
         auto session = std::make_shared<Poco::Data::SessionPool>(
             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));

dbms/src/Dictionaries/tests/CMakeLists.txt

+add_executable (validate-odbc-connection-string validate-odbc-connection-string.cpp)
+target_link_libraries (validate-odbc-connection-string dbms)

dbms/src/Dictionaries/tests/validate-odbc-connection-string.cpp

+#include <iostream>
+#include <Common/Exception.h>
+#include <Dictionaries/validateODBCConnectionString.h>
+
+
+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;
+}

dbms/src/Dictionaries/tests/validate-odbc-connection-string.reference

+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

dbms/src/Dictionaries/tests/validate-odbc-connection-string.sh

+#!/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

dbms/src/Dictionaries/validateODBCConnectionString.cpp

+#include <map>
+#include <cstring>
+#include <Poco/String.h>
+#include <common/find_first_symbols.h>
+#include <Common/Exception.h>
+#include <Common/StringUtils/StringUtils.h>
+#include <Dictionaries/validateODBCConnectionString.h>
+
+
+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<std::string, std::string> 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;
+}
+
+}

dbms/src/Dictionaries/validateODBCConnectionString.h

+#pragma once
+
+#include <string>
+
+
+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);
+
+}

dbms/src/Storages/StorageODBC.cpp

@@ -4,6 +4,7 @@
 #include <Storages/StorageFactory.h>
 #include <Interpreters/evaluateConstantExpression.h>
 #include <Dictionaries/ODBCBlockInputStream.h>
+#include <Dictionaries/validateODBCConnectionString.h>
 #include <Parsers/ASTLiteral.h>
 
 
@@ -29,7 +30,7 @@ StorageODBC::StorageODBC(
 {
     pool = createAndCheckResizePocoSessionPool([&]
     {
-        return std::make_shared<Poco::Data::SessionPool>("ODBC", connection_string);
+        return std::make_shared<Poco::Data::SessionPool>("ODBC", validateODBCConnectionString(connection_string));
     });
 }
 

dbms/src/TableFunctions/TableFunctionODBC.cpp

@@ -9,6 +9,7 @@
 #include <Parsers/ASTFunction.h>
 #include <Parsers/ASTLiteral.h>
 #include <Storages/StorageODBC.h>
+
 #include <TableFunctions/ITableFunction.h>
 #include <TableFunctions/TableFunctionFactory.h>
 #include <Common/Exception.h>