Skip to content

Commit

Permalink
More protection against SQL injection
Browse files Browse the repository at this point in the history
  • Loading branch information
rschu1ze committed May 5, 2023
1 parent 7e5fded commit f7e31a9
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 12 deletions.
25 changes: 16 additions & 9 deletions src/Interpreters/InterpreterShowColumnsQuery.cpp
@@ -1,11 +1,12 @@
#include <Interpreters/InterpreterShowColumnsQuery.h>

#include <Common/quoteString.h>
#include <IO/Operators.h>
#include <IO/WriteBufferFromString.h>
#include <Parsers/ASTShowColumnsQuery.h>
#include <Parsers/formatAST.h>
#include <Interpreters/Context.h>
#include <Interpreters/executeQuery.h>
#include <IO/Operators.h>


namespace DB
Expand All @@ -23,16 +24,22 @@ String InterpreterShowColumnsQuery::getRewrittenQuery()
{
const auto & query = query_ptr->as<ASTShowColumnsQuery &>();

String database = query.database.empty() ? getContext()->getCurrentDatabase() : query.database;
String table = query.table;
WriteBufferFromOwnString buf_database;
String resolved_database = getContext()->resolveDatabase(query.database);
writeEscapedString(resolved_database, buf_database);
String database = buf_database.str();

WriteBufferFromOwnString buf_table;
writeEscapedString(query.table, buf_table);
String table = buf_table.str();

String rewritten_query = R"(
SELECT
name AS field,
type AS type,
startsWith(type, 'Nullable') AS null,
trim(concatWithSeparator(' ', if(is_in_primary_key, 'PRI', ''), if (is_in_sorting_key, 'SOR', ''))) AS key,
if(default_kind IN ('ALIAS', 'DEFAULT', 'MATERIALIZED'), default_expression, NULL) AS default,
trim(concatWithSeparator(' ', if (is_in_primary_key, 'PRI', ''), if (is_in_sorting_key, 'SOR', ''))) AS key,
if (default_kind IN ('ALIAS', 'DEFAULT', 'MATERIALIZED'), default_expression, NULL) AS default,
'' AS extra )";

// TODO Interpret query.extended. It is supposed to show internal/virtual columns. Need to fetch virtual column names, see
Expand All @@ -51,10 +58,10 @@ SELECT
}

rewritten_query += fmt::format(R"(
FROM system.columns
WHERE
database = '{}'
AND table = '{}' )", database, table);
FROM system.columns
WHERE
database = '{}'
AND table = '{}' )", database, table);

