Merge pull request #777 from metabase/block-unsafe-h2-native-queries

 block unsafe native queries against H2 databases

src/metabase/driver/generic_sql.clj

@@ -128,21 +128,30 @@
               (float (/ url-count total-non-null-count))))))
       0.0))
 
-(defn extend-add-generic-sql-mixins [driver-type]
-  (extend driver-type
-    IDriver
-    {:can-connect?                  can-connect?
-     :can-connect-with-details?     can-connect-with-details?
-     :wrap-process-query-middleware wrap-process-query-middleware
-     :process-query                 process-query
-     :sync-in-context               sync-in-context
-     :active-table-names            active-table-names
-     :active-column-names->type     active-column-names->type
-     :table-pks                     table-pks
-     :field-values-lazy-seq         field-values-lazy-seq}
-    ISyncDriverTableFKs
-    {:table-fks table-fks}
-    ISyncDriverFieldAvgLength
-    {:field-avg-length field-avg-length}
-    ISyncDriverFieldPercentUrls
-    {:field-percent-urls field-percent-urls}))
+(def ^:const GenericSQLIDriverMixin
+  "Generic SQL implementation of the `IDriver` protocol.
+
+     (extend H2Driver
+       IDriver
+       GenericSQLIDriverMixin)"
+  {:can-connect?                  can-connect?
+   :can-connect-with-details?     can-connect-with-details?
+   :wrap-process-query-middleware wrap-process-query-middleware
+   :process-query                 process-query
+   :sync-in-context               sync-in-context
+   :active-table-names            active-table-names
+   :active-column-names->type     active-column-names->type
+   :table-pks                     table-pks
+   :field-values-lazy-seq         field-values-lazy-seq})
+
+(def ^:const GenericSQLISyncDriverTableFKsMixin
+  "Generic SQL implementation of the `ISyncDriverTableFKs` protocol."
+  {:table-fks table-fks})
+
+(def ^:const GenericSQLISyncDriverFieldAvgLengthMixin
+  "Generic SQL implementation of the `ISyncDriverFieldAvgLengthMixin` protocol."
+  {:field-avg-length field-avg-length})
+
+(def ^:const GenericSQLISyncDriverFieldPercentUrlsMixin
+  "Generic SQL implementation of the `ISyncDriverFieldPercentUrls` protocol."
+  {:field-percent-urls field-percent-urls})

src/metabase/driver/generic_sql/native.clj

@@ -23,7 +23,7 @@
   {:pre [(string? sql)
          (integer? database-id)]}
   (log/debug "QUERY: \n"
-             (with-out-str (clojure.pprint/pprint query)))
+             (with-out-str (clojure.pprint/pprint (update query :driver class))))
   (try (let [database (sel :one [Database :engine :details] :id database-id)
              db (-> database
                     db->korma-db

src/metabase/driver/h2.clj

@@ -3,8 +3,11 @@
             [korma.db :as kdb]
             [metabase.db :as db]
             [metabase.driver :as driver]
-            [metabase.driver.generic-sql :as generic-sql]
-            [metabase.driver.generic-sql.interface :refer :all]))
+            (metabase.driver [generic-sql :as generic-sql, :refer [GenericSQLIDriverMixin GenericSQLISyncDriverTableFKsMixin
+                                                                   GenericSQLISyncDriverFieldAvgLengthMixin GenericSQLISyncDriverFieldPercentUrlsMixin]]
+                             [interface :refer [IDriver ISyncDriverTableFKs ISyncDriverFieldAvgLength ISyncDriverFieldPercentUrls]])
+            [metabase.driver.generic-sql.interface :refer :all]
+            [metabase.models.database :refer [Database]]))
 
 (def ^:private ^:const column->base-type
   "Map of H2 Column types -> Field base types. (Add more mappings here as needed)"
@@ -115,7 +118,27 @@
               :milliseconds "MILLISECOND")
             table-name field-name)))
 
-(generic-sql/extend-add-generic-sql-mixins H2Driver)
+(defn- wrap-process-query-middleware [_ qp]
+  (fn [{query-type :type, :as query}]
+    {:pre [query-type]}
+    ;; For :native queries check to make sure the DB in question has a (non-default) NAME property specified in the connection string.
+    ;; We don't allow SQL execution on H2 databases for the default admin account for security reasons
+    (when (= (keyword query-type) :native)
+      (let [{:keys [db]}   (db/sel :one :field [Database :details] :id (:database query))
+            _              (assert db)
+            [_ options]    (connection-string->file+options db)
+            {:strs [USER]} options]
+        (when (or (s/blank? USER)
+                  (= USER "sa")) ; "sa" is the default USER
+          (throw (Exception. "Running SQL queries against H2 databases using the default (admin) database user is forbidden.")))))
+    (qp query)))
+
+(extend H2Driver
+  ;; Override the generic SQL implementation of wrap-process-query-middleware so we can block unsafe native queries (see above)
+  IDriver                     (assoc GenericSQLIDriverMixin :wrap-process-query-middleware wrap-process-query-middleware)
+  ISyncDriverTableFKs         GenericSQLISyncDriverTableFKsMixin
+  ISyncDriverFieldAvgLength   GenericSQLISyncDriverFieldAvgLengthMixin
+  ISyncDriverFieldPercentUrls GenericSQLISyncDriverFieldPercentUrlsMixin)
 
 (def ^:const driver
   (map->H2Driver {:column->base-type    column->base-type

src/metabase/driver/postgres.clj

@@ -8,7 +8,10 @@
             [metabase.db :refer [upd]]
             [metabase.models.field :refer [Field]]
             [metabase.driver :as driver]
-            [metabase.driver.interface :refer [ISyncDriverSpecificSyncField driver-specific-sync-field!]]
+            (metabase.driver [generic-sql :as generic-sql, :refer [GenericSQLIDriverMixin GenericSQLISyncDriverTableFKsMixin
+                                                                   GenericSQLISyncDriverFieldAvgLengthMixin GenericSQLISyncDriverFieldPercentUrlsMixin]]
+                             [interface :refer [IDriver ISyncDriverTableFKs ISyncDriverFieldAvgLength ISyncDriverFieldPercentUrls
+                                                ISyncDriverSpecificSyncField driver-specific-sync-field!]])
             [metabase.driver.generic-sql :as generic-sql]
             (metabase.driver.generic-sql [interface :refer :all]
                                          [util :refer [with-jdbc-metadata]])))
@@ -117,7 +120,11 @@
           (upd Field (:id field) :special_type :json)
           (assoc field :special_type :json))))))
 
