Prevent malicious H2 connection string properties (#32733)

* Prevent malicious H2 connection string properties

* Fix Kondo lint warning

---------

Co-authored-by: dan sutton <dan@dpsutton.com>

src/metabase/api/setup.clj

@@ -175,6 +175,9 @@
   [:as {{{:keys [engine details]} :details, token :token} :body}]
   {token  SetupToken
    engine DBEngineString}
+  (when (setup/has-user-setup)
+    (throw (ex-info (tru "Instance already initialized")
+                    {:status-code 400})))
   (let [engine       (keyword engine)
         error-or-nil (api.database/test-database-connection engine details)]
     (when error-or-nil

src/metabase/driver/h2.clj

@@ -39,6 +39,19 @@
   [_driver]
   2)
 
+(defn- get-field
+  "Returns value of private field. This function is used to bypass field protection to instantiate
+   a low-level H2 Parser object in order to detect DDL statements in queries."
+  ([obj field]
+   (.get (doto (.getDeclaredField (class obj) field)
+           (.setAccessible true))
+         obj))
+  ([obj field or-else]
+   (try (get-field obj field)
+        (catch java.lang.NoSuchFieldException _e
+          ;; when there are no fields: return or-else
+          or-else))))
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                             metabase.driver impls                                              |
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -74,6 +87,41 @@
    (map u/one-or-many)
    (apply concat)))
 
+(defn- malicious-property-value
+  "Checks an h2 connection string for connection properties that could be malicious. Markers of this include semi-colons
+  which allow for sql injection in org.h2.engine.Engine/openSession. The others are markers for languages like
+  javascript and ruby that we want to suppress."
+  [s]
+  ;; list of strings it looks for to compile scripts:
+  ;; https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/util/SourceCompiler.java#L178-L187 we
+  ;; can't use the static methods themselves since they expect to check the beginning of the string
+  (let [bad-markers [";"
+                     "//javascript"
+                     "#ruby"
+                     "//groovy"
+                     "@groovy"]
+        pred        (apply some-fn (map (fn [marker] (fn [s] (str/includes? s marker)))
+                                        bad-markers))]
+    (pred s)))
+
+(defmethod driver/can-connect? :h2
+  [driver {:keys [db] :as details}]
+  (when (string? db)
+    (let [connection-str  (cond-> db
+                            (not (str/includes? db "h2:")) (str/replace-first #"^" "h2:")
+                            (not (str/includes? db "jdbc:")) (str/replace-first #"^" "jdbc:"))
+          connection-info (org.h2.engine.ConnectionInfo. connection-str nil nil nil)
+          properties      (get-field connection-info "prop")
+          bad-props       (into {} (keep (fn [[k v]] (when (malicious-property-value v) [k v])))
+                                properties)]
+      (when (seq bad-props)
+        (throw (ex-info "Malicious keys detected" {:keys (keys bad-props)})))
+      ;; keys are uppercased by h2 when parsed:
+      ;; https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/engine/ConnectionInfo.java#L298
+      (when (contains? properties "INIT")
+        (throw (ex-info "INIT not allowed" {:keys ["INIT"]})))))
+  (sql-jdbc.conn/can-connect? driver details))
+
 (defmethod driver/db-start-of-week :h2
   [_]
   :monday)
@@ -111,18 +159,6 @@
            (ex-info (tru "Running SQL queries against H2 databases using the default (admin) database user is forbidden.")
                     {:type qp.error-type/db})))))))
 
-(defn- get-field
-  "Returns value of private field. This function is used to bypass field protection to instantiate
-   a low-level H2 Parser object in order to detect DDL statements in queries."
-  ([obj field]
-   (.get (doto (.getDeclaredField (class obj) field)
-           (.setAccessible true))
-         obj))
-  ([obj field or-else]
-   (try (get-field obj field)
-        (catch java.lang.NoSuchFieldException _e
-          ;; when there are no fields: return or-else
-          or-else))))
 
 (defn- make-h2-parser
   "Returns an H2 Parser object for the given (H2) database ID"

src/metabase/setup.clj

@@ -1,9 +1,11 @@
 (ns metabase.setup
   (:require
    [environ.core :as env]
+   [metabase.config :as config]
    [metabase.db.connection :as mdb.connection]
    [metabase.models.setting :as setting :refer [defsetting Setting]]
    [metabase.models.user :refer [User]]
+   [metabase.util.i18n :refer [deferred-tru tru]]
    [toucan2.core :as t2]))
 
 (set! *warn-on-reflection* true)
@@ -35,10 +37,14 @@
       (setting/set-value-of-type! :string :setup-token (str (random-uuid)))))
 
 (defsetting has-user-setup
-  "A value that is true iff the metabase instance has one or more users registered."
+  (deferred-tru "A value that is true iff the metabase instance has one or more users registered.")
   :visibility :public
   :type       :boolean
-  :setter     :none
+  :setter     (fn [value]
+                (if (or config/is-dev? config/is-test?)
+                  (setting/set-value-of-type! :boolean :has-user-setup value)
+                  (throw (ex-info (tru "Cannot set `has-user-setup`.")
+                                  {:value value}))))
   ;; Once a User is created it's impossible for this to ever become falsey -- deleting the last User is disallowed.
   ;; After this returns true once the result is cached and it will continue to return true forever without any
   ;; additional DB hits.
@@ -47,8 +53,14 @@
   ;; it out in the REPL
   :getter     (let [app-db-id->user-exists? (atom {})]
                 (fn []
-                  (or (get @app-db-id->user-exists? (mdb.connection/unique-identifier))
-                      (let [exists? (t2/exists? User)]
-                        (swap! app-db-id->user-exists? assoc (mdb.connection/unique-identifier) exists?)
-                        exists?))))
+                  (let [possible-override (when (or config/is-dev? config/is-test?)
+                                            ;; allow for overriding in dev and test
+                                            (setting/get-value-of-type :boolean :has-user-setup))]
+                    ;; override could be false so have to check non-nil
+                    (if (some? possible-override)
+                      possible-override
+                      (or (get @app-db-id->user-exists? (mdb.connection/unique-identifier))
+                          (let [exists? (t2/exists? User)]
+                            (swap! app-db-id->user-exists? assoc (mdb.connection/unique-identifier) exists?)
+                            exists?))))))
   :doc        false)

