From 32adcb1dbfdf9df73ccdd20a77d1aa69132539ef Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Fri, 24 Feb 2023 17:40:19 +0200
Subject: [PATCH] Whitelist H2 commands for actions (#28583)
* Decouple checking ddl from classifying h2 stmts
- should enable followup for easily blocking more kinds of queries
- check all statements to make sure they aren't "ddl".
* fix classify-query
* linter fixes + get-field refactor
* return the CommandInterface values as ints
Reach into the CommandList when needed
* docstring wording
* catch invalid queries -- they can't be classified
* Remove action subtypes from inlined-models
* Add ddl check for `execute-write-query!`
* Remove ACCESS_MODE_DATA
* Check queries are single select statements
* Add test for sample database privileges
* Fix single-select check
* Add single-select test
* Rename and add more tests for checking read only commands
* commands -> statements
* Fix check-disallow-ddl-commands
* new line
* Add more read-only statements to the tests
* Update error text
* Use are
* Add integration test for executing actions with disallowed commands
* Add test before inserting row
* Run GRANT ALL ON SCHEMA "PUBLIC" TO GUEST
* Restore classify-query
* whitespace
* Whitelist command types for actions
* Add comment
* Rename
* Remove dupes
* Add truncate test
* Add DDL commands
* Rename to query-classification
* Update error message
* Fix test
---------
Co-authored-by: Bryan Maass <bryan.maass@gmail.com>
---
src/metabase/driver/h2.clj | 32 +++++++++++++++++---------------
test/metabase/driver/h2_test.clj | 23 +++++++++++++++--------
2 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/src/metabase/driver/h2.clj b/src/metabase/driver/h2.clj
index 32438e9cafc..a3fbadb8083 100644
--- a/src/metabase/driver/h2.clj
+++ b/src/metabase/driver/h2.clj
@@ -164,25 +164,27 @@
(catch org.h2.message.DbException _
{:command-types [] :remaining-sql nil}))))
-(defn- cmd-type-ddl? [cmd-type]
- ;; Command types are organized with all DDL commands listed first, so all ddl commands are before ALTER_SEQUENCE.
- ;; see https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/command/CommandInterface.java#L297
- (< cmd-type CommandInterface/ALTER_SEQUENCE))
-
-(defn- contains-ddl? [{:keys [command-types remaining-sql]}]
+(defn- every-command-allowed-for-actions? [{:keys [command-types remaining-sql]}]
(let [cmd-type-nums command-types]
(boolean
- (or (some cmd-type-ddl? cmd-type-nums)
- (some? remaining-sql)))))
-
-;; TODO: black-list RUNSCRIPT, and a bunch more -- but they're not technically ddl. Should be simple to build off of [[classify-query]].
-;; e.g.: similar to contains-ddl? but instead of cmd-type-ddl? use: #(#{CommandInterface/RUNSCRIPT} %)
+ ;; Command types are organized with all DDL commands listed first, so all ddl commands are before ALTER_SEQUENCE.
+ ;; see https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/command/CommandInterface.java#L297
+ (and (every? #{CommandInterface/INSERT
+ CommandInterface/MERGE
+ CommandInterface/TRUNCATE_TABLE
+ CommandInterface/UPDATE
+ CommandInterface/DELETE
+ ;; Read-only commands might not make sense for actions, but they are allowed
+ CommandInterface/SELECT ; includes SHOW, TABLE, VALUES
+ CommandInterface/EXPLAIN
+ CommandInterface/CALL} cmd-type-nums)
+ (nil? remaining-sql)))))
-(defn- check-disallow-ddl-commands [{:keys [database] {:keys [query]} :native}]
+(defn- check-action-commands-allowed [{:keys [database] {:keys [query]} :native}]
(when query
(when-let [query-classification (classify-query database query)]
- (when (contains-ddl? query-classification)
- (throw (ex-info "IllegalArgument: DDL commands are not allowed to be used with h2."
+ (when-not (every-command-allowed-for-actions? query-classification)
+ (throw (ex-info "DDL commands are not allowed to be used with H2."
{:classification query-classification}))))))
(defn- read-only-statements? [{:keys [command-types remaining-sql]}]
@@ -209,7 +211,7 @@
(defmethod driver/execute-write-query! :h2
[driver query]
(check-native-query-not-using-default-user query)
- (check-disallow-ddl-commands query)
+ (check-action-commands-allowed query)
((get-method driver/execute-write-query! :sql-jdbc) driver query))
(defmethod sql.qp/add-interval-honeysql-form :h2
diff --git a/test/metabase/driver/h2_test.clj b/test/metabase/driver/h2_test.clj
index 9820914593e..cb87b9a1e33 100644
--- a/test/metabase/driver/h2_test.clj
+++ b/test/metabase/driver/h2_test.clj
@@ -168,9 +168,9 @@
(some-> (qp/compile query) :query pretty-sql))))))))
-(deftest classify-ddl-test
+(deftest check-action-commands-test
(mt/test-driver :h2
- (are [query] (= false (#'h2/contains-ddl? (#'h2/classify-query (u/the-id (mt/db)) query)))
+ (are [query] (= true (#'h2/every-command-allowed-for-actions? (#'h2/classify-query (u/the-id (mt/db)) query)))
"select 1"
"update venues set name = 'bill'"
"delete venues"
@@ -179,9 +179,16 @@
delete venues;"
"update venues set name = 'stomp';"
"select * from venues; update venues set name = 'stomp';"
- "update venues set name = 'stomp'; select * from venues;")
+ "update venues set name = 'stomp'; select * from venues;"
+ "truncate table venues"
+ "insert into venues values (1, 'Chicken Chow')"
+ "merge into venues key(1) values (1, 'Chicken Chow')"
+ "merge into venues using (select 1 as id) as source on (venues.id = source.id) when matched then update set name = 'Chicken Chow';")
- (are [query] (= true (#'h2/contains-ddl? (#'h2/classify-query (u/the-id (mt/db)) query)))
+ (are [query] (= false (#'h2/every-command-allowed-for-actions? (#'h2/classify-query (u/the-id (mt/db)) query)))
+ "create table venues (id int, name varchar(255))"
+ "alter table venues add column address varchar(255)"
+ "drop table venues"
"select * from venues; update venues set name = 'stomp';
CREATE ALIAS EXEC AS 'String shellexec(String cmd) throws java.io.IOException {Runtime.getRuntime().exec(cmd);return \"y4tacker\";}';
EXEC ('open -a Calculator.app')"
@@ -189,9 +196,9 @@
CREATE ALIAS EXEC AS 'String shellexec(String cmd) throws java.io.IOException {Runtime.getRuntime().exec(cmd);return \"y4tacker\";}';"
"CREATE ALIAS EXEC AS 'String shellexec(String cmd) throws java.io.IOException {Runtime.getRuntime().exec(cmd);return \"y4tacker\";}';")
- (is (= nil (#'h2/check-disallow-ddl-commands {:database (u/the-id (mt/db)) :native {:query nil}})))
+ (is (= nil (#'h2/check-action-commands-allowed {:database (u/the-id (mt/db)) :native {:query nil}})))
- (is (= nil (#'h2/check-disallow-ddl-commands
+ (is (= nil (#'h2/check-action-commands-allowed
{:database (u/the-id (mt/db))
:engine :h2
:native {:query (str/join "; "
@@ -203,7 +210,7 @@
"SELECT * FROM INFORMATION_SCHEMA.Users;"])]
(is (thrown? clojure.lang.ExceptionInfo
#"DDL commands are not allowed to be used with h2."
- (#'h2/check-disallow-ddl-commands
+ (#'h2/check-action-commands-allowed
{:database (u/the-id (mt/db))
:engine :h2
:native {:query trigger-creation-attempt}}))))))
@@ -251,7 +258,7 @@
:dataset_query {:database (mt/id)
:type "native"
:native {:query sql}}}]
- (is (=? {:message "Error executing Action: IllegalArgument: DDL commands are not allowed to be used with h2."}
+ (is (=? {:message "Error executing Action: DDL commands are not allowed to be used with H2."}
(mt/user-http-request :crowberto
:post 500
(format "action/%s/execute" action-id)))))))
--
GitLab