-(generic-sql/extend-add-generic-sql-mixins PostgresDriver)
+(extend PostgresDriver
+  IDriver                     GenericSQLIDriverMixin
+  ISyncDriverTableFKs         GenericSQLISyncDriverTableFKsMixin
+  ISyncDriverFieldAvgLength   GenericSQLISyncDriverFieldAvgLengthMixin
+  ISyncDriverFieldPercentUrls GenericSQLISyncDriverFieldPercentUrlsMixin)
 
 (def ^:const driver
   (map->PostgresDriver {:column->base-type    column->base-type

test/metabase/driver/generic_sql/native_test.clj

@@ -2,7 +2,9 @@
   (:require [clojure.tools.logging :as log]
             [colorize.core :as color]
             [expectations :refer :all]
+            [metabase.db :refer [ins cascade-delete]]
             [metabase.driver :as driver]
+            [metabase.models.database :refer [Database]]
             [metabase.test.data :refer :all]))
 
 ;; Just check that a basic query works
@@ -40,3 +42,13 @@
               :stacktrace
               :query
               :expanded-query)))
+
+;; Check that we're not allowed to run SQL against an H2 database with a non-admin account
+(expect "Running SQL queries against H2 databases using the default (admin) database user is forbidden."
+  ;; Insert a fake Database. It doesn't matter that it doesn't actually exist since query processing should
+  ;; fail immediately when it realizes this DB doesn't have a USER
+  (let [db (ins Database :name "Fake-H2-DB", :engine "h2", :details {:db "mem:fake-h2-db"})]
+    (try (:error (driver/process-query {:database (:id db)
+                                        :type     :native
+                                        :native   {:query "SELECT 1;"}}))
+         (finally (cascade-delete Database :name "Fake-H2-DB")))))

test/metabase/test/data/h2.clj

@@ -28,10 +28,7 @@
 (defn- connection-details
   "Return a Metabase `Database.details` for H2 database defined by DATABASE-DEFINITION."
   [^DatabaseDefinition {:keys [short-lived?], :as database-definition}]
-  {:db (str (format "mem:%s" (escaped-name database-definition))
-            ;; For non "short-lived" (temp) databases keep the connection open for the duration of unit tests
-            (when-not short-lived?
-              ";DB_CLOSE_DELAY=-1"))
+  {:db           (format "mem:%s" (escaped-name database-definition))
    :short-lived? short-lived?})
 
 (defn- korma-connection-pool
@@ -72,13 +69,16 @@
   (generic/field-base-type->sql-type [_ field-type]
     (field-base-type->sql-type field-type)))
 
+
 (extend-protocol IDatasetLoader
   H2DatasetLoader
   (engine [_]
     :h2)
 
   (database->connection-details [_ database-definition]
-    (connection-details database-definition))
+    ;; Return details with the GUEST user added so SQL queries are allowed
+    (let [details (connection-details database-definition)]
+      (update details :db str ";USER=GUEST;PASSWORD=guest")))
 
   (drop-physical-db! [_ database-definition]
     ;; Nothing to do here - there are no physical dbs <3
@@ -88,7 +88,19 @@
     (generic/create-physical-table! this database-definition (format-for-h2 table-definition)))
 
   (create-physical-db! [this database-definition]
-    (generic/create-physical-db! this (format-for-h2 database-definition)))
+    ;; Create the "physical" database which in this case actually just means creating the schema
+    (generic/create-physical-db! this (format-for-h2 database-definition))
+
+    ;; Now create a non-admin account 'GUEST' which will be used from here on out
+    (generic/execute-sql! this database-definition "CREATE USER IF NOT EXISTS GUEST PASSWORD 'guest';")
+    ;; Grant the GUEST account SELECT permissions for all the Tables in this DB
+    (doseq [{:keys [table-name]} (:table-definitions database-definition)]
+      (generic/execute-sql! this database-definition (format "GRANT SELECT ON %s TO GUEST;" table-name)))
+
+    ;; If this isn't a "short-lived" database we need to set DB_CLOSE_DELAY to -1 here because only admins are allowed to do it
+    ;; so we can't set it via the connection string :/
+    (when-not (:short-lived? database-definition)
+      (generic/execute-sql! this database-definition "SET DB_CLOSE_DELAY -1;")))
 
   (load-table-data! [this database-definition table-definition]
     (generic/load-table-data! this database-definition table-definition))