fix(ruby): tighten sql injection patterns (#465)

Bearer · Oct 7, 2024 · b6a88ee · b6a88ee
1 parent fee8e3d
commit b6a88ee
Show file tree

Hide file tree

Showing 4 changed files with 116 additions and 76 deletions.
diff --git a/rules/ruby/rails/sql_injection.yml b/rules/ruby/rails/sql_injection.yml
@@ -1,112 +1,126 @@
 imports:
   - ruby_shared_common_external_input
 patterns:
-  - pattern: $<_>.$<METHOD>($<...>$<USER_INPUT>$<...>)
-    filters:
-      - variable: METHOD
-        detection: ruby_rails_sql_injection_regular_method
-        scope: cursor
-      - variable: USER_INPUT
-        detection: ruby_rails_sql_injection_external_input
-        scope: result
-  - pattern: $<METHOD>($<USER_INPUT>$<...>)
-    filters:
-      - variable: METHOD
-        detection: ruby_rails_sql_injection_regular_method
-        scope: cursor
-      - variable: USER_INPUT
-        detection: ruby_rails_sql_injection_external_input
-        scope: result
-  - pattern: ActiveRecord::Base.connection.$<CONN_METHOD>($<USER_INPUT>$<...>)
+  - pattern: ActiveRecord::Base.connection.$<CONN_METHOD>($<EXTERNAL_INPUT>$<...>)
     filters:
       - variable: CONN_METHOD
         detection: ruby_rails_sql_injection_connection_method
         scope: cursor
-      - variable: USER_INPUT
+      - variable: EXTERNAL_INPUT
         detection: ruby_rails_sql_injection_external_input
         scope: result
-  - pattern: connection.$<CONN_METHOD>($<USER_INPUT>$<...>)
+  - pattern: connection.$<CONN_METHOD>($<EXTERNAL_INPUT>$<...>)
     filters:
       - variable: CONN_METHOD
         detection: ruby_rails_sql_injection_connection_method
         scope: cursor
-      - variable: USER_INPUT
+      - variable: EXTERNAL_INPUT
         detection: ruby_rails_sql_injection_external_input
         scope: result
-  - pattern: $<_>.$<SPECIAL_METHOD>($<USER_INPUT>$<...>)
+  - pattern: $<_>.$<SPECIAL_METHOD>($<EXTERNAL_INPUT>$<...>)
     filters:
       - variable: SPECIAL_METHOD
         detection: ruby_rails_sql_injection_special_arg_method
         scope: cursor
-      - variable: USER_INPUT
+      - variable: EXTERNAL_INPUT
         detection: ruby_rails_sql_injection_external_input
         scope: result
       - not:
-          variable: USER_INPUT
+          variable: EXTERNAL_INPUT
           detection: ruby_rails_sql_injection_safe_special_arg
-  - pattern: $<SPECIAL_METHOD>($<USER_INPUT>$<...>)
+  - pattern: $<SPECIAL_METHOD>($<EXTERNAL_INPUT>$<...>)
     filters:
       - variable: SPECIAL_METHOD
         detection: ruby_rails_sql_injection_special_arg_method
         scope: cursor
-      - variable: USER_INPUT
+      - variable: EXTERNAL_INPUT
         detection: ruby_rails_sql_injection_external_input
         scope: result
       - not:
-          variable: USER_INPUT
+          variable: EXTERNAL_INPUT
           detection: ruby_rails_sql_injection_safe_special_arg
+  - pattern: $<_>.$<METHOD>($<...>$<EXTERNAL_INPUT>$<...>)
+    filters:
+      - variable: METHOD
+        detection: ruby_rails_sql_injection_regular_method
+        scope: cursor
+      - variable: EXTERNAL_INPUT
+        detection: ruby_rails_sql_injection_external_input
+        scope: result
+  - pattern: $<METHOD>($<...>$<EXTERNAL_INPUT>$<...>)
+    filters:
+      - variable: METHOD
+        detection: ruby_rails_sql_injection_regular_method
+        scope: cursor
+      - variable: EXTERNAL_INPUT
+        detection: ruby_rails_sql_injection_external_input
+        scope: result
+  - pattern: $<ACTIVE_RECORD>.$<SHARED_METHOD>($<...>$<EXTERNAL_INPUT>$<...>)
+    filters:
+      - variable: ACTIVE_RECORD
+        detection: ruby_rails_sql_injection_active_record
+        scope: cursor
+      - variable: SHARED_METHOD
+        detection: ruby_rails_sql_injection_shared_method
+        scope: cursor
+      - variable: EXTERNAL_INPUT
+        detection: ruby_rails_sql_injection_external_input
+        scope: result
 auxiliary:
   - id: ruby_rails_sql_injection_regular_method
     patterns:
-      - pattern: $<METHOD>
-        filters:
-          - variable: METHOD
-            values:
-              - joins
-              - select
-              - reselect
-              - group
-              - having
-              - order
-              - reorder
-              - minimum
-              - maximum
-              - calculate
-              - count
-              - count_by_sql
-              - sum
-              - average
+      - joins
+      - group
+      - having
+      - order
+      - reorder
+      - minimum
+      - maximum
+      - count_by_sql
+      - average
+  - id: ruby_rails_sql_injection_shared_method
+    patterns:
+      - count
+      - select
+      - sum
   - id: ruby_rails_sql_injection_connection_method
     patterns:
-      - pattern: $<CONN_METHOD>
-        filters:
-          - variable: CONN_METHOD
-            values:
-              - execute
-              - exec_delete
-              - exec_insert
-              - exec_query
-              - exec_update
-              - select_all
-              - select_one
-              - select_rows
-              - select_value
-              - select_values
+      - execute
+      - exec_delete
+      - exec_insert
+      - exec_query
+      - exec_update
+      - select_all
+      - select_one
+      - select_rows
+      - select_value
+      - select_values
   - id: ruby_rails_sql_injection_special_arg_method
     patterns:
-      - pattern: $<SPECIAL_METHOD>
+      - find_by
+      - find_by!
+      - find_by_sql
+      - find_sole_by
+      - from
+      - where
+      - delete_by
+      - destroy_by
+      - update_all
+  - id: ruby_rails_sql_injection_active_record
+    patterns:
+      - pattern: $<ACTIVE_RECORD>.$<_>($<...>)
         filters:
-          - variable: SPECIAL_METHOD
-            values:
-              - find_by
-              - find_by!
-              - find_by_sql
-              - find_sole_by
-              - from
-              - where
-              - delete_by
-              - destroy_by
-              - update_all
+          - variable: ACTIVE_RECORD
+            detection: ruby_rails_sql_injection_active_record
+            scope: cursor
+      - pattern: $<MODEL_NAME>
+        filters:
+          - variable: MODEL_NAME
+            regex: \A[A-Z][a-zA-Z]*[a-z]+\z
+      - pattern: $<_>.$<ASSOCIATION>
+        filters:
+          - variable: ASSOCIATION
+            regex: \A\w*_?s\z
   - id: ruby_rails_sql_injection_external_input
     sanitizer: ruby_rails_sql_injection_sanitized
     patterns:

diff --git a/tests/ruby/rails/sql_injection/test.js b/tests/ruby/rails/sql_injection/test.js
@@ -7,7 +7,6 @@ const { ruleId, ruleFile, testBase } = getEnvironment(__dirname)
 describe(ruleId, () => {
   const invoke = createNewInvoker(ruleId, ruleFile, testBase)
 
-
     test("injected_params", () => {
       const testCase = "injected_params.rb"
 
@@ -16,7 +15,7 @@ describe(ruleId, () => {
       expect(results.Missing).toEqual([])
       expect(results.Extra).toEqual([])
     })
-  
+
 
     test("ok_sanitized", () => {
       const testCase = "ok_sanitized.rb"
@@ -26,7 +25,7 @@ describe(ruleId, () => {
       expect(results.Missing).toEqual([])
       expect(results.Extra).toEqual([])
     })
-  
+
 
     test("ok_using_bind", () => {
       const testCase = "ok_using_bind.rb"
@@ -36,5 +35,14 @@ describe(ruleId, () => {
       expect(results.Missing).toEqual([])
       expect(results.Extra).toEqual([])
     })
-
+
+    test("shared methods", () => {
+      const testCase = "shared_methods.rb"
+
+      const results = invoke(testCase)
+
+      expect(results.Missing).toEqual([])
+      expect(results.Extra).toEqual([])
+    })
+
 })
diff --git a/tests/ruby/rails/sql_injection/testdata/injected_params.rb b/tests/ruby/rails/sql_injection/testdata/injected_params.rb
@@ -4,8 +4,6 @@
 find_by!("oops #{params[:oops]}")
 # bearer:expected ruby_rails_sql_injection
 User.joins("INNER JOIN #{params[:oops]}")
-# bearer:expected ruby_rails_sql_injection
-select("#{params[:oops]} AS oops")
 
 # chained case
 # bearer:expected ruby_rails_sql_injection

diff --git a/tests/ruby/rails/sql_injection/testdata/shared_methods.rb b/tests/ruby/rails/sql_injection/testdata/shared_methods.rb
@@ -0,0 +1,20 @@
+
+# bearer:expected ruby_rails_sql_injection
+User.where(username: "mish").count(params[:col])
+
+# bearer:expected ruby_rails_sql_injection
+user.posts.count(params[:col])
+
+# safe uses of shared method
+my_arr = [1, 2, 3, 2]
+# ok
+my_arr.count(params[:item])
+
+ITEMS = [:apple, :carrot, "orange"]
+ITEMS.count
+ITEMS.count(:apple)
+
+# bearer:expected ruby_rails_sql_injection
+User.select("#{params[:oops]} AS oops")
+# bearer:expected ruby_rails_sql_injection
+user.posts.select("#{params[:oops]} AS oops")