Merge pull request #9788 from thiggy1342/add-activerecord-annotate

RB: Add ActiveRecord::Relation#annotate to sqlFragmentArgument()

Author: hmac

ruby/ql/lib/change-notes/2022-07-18-sqli-in-activerecord-relation-annotate.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+
+- Calls to `ActiveRecord::Relation#annotate` are now recognized as`SqlExecution`s so that it will be considered as a sink for queries like rb/sql-injection.

ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll

@@ -133,6 +133,11 @@ private Expr sqlFragmentArgument(MethodCall call) {
       or
       methodName = "reload" and
       result = call.getKeywordArgument("lock")
+      or
+      // Calls to `annotate` can be used to add block comments to SQL queries. These are potentially vulnerable to
+      // SQLi if user supplied input is passed in as an argument.
+      methodName = "annotate" and
+      result = call.getArgument(_)
     )
   )
 }

ruby/ql/test/library-tests/frameworks/ActionController.expected

@@ -2,6 +2,7 @@ actionControllerControllerClasses
 | ActiveRecord.rb:23:1:39:3 | FooController |
 | ActiveRecord.rb:41:1:64:3 | BarController |
 | ActiveRecord.rb:66:1:70:3 | BazController |
+| ActiveRecord.rb:72:1:80:3 | AnnotatedController |
 | app/controllers/comments_controller.rb:1:1:7:3 | CommentsController |
 | app/controllers/foo/bars_controller.rb:3:1:39:3 | BarsController |
 | app/controllers/photos_controller.rb:1:1:4:3 | PhotosController |
@@ -12,6 +13,8 @@ actionControllerActionMethods
 | ActiveRecord.rb:42:3:47:5 | some_other_request_handler |
 | ActiveRecord.rb:49:3:63:5 | safe_paths |
 | ActiveRecord.rb:67:3:69:5 | yet_another_handler |
+| ActiveRecord.rb:73:3:75:5 | index |
+| ActiveRecord.rb:77:3:79:5 | unsafe_action |
 | app/controllers/comments_controller.rb:2:3:3:5 | index |
 | app/controllers/comments_controller.rb:5:3:6:5 | show |
 | app/controllers/foo/bars_controller.rb:5:3:7:5 | index |
@@ -38,6 +41,7 @@ paramsCalls
 | ActiveRecord.rb:59:12:59:17 | call to params |
 | ActiveRecord.rb:62:15:62:20 | call to params |
 | ActiveRecord.rb:68:21:68:26 | call to params |
+| ActiveRecord.rb:78:59:78:64 | call to params |
 | app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
 | app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
 | app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |
@@ -57,6 +61,7 @@ paramsSources
 | ActiveRecord.rb:59:12:59:17 | call to params |
 | ActiveRecord.rb:62:15:62:20 | call to params |
 | ActiveRecord.rb:68:21:68:26 | call to params |
+| ActiveRecord.rb:78:59:78:64 | call to params |
 | app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
 | app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
 | app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |

ruby/ql/test/library-tests/frameworks/ActiveRecord.expected

@@ -22,6 +22,7 @@ activeRecordSqlExecutionRanges
 | ActiveRecord.rb:46:20:46:32 | ... + ... |
 | ActiveRecord.rb:52:16:52:28 | "name #{...}" |
 | ActiveRecord.rb:56:20:56:39 | "username = #{...}" |
+| ActiveRecord.rb:78:27:78:76 | "this is an unsafe annotation:..." |
 activeRecordModelClassMethodCalls
 | ActiveRecord.rb:2:3:2:17 | call to has_many |
 | ActiveRecord.rb:6:3:6:24 | call to belongs_to |
@@ -44,6 +45,8 @@ activeRecordModelClassMethodCalls
 | ActiveRecord.rb:60:5:60:33 | call to find_by |
 | ActiveRecord.rb:62:5:62:34 | call to find |
 | ActiveRecord.rb:68:5:68:45 | call to delete_by |
+| ActiveRecord.rb:74:13:74:54 | call to annotate |
+| ActiveRecord.rb:78:13:78:77 | call to annotate |
 potentiallyUnsafeSqlExecutingMethodCall
 | ActiveRecord.rb:9:5:9:68 | call to find |
 | ActiveRecord.rb:19:5:19:25 | call to destroy_by |
@@ -55,6 +58,7 @@ potentiallyUnsafeSqlExecutingMethodCall
 | ActiveRecord.rb:46:5:46:33 | call to delete_by |
 | ActiveRecord.rb:52:5:52:29 | call to order |
 | ActiveRecord.rb:56:7:56:40 | call to find_by |
