Fix SQL Injection Vulnerabilities in CMR Metadata App in provider and collection files

metadata-db-app/src/cmr/metadata_db/data/oracle/collection_table.clj

@@ -1,7 +1,8 @@
 (ns cmr.metadata-db.data.oracle.collection-table
   "Contains helper functions to create collection table."
   (:require
-   [clojure.java.jdbc :as j]))
+    [clojure.java.jdbc :as jdbc]
+    [cmr.metadata-db.data.util :as util]))
 
 (defmulti collection-column-sql
   "Returns the sql to define provider collection columns"
@@ -40,6 +41,7 @@
 
 (defmethod collection-constraint-sql false
   [provider table-name]
+  (util/validate-table-name table-name)
   (format (str "CONSTRAINT %s_pk PRIMARY KEY (id), "
 
                ;; Unique constraint on native id and revision id
@@ -63,6 +65,7 @@
 
 (defmethod collection-constraint-sql true
   [provider table-name]
+  (util/validate-table-name table-name)
   (format (str "CONSTRAINT %s_pk PRIMARY KEY (id), "
 
             ;; Unique constraint on provider id, native id and revision id
@@ -91,43 +94,45 @@
 
 (defmethod create-collection-indexes false
   [db _ table-name]
-  (j/db-do-commands db (format "CREATE INDEX %s_crdi ON %s (concept_id, revision_id, deleted, delete_time)"
-                               table-name
-                               table-name))
-  (j/db-do-commands db (format "CREATE INDEX %s_snv_i ON %s (short_name, version_id)"
-                               table-name
-                               table-name))
-  (j/db-do-commands db (format "CREATE INDEX %s_eid_i ON %s (entry_id)"
-                               table-name
-                               table-name))
-  (j/db-do-commands db (format "CREATE INDEX %s_et_i ON %s (entry_title)"
-                               table-name
-                               table-name))
+  (util/validate-table-name table-name)
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_crdi ON %s (concept_id, revision_id, deleted, delete_time)"
+                                  table-name
+                                  table-name))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_snv_i ON %s (short_name, version_id)"
+                                  table-name
+                                  table-name))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_eid_i ON %s (entry_id)"
+                                  table-name
+                                  table-name))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_et_i ON %s (entry_title)"
+                                  table-name
+                                  table-name))
   ;; Need this for transaction-id post commit check
-  (j/db-do-commands db (format "CREATE INDEX %s_crtid ON %s (concept_id, revision_id, transaction_id)"
-                               table-name
-                               table-name))
-  (j/db-do-commands db (format "CREATE INDEX %s_c_i ON %s (created_at)"
-                               table-name
-                               table-name)))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_crtid ON %s (concept_id, revision_id, transaction_id)"
+                                  table-name
+                                  table-name))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_c_i ON %s (created_at)"
+                                  table-name
+                                  table-name)))
 
 (defmethod create-collection-indexes true
   [db _ table-name]
-  (j/db-do-commands db (format "CREATE INDEX %s_crdi ON %s (concept_id, revision_id, deleted, delete_time)"
-                               table-name
-                               table-name))
-  (j/db-do-commands db (format "CREATE INDEX %s_p_s_i ON %s (provider_id, short_name, version_id)"
-                               table-name
-                               table-name))
-  (j/db-do-commands db (format "CREATE INDEX %s_p_ed_i ON %s (provider_id, entry_id)"
-                               table-name
-                               table-name))
-  (j/db-do-commands db (format "CREATE INDEX %s_p_et_i ON %s (provider_id, entry_title)"
-                               table-name
-                               table-name))
+  (util/validate-table-name table-name)
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_crdi ON %s (concept_id, revision_id, deleted, delete_time)"
+                                  table-name
+                                  table-name))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_p_s_i ON %s (provider_id, short_name, version_id)"
+                                  table-name
+                                  table-name))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_p_ed_i ON %s (provider_id, entry_id)"
+                                  table-name
+                                  table-name))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_p_et_i ON %s (provider_id, entry_title)"
+                                  table-name
+                                  table-name))
   ;; Need this for transaction-id post commit check
-  (j/db-do-commands db (format "CREATE INDEX %s_crtid ON %s (concept_id, revision_id, transaction_id)"
-                                table-name
-                                table-name))
-  (j/db-do-commands db (format "CREATE INDEX %s_c_i ON %s (created_at)"
-                               table-name)))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_crtid ON %s (concept_id, revision_id, transaction_id)"
+                                  table-name
+                                  table-name))
+  (jdbc/db-do-commands db (format "CREATE INDEX %s_c_i ON %s (created_at)"
+                                  table-name)))

metadata-db-app/src/cmr/metadata_db/data/oracle/providers.clj