test/metabase/api/setup_test.clj

@@ -378,28 +378,31 @@
 
 (deftest validate-setup-test
   (testing "POST /api/setup/validate"
+
     (testing "Should validate token"
-      (is (= {:errors {:token "Token does not match the setup token."}}
-             (client/client :post 400 "setup/validate" {})))
-      (is (= {:errors {:token "Token does not match the setup token."}}
-             (client/client :post 400 "setup/validate" {:token "foobar"})))
+      (mt/with-temporary-setting-values [has-user-setup false]
+        (is (= {:errors {:token "Token does not match the setup token."}}
+               (client/client :post 400 "setup/validate" {})))
+        (is (= {:errors {:token "Token does not match the setup token."}}
+               (client/client :post 400 "setup/validate" {:token "foobar"}))))
       ;; make sure we have a valid setup token
       (setup/create-token!)
       (is (= {:errors {:engine "value must be a valid database engine."}}
              (client/client :post 400 "setup/validate" {:token (setup/setup-token)}))))
 
-    (testing "should validate that database connection works"
-      (is (= {:errors {:db "check your connection string"},
-              :message "Database cannot be found."}
-             (client/client :post 400 "setup/validate" {:token   (setup/setup-token)
-                                                        :details {:engine  "h2"
-                                                                  :details {:db "file:///tmp/fake.db"}}}))))
-
-    (testing "should return 204 no content if everything is valid"
-      (is (= nil
-             (client/client :post 204 "setup/validate" {:token   (setup/setup-token)
-                                                        :details {:engine  "h2"
-                                                                  :details (:details (mt/db))}}))))))
+    (mt/with-temporary-setting-values [has-user-setup false]
+      (testing "should validate that database connection works"
+        (is (= {:errors {:db "check your connection string"},
+                :message "Database cannot be found."}
+               (client/client :post 400 "setup/validate" {:token   (setup/setup-token)
+                                                          :details {:engine  "h2"
+                                                                    :details {:db "file:///tmp/fake.db"}}}))))
+
+      (testing "should return 204 no content if everything is valid"
+        (is (= nil
+               (client/client :post 204 "setup/validate" {:token   (setup/setup-token)
+                                                          :details {:engine  "h2"
+                                                                    :details (:details (mt/db))}})))))))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+

test/metabase/driver/h2_test.clj

@@ -73,6 +73,20 @@
                   (and (re-matches #"Database .+ not found, .+" (.getMessage e))
                        ::exception-thrown)))))))
 
+(deftest ^:parallel only-connect-when-non-malicious-properties
+  (testing "Reject connection strings with malicious properties"
+    (let [conn-str (str "jdbc:h2:file:"
+                        (System/getProperty "user.dir")
+                        "/toucan_sightings.db"
+                        ";TRACE_LEVEL_SYSTEM_OUT=1\\;CREATE TRIGGER IAMPWNED BEFORE SELECT ON INFORMATION_SCHEMA.TABLES AS $$//javascript\nnew java.net.URL('http://localhost:3000/api/health').openConnection().getContentLength()\n$$--=x\\;")
+          result (try (driver/can-connect? :h2 {:db conn-str})
+                      ::did-not-throw
+                      (catch Exception e e))]
+      (is (instance? clojure.lang.ExceptionInfo result))
+      (is (partial= {:cause "Malicious keys detected"
+                     :data {:keys ["TRACE_LEVEL_SYSTEM_OUT"]}}
+                    (Throwable->map result))))))
+
 (deftest db-default-timezone-test
   (mt/test-driver :h2
     ;; [[driver/db-default-timezone]] returns `nil`. This *probably* doesn't make sense. We should go in an fix it, by

test/metabase/setup_test.clj

@@ -30,8 +30,8 @@
           (is (= false
                  (setup/has-user-setup))))
         (testing "Should continue doing new DB calls as long as there is no User"
-          (is (= 5
-                 (call-count)))))))
+          (is (<= (call-count)
+                  10)))))) ;; in dev/test we check settings for an override
   (testing "Switch back to the 'normal' app DB; value should still be cached for it"
     (t2/with-call-count [call-count]
       (is (= true