+| ActiveRecord.rb:78:13:78:77 | call to annotate |
 activeRecordModelInstantiations
 | ActiveRecord.rb:9:5:9:68 | call to find | ActiveRecord.rb:5:1:15:3 | User |
 | ActiveRecord.rb:13:5:13:40 | call to find_by | ActiveRecord.rb:1:1:3:3 | UserGroup |

ruby/ql/test/library-tests/frameworks/ActiveRecord.rb

@@ -68,3 +68,13 @@ def yet_another_handler
     Admin.delete_by(params[:admin_condition])
   end
 end
+
+class AnnotatedController < ActionController::Base
+  def index
+    users = User.annotate("this is a safe annotation")
+  end
+
+  def unsafe_action
+    users = User.annotate("this is an unsafe annotation:#{params[:comment]}")
+  end
+end

ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb

@@ -137,3 +137,17 @@ def yet_another_handler
     Admin.delete_by(params[:admin_condition])
   end
 end
+
+class AnnotatedController < ActionController::Base
+  def index
+    name = params[:user_name]
+    # GOOD: string literal arguments not controlled by user are safe for annotations
+    users = User.annotate("this is a safe annotation").find_by(user_name: name)
+  end
+
+  def unsafe_action
+    name = params[:user_name]
+    # BAD: user input passed into annotations are vulnerable to SQLi
+    users = User.annotate("this is an unsafe annotation:#{params[:comment]}").find_by(user_name: name)
+  end
+end

ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected

@@ -31,6 +31,8 @@ edges
 | ActiveRecordInjection.rb:99:11:99:17 | ...[...] :  | ActiveRecordInjection.rb:104:20:104:32 | ... + ... |
 | ActiveRecordInjection.rb:137:21:137:26 | call to params :  | ActiveRecordInjection.rb:137:21:137:44 | ...[...] :  |
 | ActiveRecordInjection.rb:137:21:137:44 | ...[...] :  | ActiveRecordInjection.rb:20:22:20:30 | condition :  |
+| ActiveRecordInjection.rb:151:59:151:64 | call to params :  | ActiveRecordInjection.rb:151:59:151:74 | ...[...] :  |
+| ActiveRecordInjection.rb:151:59:151:74 | ...[...] :  | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." |
 nodes
 | ActiveRecordInjection.rb:8:25:8:28 | name :  | semmle.label | name :  |
 | ActiveRecordInjection.rb:8:31:8:34 | pass :  | semmle.label | pass :  |
@@ -80,6 +82,9 @@ nodes
 | ActiveRecordInjection.rb:104:20:104:32 | ... + ... | semmle.label | ... + ... |
 | ActiveRecordInjection.rb:137:21:137:26 | call to params :  | semmle.label | call to params :  |
 | ActiveRecordInjection.rb:137:21:137:44 | ...[...] :  | semmle.label | ...[...] :  |
+| ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | semmle.label | "this is an unsafe annotation:..." |
+| ActiveRecordInjection.rb:151:59:151:64 | call to params :  | semmle.label | call to params :  |
+| ActiveRecordInjection.rb:151:59:151:74 | ...[...] :  | semmle.label | ...[...] :  |
 subpaths
 #select
 | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:70:23:70:28 | call to params :  | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:70:23:70:28 | call to params | a user-provided value |
@@ -99,3 +104,4 @@ subpaths
 | ActiveRecordInjection.rb:88:18:88:35 | ...[...] | ActiveRecordInjection.rb:88:18:88:23 | call to params :  | ActiveRecordInjection.rb:88:18:88:35 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:88:18:88:23 | call to params | a user-provided value |
 | ActiveRecordInjection.rb:92:21:92:35 | ...[...] | ActiveRecordInjection.rb:92:21:92:26 | call to params :  | ActiveRecordInjection.rb:92:21:92:35 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:92:21:92:26 | call to params | a user-provided value |
 | ActiveRecordInjection.rb:104:20:104:32 | ... + ... | ActiveRecordInjection.rb:98:10:98:15 | call to params :  | ActiveRecordInjection.rb:104:20:104:32 | ... + ... | This SQL query depends on $@. | ActiveRecordInjection.rb:98:10:98:15 | call to params | a user-provided value |
+| ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | ActiveRecordInjection.rb:151:59:151:64 | call to params :  | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | This SQL query depends on $@. | ActiveRecordInjection.rb:151:59:151:64 | call to params | a user-provided value |