From f09d46c8e66eedd2d9f4d3fff2a65c1d74d8fb25 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Thu, 2 May 2019 14:19:44 +0300 Subject: [PATCH 1/9] MXS-2457 Allow string arguments to be treated as fields Before this change, the masking could be bypassed simply by > set @@sql_mode='ANSI_QUOTES'; > select concat("ssn") from person; The reason is that as the query classifier is not aware of whether 'ANSI_QUOTES' is on or not, it will not know that what above appears to be the string "ssn", actually is the field name `ssn`. Consequently, the select will not be blocked and the result returned in cleartext. It's now possible to instruct the query classifier to report all string arguments of functions as fields, which will prevent the above. However, it will also mean that there may be false positives. --- include/maxscale/query_classifier.h | 42 +++++++++++++++++++ .../qc_mysqlembedded/qc_mysqlembedded.cc | 38 +++++++++++++++++ query_classifier/qc_sqlite/qc_sqlite.cc | 33 +++++++++++++++ server/core/query_classifier.cc | 38 ++++++++++++++--- 4 files changed, 146 insertions(+), 5 deletions(-) diff --git a/include/maxscale/query_classifier.h b/include/maxscale/query_classifier.h index ac7b0f984..d05c97bbe 100644 --- a/include/maxscale/query_classifier.h +++ b/include/maxscale/query_classifier.h @@ -30,6 +30,16 @@ typedef enum qc_init_kind QC_INIT_BOTH = 0x03 } qc_init_kind_t; +/** + * qc_option_t defines options that affect the classification. + */ +enum qc_option_t +{ + QC_OPTION_STRING_ARG_AS_FIELD = (1 << 0), /*< Report a string argument to a function as a field. */ +}; + +const uint32_t QC_OPTION_MASK = QC_OPTION_STRING_ARG_AS_FIELD; + /** * qc_sql_mode_t specifies what should be assumed of the statements * that will be parsed. @@ -441,6 +451,22 @@ typedef struct query_classifier * @param info The info to be closed. */ void (* qc_info_close)(QC_STMT_INFO* info); + + /** + * Gets the options of the *calling* thread. + * + * @return Bit mask of values from qc_option_t. + */ + uint32_t (* qc_get_options)(); + + /** + * Sets the options for the *calling* thread. + * + * @param options Bits from qc_option_t. + * + * @return QC_RESULT_OK if @c options is valid, otherwise QC_RESULT_ERROR. + */ + int32_t (* qc_set_options)(uint32_t options); } QUERY_CLASSIFIER; /** @@ -952,4 +978,20 @@ json_t* qc_get_cache_stats_as_json(); */ const char* qc_result_to_string(qc_parse_result_t result); +/** + * Gets the options of the *calling* thread. + * + * @return Bit mask of values from qc_option_t. + */ +uint32_t qc_get_options(); + +/** + * Sets the options for the *calling* thread. + * + * @param options Bits from qc_option_t. + * + * @return true if the options were valid, false otherwise. + */ +bool qc_set_options(uint32_t options); + MXS_END_DECLS diff --git a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc index ceb581800..ad2c1e2fd 100644 --- a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc +++ b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc @@ -195,6 +195,7 @@ static struct static thread_local struct { qc_sql_mode_t sql_mode; + uint32_t options; NAME_MAPPING* function_name_mappings; // The version information is not used; the embedded library parses according // to the version of the embedded library it has been linked with. However, we @@ -203,6 +204,7 @@ static thread_local struct } this_thread = { QC_SQL_MODE_DEFAULT, + 0, function_name_mappings_default, 0 }; @@ -2475,6 +2477,19 @@ static void add_function_field_usage(st_select_lex* select, add_function_field_usage(select, static_cast(item), fi); break; + case Item::STRING_ITEM: + if (this_thread.options & QC_OPTION_STRING_ARG_AS_FIELD) + { + String* s = item->val_str(); + int len = s->length(); + char tmp[len + 1]; + memcpy(tmp, s->ptr(), len); + tmp[len] = 0; + + add_function_field_usage(nullptr, nullptr, tmp, fi); + } + break; + default: // mxb_assert(!true); ; @@ -3526,6 +3541,27 @@ int32_t qc_mysql_set_sql_mode(qc_sql_mode_t sql_mode) return rv; } +uint32_t qc_mysql_get_options() +{ + return this_thread.options; +} + +int32_t qc_mysql_set_options(uint32_t options) +{ + int32_t rv = QC_RESULT_OK; + + if ((options & ~QC_OPTION_MASK) == 0) + { + this_thread.options = options; + } + else + { + rv = QC_RESULT_ERROR; + } + + return rv; +} + /** * EXPORTS */ @@ -3561,6 +3597,8 @@ extern "C" qc_mysql_set_sql_mode, nullptr, // qc_info_dup not supported. nullptr, // qc_info_close not supported. + qc_mysql_get_options, + qc_mysql_set_options }; static MXS_MODULE info = diff --git a/query_classifier/qc_sqlite/qc_sqlite.cc b/query_classifier/qc_sqlite/qc_sqlite.cc index 0ae6e2023..f0f2e6323 100644 --- a/query_classifier/qc_sqlite/qc_sqlite.cc +++ b/query_classifier/qc_sqlite/qc_sqlite.cc @@ -156,6 +156,7 @@ static thread_local struct bool initialized; // Whether the thread specific data has been initialized. sqlite3* pDb; // Thread specific database handle. qc_sql_mode_t sql_mode; // What sql_mode is used. + uint32_t options; // Options affecting classification. QcSqliteInfo* pInfo; // The information for the current statement being classified. uint64_t version; // Encoded version number uint32_t version_major; @@ -1141,6 +1142,13 @@ public: } } } + else if (pExpr->op == TK_STRING) + { + if (this_thread.options & QC_OPTION_STRING_ARG_AS_FIELD) + { + zColumn = pExpr->u.zToken; + } + } if (zColumn) { @@ -4541,6 +4549,8 @@ static int32_t qc_sqlite_get_sql_mode(qc_sql_mode_t* sql_mode); static int32_t qc_sqlite_set_sql_mode(qc_sql_mode_t sql_mode); static QC_STMT_INFO* qc_sqlite_info_dup(QC_STMT_INFO* info); static void qc_sqlite_info_close(QC_STMT_INFO* info); +static uint32_t qc_sqlite_get_options(); +static int32_t qc_sqlite_set_options(uint32_t options); static bool get_key_and_value(char* arg, const char** pkey, const char** pvalue) { @@ -5223,6 +5233,27 @@ void qc_sqlite_info_close(QC_STMT_INFO* info) static_cast(info)->dec_ref(); } +uint32_t qc_sqlite_get_options() +{ + return this_thread.options; +} + +int32_t qc_sqlite_set_options(uint32_t options) +{ + int32_t rv = QC_RESULT_OK; + + if ((options & ~QC_OPTION_MASK) == 0) + { + this_thread.options = options; + } + else + { + rv = QC_RESULT_ERROR; + } + + return rv; +} + /** * EXPORTS */ @@ -5258,6 +5289,8 @@ extern "C" qc_sqlite_set_sql_mode, qc_sqlite_info_dup, qc_sqlite_info_close, + qc_sqlite_get_options, + qc_sqlite_set_options }; static MXS_MODULE info = diff --git a/server/core/query_classifier.cc b/server/core/query_classifier.cc index 9208d31a9..cfb195591 100644 --- a/server/core/query_classifier.cc +++ b/server/core/query_classifier.cc @@ -96,9 +96,11 @@ class QCInfoCache; static thread_local struct { QCInfoCache* pInfo_cache; + uint32_t options; } this_thread = { - nullptr + nullptr, + 0 }; @@ -147,7 +149,8 @@ public: { const Entry& entry = i->second; - if (entry.sql_mode == this_unit.qc_sql_mode) + if ((entry.sql_mode == this_unit.qc_sql_mode) && + (entry.options == this_thread.options)) { mxb_assert(this_unit.classifier); this_unit.classifier->qc_info_dup(entry.pInfo); @@ -157,7 +160,7 @@ public: } else { - // If the sql_mode has changed, we discard the existing result. + // If the sql_mode or options has changed, we discard the existing result. erase(i); ++m_stats.misses; @@ -197,7 +200,7 @@ public: { this_unit.classifier->qc_info_dup(pInfo); - m_infos.emplace(canonical_stmt, Entry(pInfo, this_unit.qc_sql_mode)); + m_infos.emplace(canonical_stmt, Entry(pInfo, this_unit.qc_sql_mode, this_thread.options)); ++m_stats.inserts; m_stats.size += size; @@ -213,14 +216,16 @@ public: private: struct Entry { - Entry(QC_STMT_INFO* pInfo, qc_sql_mode_t sql_mode) + Entry(QC_STMT_INFO* pInfo, qc_sql_mode_t sql_mode, uint32_t options) : pInfo(pInfo) , sql_mode(sql_mode) + , options(options) { } QC_STMT_INFO* pInfo; qc_sql_mode_t sql_mode; + uint32_t options; }; typedef std::unordered_map InfosByStmt; @@ -1282,6 +1287,29 @@ void qc_set_sql_mode(qc_sql_mode_t sql_mode) } } +uint32_t qc_get_options() +{ + QC_TRACE(); + mxb_assert(this_unit.classifier); + + return this_unit.classifier->qc_get_options(); +} + +bool qc_set_options(uint32_t options) +{ + QC_TRACE(); + mxb_assert(this_unit.classifier); + + int32_t rv = this_unit.classifier->qc_set_options(options); + + if (rv == QC_RESULT_OK) + { + this_thread.options = options; + } + + return rv == QC_RESULT_OK; +} + void qc_get_cache_properties(QC_CACHE_PROPERTIES* properties) { properties->max_size = this_unit.cache_max_size(); From 3a5a8b13b9d6802d4c68b4705dfabeb88597f8bc Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Thu, 2 May 2019 14:16:30 +0300 Subject: [PATCH 2/9] MXS-2457 Treat string args as fields The masking filter will now consider all string arguments to functions to be fields. This in order to prevent bypassing of the masking with > set @@sql_mode='ANSI_QUOTES'; > select concat("ssn") from masking; This may lead to false positives, but no can do. --- Documentation/Filters/Masking.md | 31 +++++++++++++ .../modules/filter/masking/maskingfilter.cc | 6 +++ .../filter/masking/maskingfilterconfig.cc | 29 ++++++++---- .../filter/masking/maskingfilterconfig.hh | 16 +++++++ .../filter/masking/maskingfiltersession.cc | 46 +++++++++++++++++++ 5 files changed, 119 insertions(+), 9 deletions(-) diff --git a/Documentation/Filters/Masking.md b/Documentation/Filters/Masking.md index 05a763015..dbeb2dbaf 100644 --- a/Documentation/Filters/Masking.md +++ b/Documentation/Filters/Masking.md @@ -96,6 +96,26 @@ Please see the configuration parameter [require_fully_parsed](#require_fully_parsed) for how to change the default behaviour. +From MaxScale 2.3.7 onwards, the masking filter will treat any strings +passed to functions as if they were fields. The reason is that as the +MaxScale query classifier is not aware of whether `ANSI_QUOTES` is +enabled or not, it is possible to bypass the masking by turning that +option on. +``` +mysql> set @@sql_mode = 'ANSI_QUOTES'; +mysql> select concat("ssn") from managers; +``` +Before this change, the content of the field `ssn` would have been +returned in clear text even if the column should have been masked. + +Note that this change will mean that there may be false positives +if `ANSI_QUOTES` is not enabled and a string argument happens to +be the same as the name of a field to be masked. + +Please see the configuration parameter +[treat_string_arg_as_field(#treat_string_arg_as_field) +for how to change the default behaviour. + ## Limitations The masking filter can _only_ be used for masking columns of the following @@ -215,6 +235,17 @@ Note that if this parameter is set to false, then `prevent_function_usage`, less effective, as it with a statement that can not be fully parsed may be possible to bypass the protection that they are intended to provide. +#### `treat_string_arg_as_field` + +This optional parameter specifies how the masking filter should treat +strings used as arguments to functions. If true, they will be handled +as fields, which will cause fields to be masked even if `ANSI_QUOTES` has +been enabled and `"` is used instead of backtick. +``` +treat_string_arg_as_field=false +``` +The default value is `true`. + #### `check_user_variables` This optional parameter specifies how the masking filter should diff --git a/server/modules/filter/masking/maskingfilter.cc b/server/modules/filter/masking/maskingfilter.cc index e33d55ebc..54dda8a8c 100644 --- a/server/modules/filter/masking/maskingfilter.cc +++ b/server/modules/filter/masking/maskingfilter.cc @@ -145,6 +145,12 @@ extern "C" MXS_MODULE* MXS_CREATE_MODULE() Config::require_fully_parsed_default, MXS_MODULE_OPT_NONE, }, + { + Config::treat_string_arg_as_field_name, + MXS_MODULE_PARAM_BOOL, + Config::treat_string_arg_as_field_default, + MXS_MODULE_OPT_NONE + }, {MXS_END_MODULE_PARAMS} } }; diff --git a/server/modules/filter/masking/maskingfilterconfig.cc b/server/modules/filter/masking/maskingfilterconfig.cc index eab0ed540..90cc813cb 100644 --- a/server/modules/filter/masking/maskingfilterconfig.cc +++ b/server/modules/filter/masking/maskingfilterconfig.cc @@ -17,14 +17,15 @@ namespace { -const char config_name_check_subqueries[] = "check_subqueries"; -const char config_name_check_unions[] = "check_unions"; -const char config_name_check_user_variables[] = "check_user_variables"; -const char config_name_large_payload[] = "large_payload"; -const char config_name_prevent_function_usage[] = "prevent_function_usage"; -const char config_name_require_fully_parsed[] = "require_fully_parsed"; -const char config_name_rules[] = "rules"; -const char config_name_warn_type_mismatch[] = "warn_type_mismatch"; +const char config_name_check_subqueries[] = "check_subqueries"; +const char config_name_check_unions[] = "check_unions"; +const char config_name_check_user_variables[] = "check_user_variables"; +const char config_name_large_payload[] = "large_payload"; +const char config_name_prevent_function_usage[] = "prevent_function_usage"; +const char config_name_require_fully_parsed[] = "require_fully_parsed"; +const char config_name_rules[] = "rules"; +const char config_name_warn_type_mismatch[] = "warn_type_mismatch"; +const char config_name_treat_string_arg_as_field[] = "treat_string_arg_as_field"; const char config_value_abort[] = "abort"; @@ -105,7 +106,11 @@ const char* MaskingFilterConfig::check_subqueries_default = config_value_true; const char* MaskingFilterConfig::require_fully_parsed_name = config_name_require_fully_parsed; const char* MaskingFilterConfig::require_fully_parsed_default = config_name_require_fully_parsed; - +/* + * PARAM treat_string_arg_as_field + */ +const char* MaskingFilterConfig::treat_string_arg_as_field_name = config_name_treat_string_arg_as_field; +const char* MaskingFilterConfig::treat_string_arg_as_field_default = config_value_true; /* * MaskingFilterConfig */ @@ -161,3 +166,9 @@ bool MaskingFilterConfig::get_require_fully_parsed(const MXS_CONFIG_PARAMETER* p { return config_get_bool(pParams, require_fully_parsed_name); } + +// static +bool MaskingFilterConfig::get_treat_string_arg_as_field(const MXS_CONFIG_PARAMETER* pParams) +{ + return config_get_bool(pParams, treat_string_arg_as_field_name); +} diff --git a/server/modules/filter/masking/maskingfilterconfig.hh b/server/modules/filter/masking/maskingfilterconfig.hh index 9b4cb6f37..7f831e4a6 100644 --- a/server/modules/filter/masking/maskingfilterconfig.hh +++ b/server/modules/filter/masking/maskingfilterconfig.hh @@ -57,6 +57,9 @@ public: static const char* require_fully_parsed_name; static const char* require_fully_parsed_default; + static const char* treat_string_arg_as_field_name; + static const char* treat_string_arg_as_field_default; + MaskingFilterConfig(const char* zName, const MXS_CONFIG_PARAMETER* pParams) : m_name(zName) , m_large_payload(get_large_payload(pParams)) @@ -67,6 +70,7 @@ public: , m_check_unions(get_check_unions(pParams)) , m_check_subqueries(get_check_subqueries(pParams)) , m_require_fully_parsed(get_require_fully_parsed(pParams)) + , m_treat_string_arg_as_field(get_treat_string_arg_as_field(pParams)) { } @@ -119,6 +123,11 @@ public: return m_require_fully_parsed; } + bool treat_string_arg_as_field() const + { + return m_treat_string_arg_as_field; + } + void set_large_payload(large_payload_t l) { m_large_payload = l; @@ -158,6 +167,11 @@ public: m_require_fully_parsed = b; } + void set_treat_string_arg_as_field(bool b) + { + m_treat_string_arg_as_field = b; + } + bool is_parsing_needed() const { return prevent_function_usage() || check_user_variables() || check_unions() || check_subqueries(); @@ -171,6 +185,7 @@ public: static bool get_check_unions(const MXS_CONFIG_PARAMETER* pParams); static bool get_check_subqueries(const MXS_CONFIG_PARAMETER* pParams); static bool get_require_fully_parsed(const MXS_CONFIG_PARAMETER* pParams); + static bool get_treat_string_arg_as_field(const MXS_CONFIG_PARAMETER* pParams); private: std::string m_name; @@ -182,4 +197,5 @@ private: bool m_check_unions; bool m_check_subqueries; bool m_require_fully_parsed; + bool m_treat_string_arg_as_field; }; diff --git a/server/modules/filter/masking/maskingfiltersession.cc b/server/modules/filter/masking/maskingfiltersession.cc index b9614f046..a2276e1a7 100644 --- a/server/modules/filter/masking/maskingfiltersession.cc +++ b/server/modules/filter/masking/maskingfiltersession.cc @@ -48,6 +48,46 @@ GWBUF* create_parse_error_response() return create_error_response(zMessage); } +class EnableOption +{ +public: + EnableOption(const EnableOption&) = delete; + EnableOption& operator=(const EnableOption&) = delete; + + EnableOption(uint32_t option) + : m_option(option) + , m_options(0) + , m_disable(false) + { + if (m_option) + { + m_options = qc_get_options(); + + if (!(m_options & m_option)) + { + uint32_t options = (m_options | m_option); + MXB_AT_DEBUG(bool rv = )qc_set_options(options); + mxb_assert(rv); + m_disable = true; + } + } + } + + ~EnableOption() + { + if (m_disable) + { + MXB_AT_DEBUG(bool rv = )qc_set_options(m_options); + mxb_assert(rv); + } + } + +private: + uint32_t m_option; + uint32_t m_options; + bool m_disable; +}; + } MaskingFilterSession::MaskingFilterSession(MXS_SESSION* pSession, const MaskingFilter* pFilter) @@ -127,6 +167,9 @@ bool MaskingFilterSession::check_textual_query(GWBUF* pPacket) { bool rv = false; + uint32_t option = m_filter.config().treat_string_arg_as_field() ? QC_OPTION_STRING_ARG_AS_FIELD : 0; + EnableOption enable(option); + if (qc_parse(pPacket, QC_COLLECT_FIELDS | QC_COLLECT_FUNCTIONS) == QC_QUERY_PARSED || !m_filter.config().require_fully_parsed()) { @@ -166,6 +209,9 @@ bool MaskingFilterSession::check_binary_query(GWBUF* pPacket) { bool rv = false; + uint32_t option = m_filter.config().treat_string_arg_as_field() ? QC_OPTION_STRING_ARG_AS_FIELD : 0; + EnableOption enable(option); + if (qc_parse(pPacket, QC_COLLECT_FIELDS | QC_COLLECT_FUNCTIONS) == QC_QUERY_PARSED || !m_filter.config().require_fully_parsed()) { From 3fa1f0773eb2996fc3d707951891c1e888b6610f Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Thu, 2 May 2019 15:13:11 +0300 Subject: [PATCH 3/9] MXS-2457 Update test program --- ...axscale.cnf.template.masking_auto_firewall | 1 + .../masking_auto_firewall.cpp | 36 +++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/maxscale-system-test/cnf/maxscale.cnf.template.masking_auto_firewall b/maxscale-system-test/cnf/maxscale.cnf.template.masking_auto_firewall index 081d14cfe..7943dc83c 100644 --- a/maxscale-system-test/cnf/maxscale.cnf.template.masking_auto_firewall +++ b/maxscale-system-test/cnf/maxscale.cnf.template.masking_auto_firewall @@ -23,6 +23,7 @@ module=masking rules=/home/vagrant/masking_auto_firewall.json warn_type_mismatch=always large_payload=ignore +treat_string_arg_as_field=false [RWS] type=service diff --git a/maxscale-system-test/masking_auto_firewall.cpp b/maxscale-system-test/masking_auto_firewall.cpp index fd7e7a64d..2afe82242 100644 --- a/maxscale-system-test/masking_auto_firewall.cpp +++ b/maxscale-system-test/masking_auto_firewall.cpp @@ -74,8 +74,6 @@ void test_one_ps(TestConnections& test, const char* zQuery, Expect expect) void run(TestConnections& test) { - init(test); - MYSQL* pMysql = test.maxscales->conn_rwsplit[0]; int rv; @@ -130,6 +128,38 @@ void run(TestConnections& test) test_one(test, "select * FROM (select * from masking_auto_firewall) tbl", Expect::FAILURE); } +void run_ansi_quotes(TestConnections& test) +{ + // This SHOULD go through as we have 'treat_string_arg_as_field=false" + test_one(test, "select concat(\"a\") from masking_auto_firewall", Expect::SUCCESS); + + Connection c = test.maxscales->rwsplit(); + c.connect(); + + test.expect(c.query("SET @@SQL_MODE = CONCAT(@@SQL_MODE, ',ANSI_QUOTES')"), + "Could not turn on 'ANSI_QUOTES'"); + + // This SHOULD still go through as we still have 'treat_string_arg_as_field=false" + test_one(test, "select concat(\"a\") from masking_auto_firewall", Expect::SUCCESS); + + // Let's turn on 'treat_string_arg_as_field=true' + test.maxscales->ssh_node(0, + "sed -i -e " + "'s/treat_string_arg_as_field=false/treat_string_arg_as_field=true/' " + "/etc/maxscale.cnf", + true); + // and restart MaxScale + test.maxscales->restart(); + + // This should NOT go through as we have 'treat_string_arg_as_field=true" and ANSI_QUOTES. + test_one(test, "select concat(\"a\") from masking_auto_firewall", Expect::FAILURE); + + // Have to reconnect as we restarted MaxScale. + c.connect(); + test.expect(c.query("SET @@SQL_MODE = REPLACE(@@SQL_MODE, 'ANSI_QUOTES', '')"), + "Could not turn off 'ANSI_QUOTES'"); +} + } int main(int argc, char* argv[]) @@ -151,7 +181,9 @@ int main(int argc, char* argv[]) if (test.maxscales->connect_rwsplit() == 0) { + init(test); run(test); + run_ansi_quotes(test); } else { From fe5160a714e39234ac47ec3b458a1602d71228d3 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Thu, 2 May 2019 19:34:50 +0300 Subject: [PATCH 4/9] MXS-2457 Add 'treat_string_arg_as_field' to DB Firewall --- .../Filters/Database-Firewall-Filter.md | 10 ++++ .../modules/filter/dbfwfilter/dbfwfilter.cc | 50 +++++++++++++++++++ .../modules/filter/dbfwfilter/dbfwfilter.hh | 11 ++++ .../filter/masking/maskingfiltersession.cc | 1 + 4 files changed, 72 insertions(+) diff --git a/Documentation/Filters/Database-Firewall-Filter.md b/Documentation/Filters/Database-Firewall-Filter.md index d16add626..34ca1ade8 100644 --- a/Documentation/Filters/Database-Firewall-Filter.md +++ b/Documentation/Filters/Database-Firewall-Filter.md @@ -146,6 +146,16 @@ query itself is logged. The log messages are logged at the notice level. Log all queries that do not match a rule. The matched user and the query is logged. The log messages are logged at the notice level. +#### `treat_string_arg_as_field` +This optional parameter specifies how the database firewall should treat +strings used as arguments to functions. If true, they will be handled +as fields, which will cause fields to be masked even if `ANSI_QUOTES` has +been enabled and `"` is used instead of backtick. +``` +treat_string_arg_as_field=false +``` +The default value is `true`. + ## Rule syntax The rules are defined by using the following syntax: diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.cc b/server/modules/filter/dbfwfilter/dbfwfilter.cc index 908fe9e80..ab3911e93 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.cc +++ b/server/modules/filter/dbfwfilter/dbfwfilter.cc @@ -129,6 +129,48 @@ private: }; thread_local DbfwThread* this_thread = NULL; + +// TODO: In 2.4 move to query_classifier.hh. +class EnableOption +{ +public: + EnableOption(const EnableOption&) = delete; + EnableOption& operator=(const EnableOption&) = delete; + + EnableOption(uint32_t option) + : m_option(option) + , m_options(0) + , m_disable(false) + { + if (m_option) + { + m_options = qc_get_options(); + + if (!(m_options & m_option)) + { + uint32_t options = (m_options | m_option); + MXB_AT_DEBUG(bool rv = )qc_set_options(options); + mxb_assert(rv); + m_disable = true; + } + } + } + + ~EnableOption() + { + if (m_disable) + { + MXB_AT_DEBUG(bool rv = )qc_set_options(m_options); + mxb_assert(rv); + } + } + +private: + uint32_t m_option; + uint32_t m_options; + bool m_disable; +}; + } bool parse_at_times(const char** tok, char** saveptr, Rule* ruledef); @@ -585,6 +627,11 @@ MXS_MODULE* MXS_CREATE_MODULE() MXS_MODULE_OPT_ENUM_UNIQUE, action_values }, + { + "treat_string_arg_as_field", + MXS_MODULE_PARAM_BOOL, + "true" + }, {MXS_END_MODULE_PARAMS} } }; @@ -1200,6 +1247,7 @@ int global_version = 1; Dbfw::Dbfw(MXS_CONFIG_PARAMETER* params) : m_action((enum fw_actions)config_get_enum(params, "action", action_values)) , m_log_match(0) + , m_treat_string_arg_as_field(config_get_bool(params, "treat_string_arg_as_field")) , m_filename(config_get_string(params, "rules")) , m_version(atomic_add(&global_version, 1)) { @@ -1469,6 +1517,8 @@ int DbfwSession::routeQuery(GWBUF* buffer) } else { + uint32_t option = m_instance->treat_string_arg_as_field() ? QC_OPTION_STRING_ARG_AS_FIELD : 0; + EnableOption enable(option); GWBUF* analyzed_queue = buffer; // QUERY_TYPE_PREPARE_STMT need not be handled separately as the diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.hh b/server/modules/filter/dbfwfilter/dbfwfilter.hh index 7c69a25ff..3ff2b8d63 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.hh +++ b/server/modules/filter/dbfwfilter/dbfwfilter.hh @@ -224,6 +224,16 @@ public: */ fw_actions get_action() const; + /** + * Should string args be treated as fields? + * + * @return True, if they should, false otherwise. + */ + bool treat_string_arg_as_field() const + { + return m_treat_string_arg_as_field; + } + /** * Get logging option bitmask * @@ -267,6 +277,7 @@ public: private: fw_actions m_action; /*< Default operation mode, defaults to deny */ int m_log_match; /*< Log matching and/or non-matching queries */ + bool m_treat_string_arg_as_field; mutable std::mutex m_lock; /*< Instance spinlock */ std::string m_filename; /*< Path to the rule file */ int m_version; /*< Latest rule file version, incremented on reload */ diff --git a/server/modules/filter/masking/maskingfiltersession.cc b/server/modules/filter/masking/maskingfiltersession.cc index a2276e1a7..397ba850e 100644 --- a/server/modules/filter/masking/maskingfiltersession.cc +++ b/server/modules/filter/masking/maskingfiltersession.cc @@ -48,6 +48,7 @@ GWBUF* create_parse_error_response() return create_error_response(zMessage); } +// TODO: In 2.4 move to query_classifier.hh. class EnableOption { public: From 4aa8eac799368632152c3c711935239d444214eb Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 3 May 2019 08:58:56 +0300 Subject: [PATCH 5/9] MXS-2457 Allow strings to be treated as fields Before this change, if the firewall was configured to block the use of certain columns, it could be be bypassed simply by > set @@sql_mode='ANSI_QUOTES'; > select "ssn" from person; The reason is that as the query classifier is not aware of whether 'ANSI_QUOTES' is on or not, it will not know that what above appears to be the string "ssn", actually is the field name `ssn`. Consequently, the select will not be blocked and the result returned in cleartext. It's now possible to instruct the query classifier to report all strings as fields, which will prevent the above. However, it will also mean that there may be false positives. --- include/maxscale/query_classifier.h | 3 +- .../qc_mysqlembedded/qc_mysqlembedded.cc | 30 +++++++++++++++++-- query_classifier/qc_sqlite/qc_sqlite.cc | 19 ++++++++++++ 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/include/maxscale/query_classifier.h b/include/maxscale/query_classifier.h index d05c97bbe..6ce242303 100644 --- a/include/maxscale/query_classifier.h +++ b/include/maxscale/query_classifier.h @@ -36,9 +36,10 @@ typedef enum qc_init_kind enum qc_option_t { QC_OPTION_STRING_ARG_AS_FIELD = (1 << 0), /*< Report a string argument to a function as a field. */ + QC_OPTION_STRING_AS_FIELD = (1 << 1), /*< Report strings as fields. */ }; -const uint32_t QC_OPTION_MASK = QC_OPTION_STRING_ARG_AS_FIELD; +const uint32_t QC_OPTION_MASK = QC_OPTION_STRING_ARG_AS_FIELD | QC_OPTION_STRING_AS_FIELD; /** * qc_sql_mode_t specifies what should be assumed of the statements diff --git a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc index ad2c1e2fd..7689c99d1 100644 --- a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc +++ b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc @@ -2268,7 +2268,6 @@ static void unalias_names(st_select_lex* select, } static void add_field_info(parsing_info_t* info, - st_select_lex* select, const char* database, const char* table, const char* column, @@ -2276,8 +2275,6 @@ static void add_field_info(parsing_info_t* info, { mxb_assert(column); - unalias_names(select, database, table, &database, &table); - QC_FIELD_INFO item = {(char*)database, (char*)table, (char*)column}; size_t i; @@ -2353,6 +2350,20 @@ static void add_field_info(parsing_info_t* info, } } +static void add_field_info(parsing_info_t* info, + st_select_lex* select, + const char* database, + const char* table, + const char* column, + List* excludep) +{ + mxb_assert(column); + + unalias_names(select, database, table, &database, &table); + + add_field_info(info, database, table, column, excludep); +} + static void add_function_field_usage(const char* database, const char* table, const char* column, @@ -3063,6 +3074,19 @@ static void update_field_infos(parsing_info_t* pi, } break; + case Item::STRING_ITEM: + if (this_thread.options & QC_OPTION_STRING_AS_FIELD) + { + String* s = item->val_str(); + int len = s->length(); + char tmp[len + 1]; + memcpy(tmp, s->ptr(), len); + tmp[len] = 0; + + add_field_info(pi, nullptr, nullptr, tmp, excludep); + } + break; + default: break; } diff --git a/query_classifier/qc_sqlite/qc_sqlite.cc b/query_classifier/qc_sqlite/qc_sqlite.cc index f0f2e6323..ca8545eff 100644 --- a/query_classifier/qc_sqlite/qc_sqlite.cc +++ b/query_classifier/qc_sqlite/qc_sqlite.cc @@ -832,6 +832,14 @@ public: update_field_infos_from_expr(pAliases, context, pExpr, pExclude); break; + case TK_STRING: // select "a" ..., for @@sql_mode containing 'ANSI_QUOTES' + if (this_thread.options & QC_OPTION_STRING_AS_FIELD) + { + const char* zColumn = pExpr->u.zToken; + update_field_infos_from_column(pAliases, context, zColumn, pExclude); + } + break; + case TK_VARIABLE: { if (zToken[0] == '@') @@ -1184,6 +1192,17 @@ public: } } + void update_field_infos_from_column(QcAliases* pAliases, + uint32_t context, + const char* zColumn, + const ExprList* pExclude) + { + if (must_check_sequence_related_functions() || must_collect_fields()) + { + update_field_info(pAliases, context, nullptr, nullptr, zColumn, pExclude); + } + } + void update_field_infos_from_exprlist(QcAliases* pAliases, uint32_t context, const ExprList* pEList, From 5833c39a8a55c67f96414a0cbed6d6e619fc64ea Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 3 May 2019 10:03:01 +0300 Subject: [PATCH 6/9] MXS-2457 Disable query classifier cache in masking As the canonicalization is also not aware of 'ANSI_QUOTES', the cache must be disabled if the masking filter has 'treat_string_arg_as_field' enabled. --- .../modules/filter/masking/maskingfilter.cc | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/server/modules/filter/masking/maskingfilter.cc b/server/modules/filter/masking/maskingfilter.cc index 54dda8a8c..cd33cc8ae 100644 --- a/server/modules/filter/masking/maskingfilter.cc +++ b/server/modules/filter/masking/maskingfilter.cc @@ -185,6 +185,28 @@ MaskingFilter* MaskingFilter::create(const char* zName, MXS_CONFIG_PARAMETER* pP if (sRules.get()) { pFilter = new MaskingFilter(config, sRules); + + if (config.treat_string_arg_as_field()) + { + MXS_NOTICE("The parameter 'treat_string_arg_as_field' is enabled for %s. " + "As a consequence, the query classifier cache must be disabled.", + zName); + + QC_CACHE_PROPERTIES cache_properties; + qc_get_cache_properties(&cache_properties); + + if (cache_properties.max_size != 0) + { + cache_properties.max_size = 0; + qc_set_cache_properties(&cache_properties); + + MXS_NOTICE("Query classifier cache disabled."); + } + else + { + MXS_NOTICE("The query classifier cache was disabled already."); + } + } } return pFilter; From 279edce16ed9eb712d352323906ab1382004990b Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 3 May 2019 10:06:53 +0300 Subject: [PATCH 7/9] MXS-2457 Add treat_string_as_field to firewall Necessary if the firewall should be able to block columns when 'ANSI_QUOTES' as enabled and " instead of backticks are used. Without this, the following > set @@sql_mode='ANSI_QUOTES'; > select "ssn" from person; will not be blocked if the database firewall has been configured to block the column ssn. --- .../Filters/Database-Firewall-Filter.md | 20 ++++++++- .../modules/filter/dbfwfilter/dbfwfilter.cc | 45 ++++++++++++++++++- .../modules/filter/dbfwfilter/dbfwfilter.hh | 11 +++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/Documentation/Filters/Database-Firewall-Filter.md b/Documentation/Filters/Database-Firewall-Filter.md index 34ca1ade8..9d575f35f 100644 --- a/Documentation/Filters/Database-Firewall-Filter.md +++ b/Documentation/Filters/Database-Firewall-Filter.md @@ -146,16 +146,32 @@ query itself is logged. The log messages are logged at the notice level. Log all queries that do not match a rule. The matched user and the query is logged. The log messages are logged at the notice level. +#### `treat_string_as_field` +This optional parameter specifies how the database firewall should treat +strings. If true, they will be handled as fields, which will cause column +blocking rules to match even if `ANSI_QUOTES` has been enabled and `"` is +used instead of backtick. +``` +treat_string_as_field=false +``` +The default value is `true`. + +Note that this may cause a false positive, if a "true" string contains the +name of a column to be blocked. + #### `treat_string_arg_as_field` This optional parameter specifies how the database firewall should treat strings used as arguments to functions. If true, they will be handled -as fields, which will cause fields to be masked even if `ANSI_QUOTES` has -been enabled and `"` is used instead of backtick. +as fields, which will cause function column blocking rules to match even +even if `ANSI_QUOTES` has been enabled and `"` is used instead of backtick. ``` treat_string_arg_as_field=false ``` The default value is `true`. +Note that this may cause a false positive, if a "true" string contains the +name of a column to be blocked. + ## Rule syntax The rules are defined by using the following syntax: diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.cc b/server/modules/filter/dbfwfilter/dbfwfilter.cc index ab3911e93..a4044f2ca 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.cc +++ b/server/modules/filter/dbfwfilter/dbfwfilter.cc @@ -632,6 +632,11 @@ MXS_MODULE* MXS_CREATE_MODULE() MXS_MODULE_PARAM_BOOL, "true" }, + { + "treat_string_as_field", + MXS_MODULE_PARAM_BOOL, + "true" + }, {MXS_END_MODULE_PARAMS} } }; @@ -1247,6 +1252,7 @@ int global_version = 1; Dbfw::Dbfw(MXS_CONFIG_PARAMETER* params) : m_action((enum fw_actions)config_get_enum(params, "action", action_values)) , m_log_match(0) + , m_treat_string_as_field(config_get_bool(params, "treat_string_as_field")) , m_treat_string_arg_as_field(config_get_bool(params, "treat_string_arg_as_field")) , m_filename(config_get_string(params, "rules")) , m_version(atomic_add(&global_version, 1)) @@ -1276,6 +1282,32 @@ Dbfw* Dbfw::create(const char* zName, MXS_CONFIG_PARAMETER* pParams) if (process_rule_file(file, &rules, &users)) { rval = new(std::nothrow) Dbfw(pParams); + + if (rval) + { + if (rval->treat_string_as_field() || rval->treat_string_arg_as_field()) + { + MXS_NOTICE("The parameter 'treat_string_arg_as_field' and/or " + "'treat_string_as_field' is enabled for %s. " + "As a consequence, the query classifier cache must be disabled.", + zName); + + QC_CACHE_PROPERTIES cache_properties; + qc_get_cache_properties(&cache_properties); + + if (cache_properties.max_size != 0) + { + cache_properties.max_size = 0; + qc_set_cache_properties(&cache_properties); + + MXS_NOTICE("Query classifier cache disabled."); + } + else + { + MXS_NOTICE("The query classifier cache was disabled already."); + } + } + } } return rval; @@ -1517,7 +1549,18 @@ int DbfwSession::routeQuery(GWBUF* buffer) } else { - uint32_t option = m_instance->treat_string_arg_as_field() ? QC_OPTION_STRING_ARG_AS_FIELD : 0; + uint32_t option = 0; + + if (m_instance->treat_string_as_field()) + { + option |= QC_OPTION_STRING_AS_FIELD; + } + + if (m_instance->treat_string_arg_as_field()) + { + option |= QC_OPTION_STRING_ARG_AS_FIELD; + } + EnableOption enable(option); GWBUF* analyzed_queue = buffer; diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.hh b/server/modules/filter/dbfwfilter/dbfwfilter.hh index 3ff2b8d63..6882751f0 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.hh +++ b/server/modules/filter/dbfwfilter/dbfwfilter.hh @@ -224,6 +224,16 @@ public: */ fw_actions get_action() const; + /** + * Should strings be treated as fields? + * + * @return True, if they should, false otherwise. + */ + bool treat_string_as_field() const + { + return m_treat_string_as_field; + } + /** * Should string args be treated as fields? * @@ -277,6 +287,7 @@ public: private: fw_actions m_action; /*< Default operation mode, defaults to deny */ int m_log_match; /*< Log matching and/or non-matching queries */ + bool m_treat_string_as_field; bool m_treat_string_arg_as_field; mutable std::mutex m_lock; /*< Instance spinlock */ std::string m_filename; /*< Path to the rule file */ From a3cf1d22c0ca499cff50088a076c40ebfd4955fc Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 3 May 2019 10:49:03 +0300 Subject: [PATCH 8/9] MXS-2457 Streamline logging --- server/core/query_classifier.cc | 5 +++++ server/modules/filter/dbfwfilter/dbfwfilter.cc | 16 +++++----------- server/modules/filter/masking/maskingfilter.cc | 14 ++++---------- 3 files changed, 14 insertions(+), 21 deletions(-) diff --git a/server/core/query_classifier.cc b/server/core/query_classifier.cc index cfb195591..665d57d64 100644 --- a/server/core/query_classifier.cc +++ b/server/core/query_classifier.cc @@ -1321,6 +1321,11 @@ bool qc_set_cache_properties(const QC_CACHE_PROPERTIES* properties) if (properties->max_size >= 0) { + if (properties->max_size == 0) + { + MXS_NOTICE("Query classifier cache disabled."); + } + this_unit.set_cache_max_size(properties->max_size); rv = true; } diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.cc b/server/modules/filter/dbfwfilter/dbfwfilter.cc index a4044f2ca..338a5fe23 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.cc +++ b/server/modules/filter/dbfwfilter/dbfwfilter.cc @@ -1287,24 +1287,18 @@ Dbfw* Dbfw::create(const char* zName, MXS_CONFIG_PARAMETER* pParams) { if (rval->treat_string_as_field() || rval->treat_string_arg_as_field()) { - MXS_NOTICE("The parameter 'treat_string_arg_as_field' and/or " - "'treat_string_as_field' is enabled for %s. " - "As a consequence, the query classifier cache must be disabled.", - zName); - QC_CACHE_PROPERTIES cache_properties; qc_get_cache_properties(&cache_properties); if (cache_properties.max_size != 0) { + MXS_NOTICE("The parameter 'treat_string_arg_as_field' or(and) " + "'treat_string_as_field' is enabled for %s, " + "disabling the query classifier cache.", + zName); + cache_properties.max_size = 0; qc_set_cache_properties(&cache_properties); - - MXS_NOTICE("Query classifier cache disabled."); - } - else - { - MXS_NOTICE("The query classifier cache was disabled already."); } } } diff --git a/server/modules/filter/masking/maskingfilter.cc b/server/modules/filter/masking/maskingfilter.cc index cd33cc8ae..5930fd0c6 100644 --- a/server/modules/filter/masking/maskingfilter.cc +++ b/server/modules/filter/masking/maskingfilter.cc @@ -188,23 +188,17 @@ MaskingFilter* MaskingFilter::create(const char* zName, MXS_CONFIG_PARAMETER* pP if (config.treat_string_arg_as_field()) { - MXS_NOTICE("The parameter 'treat_string_arg_as_field' is enabled for %s. " - "As a consequence, the query classifier cache must be disabled.", - zName); - QC_CACHE_PROPERTIES cache_properties; qc_get_cache_properties(&cache_properties); if (cache_properties.max_size != 0) { + MXS_NOTICE("The parameter 'treat_string_arg_as_field' is enabled for %s, " + "disabling the query classifier cache.", + zName); + cache_properties.max_size = 0; qc_set_cache_properties(&cache_properties); - - MXS_NOTICE("Query classifier cache disabled."); - } - else - { - MXS_NOTICE("The query classifier cache was disabled already."); } } } From e9144219f501deb9197606d6abb2d06652a7eae0 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 3 May 2019 11:36:23 +0300 Subject: [PATCH 9/9] MXS-2457 Add database firewall test --- maxscale-system-test/fw/deny19 | 2 ++ maxscale-system-test/fw/deny3 | 3 ++- maxscale-system-test/fw/pass19 | 6 ++++++ maxscale-system-test/fw/rules19 | 2 ++ maxscale-system-test/fwf.cpp | 2 +- 5 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 maxscale-system-test/fw/deny19 create mode 100644 maxscale-system-test/fw/pass19 create mode 100644 maxscale-system-test/fw/rules19 diff --git a/maxscale-system-test/fw/deny19 b/maxscale-system-test/fw/deny19 new file mode 100644 index 000000000..38771d22a --- /dev/null +++ b/maxscale-system-test/fw/deny19 @@ -0,0 +1,2 @@ +SELECT CONCAT(x1) FROM t1; +SELECT CONCAT("x1") FROM t1; \ No newline at end of file diff --git a/maxscale-system-test/fw/deny3 b/maxscale-system-test/fw/deny3 index 0c5f68f1a..5ac12cac6 100644 --- a/maxscale-system-test/fw/deny3 +++ b/maxscale-system-test/fw/deny3 @@ -1,3 +1,4 @@ update t1 set x1=1 where fl=0; SELECT x1 FROM t1; -select t1.x1 as 'something' from t1 as t1 limit 1; \ No newline at end of file +select t1.x1 as 'something' from t1 as t1 limit 1; +SELECT "x1" FROM t1; diff --git a/maxscale-system-test/fw/pass19 b/maxscale-system-test/fw/pass19 new file mode 100644 index 000000000..60f69d18e --- /dev/null +++ b/maxscale-system-test/fw/pass19 @@ -0,0 +1,6 @@ +DROP TABLE IF EXISTS t1; +CREATE TABLE t1 (x1 TEXT, x2 TEXT); +select sleep(5); +SELECT x1 FROM t1; +SELECT x2 FROM t1; +SELECT CONCAT(x2) FROM t1; diff --git a/maxscale-system-test/fw/rules19 b/maxscale-system-test/fw/rules19 new file mode 100644 index 000000000..7c67d8c3b --- /dev/null +++ b/maxscale-system-test/fw/rules19 @@ -0,0 +1,2 @@ +rule test19 match function concat columns x1 +users %@% match any rules test19 diff --git a/maxscale-system-test/fwf.cpp b/maxscale-system-test/fwf.cpp index af7cd6f1a..c20618d4b 100644 --- a/maxscale-system-test/fwf.cpp +++ b/maxscale-system-test/fwf.cpp @@ -44,7 +44,7 @@ int main(int argc, char* argv[]) Test->maxscales->access_homedir[0]); sprintf(rules_dir, "%s/fw/", test_dir); - int N = 18; + int N = 19; int i; for (i = 1; i < N + 1; i++)