@@ -51,23 +51,23 @@
       (ct/delete-provider-concept-tables db provider))
 
     ;; Delete the variable associations related to the provider via variable
-    (j/db-do-commands db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'VARIABLE-COLLECTION' and source_concept_identifier like 'V%-" provider-id "'"))
+    (j/db-do-prepared db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'VARIABLE-COLLECTION' and source_concept_identifier like ?") [(str "V%-" provider-id)])
     ;; Delete the variable associations related to the provider via collection
-    (j/db-do-commands db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'VARIABLE-COLLECTION' and associated_concept_id like 'C%-" provider-id "'"))
+    (j/db-do-prepared db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'VARIABLE-COLLECTION' and associated_concept_id like ?") [(str "C%-" provider-id)])
     ;; Delete variables of the provider
     (j/delete! db (ct/get-table-name provider :variable) ["provider_id = ?" provider-id])
 
     ;; Delete the service associations related to the provider via service
-    (j/db-do-commands db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'SERVICE-COLLECTION' and source_concept_identifier like 'S%-" provider-id "'"))
+    (j/db-do-prepared db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'SERVICE-COLLECTION' and source_concept_identifier like ?") [(str "S%-" provider-id)])
     ;; Delete the service associations related to the provider via collection
-    (j/db-do-commands db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'SERVICE-COLLECTION' and associated_concept_id like 'C%-" provider-id "'"))
+    (j/db-do-prepared db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'SERVICE-COLLECTION' and associated_concept_id like ?") [(str "C%-" provider-id)])
     ;; Delete services of the provider
     (j/delete! db (ct/get-table-name provider :service) ["provider_id = ?" provider-id])
 
     ;; Delete the tool associations related to the provider via tool
-    (j/db-do-commands db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'TOOL-COLLECTION' and source_concept_identifier like 'TL%-" provider-id "'"))
+    (j/db-do-prepared db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'TOOL-COLLECTION' and source_concept_identifier like ?") [(str "TL%-" provider-id)])
     ;; Delete the tool associations related to the provider via collection
-    (j/db-do-commands db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'TOOL-COLLECTION' and associated_concept_id like 'C%-" provider-id "'"))
+    (j/db-do-prepared db (str "DELETE FROM CMR_ASSOCIATIONS where association_type = 'TOOL-COLLECTION' and associated_concept_id like ?") [(str "C%-" provider-id)])
     ;; Delete tools of the provider
     (j/delete! db (ct/get-table-name provider :tool) ["provider_id = ?" provider-id])
 

metadata-db-app/src/cmr/metadata_db/data/util.clj

@@ -74,3 +74,9 @@
       (-> concept
           (assoc :metadata umm-json)
           (assoc :format (mt/format->mime-type :umm-json))))))
+
+(defn validate-table-name
+  "Checks if table name is valid, throws error if not valid"
+  [table-name]
+  (when-not (re-matches #"^[a-zA-Z0-9_]*$" table-name)
+    (throw (ex-info "Table name is not valid. Can only have alphanumeric and underscore chars." {:invalid-table table-name}))))

metadata-db-app/test/cmr/metadata_db/test/data/oracle/collection_table.clj

@@ -0,0 +1,92 @@
+(ns cmr.metadata-db.test.data.oracle.collection-table
+  (:require
+    [clojure.test :refer :all]
+    [cmr.metadata-db.data.oracle.collection-table :as ct]
+    [cmr.common.util :as util :refer [are3]]))
+
+(deftest collection-constraint-sql-false-test
+  (testing "valid table name"
+    (are3 [table-name query]
+        (let [non-small-provider {:provider-id "PROV1", :short-name "test provider", :cmr-only false, :small false}]
+          (is (= query (ct/collection-constraint-sql non-small-provider table-name))))
+
+        "valid table name"
+        "table_name"
+        "CONSTRAINT table_name_pk PRIMARY KEY (id), CONSTRAINT table_name_con_rev\n               UNIQUE (native_id, revision_id)\n               USING INDEX (create unique index table_name_ucr_i\n               ON table_name(native_id, revision_id)), CONSTRAINT table_name_cid_rev\n               UNIQUE (concept_id, revision_id)\n               USING INDEX (create unique index table_name_cri\n               ON table_name(concept_id, revision_id))"
+
+        "valid table name with numbers"
+        "table_123_valid"
+        "CONSTRAINT table_123_valid_pk PRIMARY KEY (id), CONSTRAINT table_123_valid_con_rev\n               UNIQUE (native_id, revision_id)\n               USING INDEX (create unique index table_123_valid_ucr_i\n               ON table_123_valid(native_id, revision_id)), CONSTRAINT table_123_valid_cid_rev\n               UNIQUE (concept_id, revision_id)\n               USING INDEX (create unique index table_123_valid_cri\n               ON table_123_valid(concept_id, revision_id))"))
+  (testing "invalid table name"
+    (are3 [table-name query]
+          (let [non-small-provider {:provider-id "PROV1", :short-name "test provider", :cmr-only false, :small false}]
+            (is (thrown? Exception (ct/collection-constraint-sql non-small-provider table-name))))
+
+          "invalid table name"
+          "table_name--;"
+          true
+
+          "invalid table name 2"
+          "table_; DELETE"
+          true)))
+
+(deftest collection-constraint-sql-true-test
+  (testing "valid table name"
+    (are3 [table-name query]
+          (let [small-provider {:provider-id "PROV1", :short-name "test provider", :cmr-only false, :small true}]
+            (is (= query (ct/collection-constraint-sql small-provider table-name))))
+
+          "valid table name"
+          "table_name"
+          "CONSTRAINT table_name_pk PRIMARY KEY (id), CONSTRAINT table_name_con_rev\n            UNIQUE (provider_id, native_id, revision_id)\n            USING INDEX (create unique index table_name_ucr_i\n            ON table_name(provider_id, native_id, revision_id)), CONSTRAINT table_name_cid_rev\n            UNIQUE (concept_id, revision_id)\n            USING INDEX (create unique index table_name_cri\n            ON table_name(concept_id, revision_id))",
+
+          "valid table name with numbers"
+          "table_123_valid"
+          "CONSTRAINT table_123_valid_pk PRIMARY KEY (id), CONSTRAINT table_123_valid_con_rev\n            UNIQUE (provider_id, native_id, revision_id)\n            USING INDEX (create unique index table_123_valid_ucr_i\n            ON table_123_valid(provider_id, native_id, revision_id)), CONSTRAINT table_123_valid_cid_rev\n            UNIQUE (concept_id, revision_id)\n            USING INDEX (create unique index table_123_valid_cri\n            ON table_123_valid(concept_id, revision_id))"))
+  (testing "invalid table name"
+    (are3 [table-name]
+          (let [small-provider {:provider-id "PROV1", :short-name "test provider", :cmr-only false, :small true}]
+            (is (thrown? Exception (ct/collection-constraint-sql small-provider table-name))))
+
+          "invalid table name"
+          "table_name--;"
+          true
+
+          "invalid table name 2"
+          "table_; DELETE"
+          true)))
+
+(deftest create-collection-indexes-false-test
+  (testing "invalid table name"
+    (let [non-small-provider {:provider-id "PROV1", :short-name "test provider", :cmr-only false, :small false}]
+      (are [table-name] (thrown? Exception (ct/create-collection-indexes nil non-small-provider table-name))
+                        "table_name--;"
+                        "table_; DELETE"))))
+
+(deftest create-collection-indexes-false-test
+  (testing "invalid table name"
+    (are3 [table-name]
+      (let [non-small-provider {:provider-id "PROV1", :short-name "test provider", :cmr-only false, :small false}]
+      (is (thrown? Exception (ct/create-collection-indexes nil non-small-provider table-name))))
+
+          "invalid table name"
+          "table_name--;"
+          true
+
+          "invalid table name 2"
+          "table_; DELETE"
+          true)))
+
+(deftest create-collection-indexes-true-test
+  (testing "invalid table name"
+    (are3 [table-name]
+    (let [small-provider {:provider-id "PROV1", :short-name "test provider", :cmr-only false, :small true}]
+      (is (thrown? Exception (ct/create-collection-indexes nil small-provider table-name))))
+
+          "invalid table name"
+          "table_name--;"
+          true
+
+          "invalid table name 2"
+          "table_; DELETE"
+          true)))

metadata-db-app/test/cmr/metadata_db/test/data/oracle/util.clj

@@ -0,0 +1,37 @@
+(ns cmr.metadata-db.test.data.oracle.util
+  (:require
+    [clojure.test :refer :all]
+    [cmr.metadata-db.data.util :as data_util]
+    [cmr.common.util :as common_util :refer [are3]]))
+
+(deftest validate-table-name-test
+  (testing "correct table name given"
+    (are3 [table-name result]
+          (is (= nil (data_util/validate-table-name table-name)))
+
+          "valid table name with only underscore"
+          "table_name"
+          true
+
+          "valid table name with numbers and underscores"
+          "123table_name"
+          true
+
+          "valid table name with multiple underscores"
+          "table_name_"
+          true))
+  (testing "incorrect table name given"
+    (are3 [table-name result]
+          (is (thrown? Exception (data_util/validate-table-name table-name)))
+
+          "invalid table name"
+          "table-name"
+          true
+
+          "invalid table name 2"
+          "; -- comment"
+          true
+
+          "invalid table name 3"
+          "; DELETE"
+          true)))