if (!query.like.empty())
{
Expand Down
14 changes: 11 additions & 3 deletions src/Interpreters/InterpreterShowIndexesQuery.cpp
@@ -1,11 +1,12 @@
#include <Interpreters/InterpreterShowIndexesQuery.h>

#include <Common/quoteString.h>
#include <IO/Operators.h>
#include <IO/WriteBufferFromString.h>
#include <Parsers/ASTShowIndexesQuery.h>
#include <Parsers/formatAST.h>
#include <Interpreters/Context.h>
#include <Interpreters/executeQuery.h>
#include <IO/Operators.h>


namespace DB
Expand All @@ -23,8 +24,15 @@ String InterpreterShowIndexesQuery::getRewrittenQuery()
{
const auto & query = query_ptr->as<ASTShowIndexesQuery &>();

String table = query.table;
String database = query.database.empty() ? getContext()->getCurrentDatabase() : query.database;
WriteBufferFromOwnString buf_table;
writeEscapedString(query.table, buf_table);
String table = buf_table.str();

WriteBufferFromOwnString buf_database;
String resolved_database = getContext()->resolveDatabase(query.database);
writeEscapedString(resolved_database, buf_database);
String database = buf_database.str();

String where_expression = query.where_expression ? fmt::format("WHERE ({})", query.where_expression) : "";

String rewritten_query = fmt::format(R"(
Expand Down
5 changes: 5 additions & 0 deletions tests/queries/0_stateless/02706_show_columns.reference
Expand Up @@ -28,6 +28,11 @@ int32 Nullable(Int32) 1 \N
uint64 UInt64 0 PRI SOR \N
--- LIMIT
int32 Nullable(Int32) 1 \N
--- Check with weird table names
c String 0 PRI SOR \N
c String 0 PRI SOR \N
c String 0 PRI SOR \N
c String 0 PRI SOR \N
--- Original table
int32 Nullable(Int32) 1 \N
str String 0 SOR \N
Expand Down
20 changes: 20 additions & 0 deletions tests/queries/0_stateless/02706_show_columns.sql
Expand Up @@ -43,6 +43,26 @@ SHOW COLUMNS FROM tab WHERE field LIKE '%int%';
SELECT '--- LIMIT';
SHOW COLUMNS FROM tab LIMIT 1;

SELECT '--- Check with weird table names';

DROP TABLE IF EXISTS `$4@^7`;
CREATE TABLE `$4@^7` (c String) ENGINE = MergeTree ORDER BY c;
SHOW COLUMNS FROM `$4@^7`;
DROP TABLE `$4@^7`;

DROP TABLE IF EXISTS NULL;
CREATE TABLE NULL (c String) ENGINE = MergeTree ORDER BY c;
SHOW COLUMNS FROM NULL;
DROP TABLE NULL;

DROP DATABASE IF EXISTS `'`;
CREATE DATABASE `'`;
CREATE TABLE `'`.`'` (c String) ENGINE = MergeTree ORDER BY c;
SHOW COLUMNS FROM `'` FROM `'`;
SHOW COLUMNS FROM `'`.`'`; -- abbreviated form
DROP TABLE `'`.`'`;
DROP DATABASE `'`;

-- Create a table in a different database. Intentionally useing the same table/column names as above so
-- we notice if something is buggy in the implementation of SHOW COLUMNS.
DROP DATABASE IF EXISTS database_123456789abcde;
Expand Down
5 changes: 5 additions & 0 deletions tests/queries/0_stateless/02724_show_indexes.reference
Expand Up @@ -23,6 +23,11 @@ tbl 0 set_idx \N \N \N \N \N \N \N set \N \N YES e
--- WHERE
tbl 0 mm1_idx \N \N \N \N \N \N \N minmax \N \N YES a, c, d
tbl 0 mm2_idx \N \N \N \N \N \N \N minmax \N \N YES c, d, e
--- Check with weird table names
$4@^7 0 PRIMARY \N \N A \N \N \N \N primary \N \N YES c
NULL 0 PRIMARY \N \N A \N \N \N \N primary \N \N YES c
\' 0 PRIMARY \N \N A \N \N \N \N primary \N \N YES c
\' 0 PRIMARY \N \N A \N \N \N \N primary \N \N YES c
--- Original table
tbl 0 blf_idx \N \N \N \N \N \N \N bloom_filter \N \N YES d, b
tbl 0 mm1_idx \N \N \N \N \N \N \N minmax \N \N YES a, c, d
Expand Down
20 changes: 20 additions & 0 deletions tests/queries/0_stateless/02724_show_indexes.sql
Expand Up @@ -30,6 +30,26 @@ SHOW EXTENDED INDEX FROM tbl;
SELECT '--- WHERE';
SHOW INDEX FROM tbl WHERE index_type LIKE '%minmax%';

SELECT '--- Check with weird table names';

DROP TABLE IF EXISTS `$4@^7`;
CREATE TABLE `$4@^7` (c String) ENGINE = MergeTree ORDER BY c;
SHOW INDEX FROM `$4@^7`;
DROP TABLE `$4@^7`;

DROP TABLE IF EXISTS NULL;
CREATE TABLE NULL (c String) ENGINE = MergeTree ORDER BY c;
SHOW INDEX FROM NULL;
DROP TABLE NULL;

DROP DATABASE IF EXISTS `'`;
CREATE DATABASE `'`;
CREATE TABLE `'`.`'` (c String) ENGINE = MergeTree ORDER BY c;
SHOW INDEX FROM `'` FROM `'`;
SHOW INDEX FROM `'`.`'`; -- abbreviated form
DROP TABLE `'`.`'`;
DROP DATABASE `'`;

-- Create a table in a different database. Intentionally using the same table/column names as above so
-- we notice if something is buggy in the implementation of SHOW INDEX.
DROP DATABASE IF EXISTS database_123456789abcde;
Expand Down

0 comments on commit f7e31a9

Please sign in to comment.