Skip to content

Commit

Permalink
CMR 9430: Fix SQL Injection Vulnerabilities in CMR Metadata App in pr…
Browse files Browse the repository at this point in the history
…ovider and collection files (#1969)

* fix provider ns

* add validate func and unit tests

* adding collection table unit tests

* exchange are for are3

* update import name jdbc
  • Loading branch information
jmaeng72 authored Sep 22, 2023
1 parent 36376e1 commit 9b97b32
Show file tree
Hide file tree
Showing 5 changed files with 159 additions and 19 deletions.
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)"
(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))
(j/db-do-commands db (format "CREATE INDEX %s_snv_i ON %s (short_name, version_id)"
(jdbc/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)"
(jdbc/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)"
(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)"
(jdbc/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)"
(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)"
(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))
(j/db-do-commands db (format "CREATE INDEX %s_p_s_i ON %s (provider_id, short_name, version_id)"
(jdbc/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)"
(jdbc/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)"
(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)"
(jdbc/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)"
(jdbc/db-do-commands db (format "CREATE INDEX %s_c_i ON %s (created_at)"
table-name)))
12 changes: 6 additions & 6 deletions metadata-db-app/src/cmr/metadata_db/data/oracle/providers.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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])

Expand Down
6 changes: 6 additions & 0 deletions metadata-db-app/src/cmr/metadata_db/data/util.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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}))))
Original file line number Diff line number Diff line change
@@ -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)))
37 changes: 37 additions & 0 deletions metadata-db-app/test/cmr/metadata_db/test/data/oracle/util.clj
Original file line number Diff line number Diff line change
@@ -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)))

0 comments on commit 9b97b32

Please sign in to comment.