Merge pull request #11058 from hmac/actioncontroller-logger

Ruby: Model various ActionController methods

Author: hmac

ruby/ql/lib/change-notes/2022-11-01-actioncontroller-logger.md

@@ -0,0 +1,6 @@
+---
+category: minorAnalysis
+---
+* Calls to `logger` in `ActiveSupport` actions are now recognised as logger instances.
+* Calls to `send_data` in `ActiveSupport` actions are recognised as HTTP responses.
+* Calls to `body_stream` in `ActiveSupport` actions are recognised as HTTP request accesses.

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

@@ -12,6 +12,7 @@ private import codeql.ruby.frameworks.ActionDispatch
 private import codeql.ruby.frameworks.ActionView
 private import codeql.ruby.frameworks.Rails
 private import codeql.ruby.frameworks.internal.Rails
+private import codeql.ruby.dataflow.internal.DataFlowDispatch
 
 /**
  * DEPRECATED: Import `codeql.ruby.frameworks.Rails` and use `Rails::ParamsCall` instead.
@@ -295,7 +296,7 @@ private module Request {
 
   /** A method call on `request` which returns the request body. */
   private class BodyCall extends RequestInputAccess {
-    BodyCall() { this.getMethodName() = ["body", "raw_post"] }
+    BodyCall() { this.getMethodName() = ["body", "raw_post", "body_stream"] }
 
     override Http::Server::RequestInputKind getKind() { result = Http::Server::bodyInputKind() }
   }
@@ -538,12 +539,34 @@ private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetti
 /**
  * A call to `send_file`, which sends the file at the given path to the client.
  */
-private class SendFile extends FileSystemAccess::Range, DataFlow::CallNode {
+private class SendFile extends FileSystemAccess::Range, Http::Server::HttpResponse::Range,
+  DataFlow::CallNode {
   SendFile() {
     this = [actionControllerInstance(), Response::response()].getAMethodCall("send_file")
   }
 
   override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
+
+  override DataFlow::Node getBody() { result = this.getArgument(0) }
+
+  override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+  override string getMimetypeDefault() { result = "application/octet-stream" }
+}
+
+/**
+ * A call to `send_data`, which sends the given data to the client.
+ */
+class SendDataCall extends DataFlow::CallNode, Http::Server::HttpResponse::Range {
+  SendDataCall() {
+    this = [actionControllerInstance(), Response::response()].getAMethodCall("send_data")
+  }
+
+  override DataFlow::Node getBody() { result = this.getArgument(0) }
+
+  override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+  override string getMimetypeDefault() { result = "application/octet-stream" }
 }
 
 private module ParamsSummaries {
@@ -733,3 +756,28 @@ private module Response {
     override DataFlow::Node getValue() { result = this.getArgument(0) }
   }
 }
+
+private class ActionControllerLoggerInstance extends DataFlow::Node {
+  ActionControllerLoggerInstance() {
+    this = actionControllerInstance().getAMethodCall("logger")
+    or
+    any(ActionControllerLoggerInstance i).(DataFlow::LocalSourceNode).flowsTo(this)
+  }
+}
+
+private class ActionControllerLoggingCall extends DataFlow::CallNode, Logging::Range {
+  ActionControllerLoggingCall() {
+    this.getReceiver() instanceof ActionControllerLoggerInstance and
+    this.getMethodName() = ["debug", "error", "fatal", "info", "unknown", "warn"]
+  }
+
+  // Note: this is identical to the definition `stdlib.Logger.LoggerInfoStyleCall`.
+  override DataFlow::Node getAnInput() {
+    // `msg` from `Logger#info(msg)`,
+    // or `progname` from `Logger#info(progname) <block>`
+    result = this.getArgument(0)
+    or
+    // a return value from the block in `Logger#info(progname) <block>`
+    exprNodeReturnedFrom(result, this.getBlock().asExpr().getExpr())
+  }
+}

ruby/ql/lib/codeql/ruby/security/LogInjectionQuery.qll

@@ -60,6 +60,14 @@ class StringReplaceSanitizer extends Sanitizer {
   }
 }
 
+/**
+ * A call to `Object#inspect`, considered as a sanitizer.
+ * This is because `inspect` will replace newlines in strings with `\n`.
+ */
+class InspectSanitizer extends Sanitizer {
+  InspectSanitizer() { this.(DataFlow::CallNode).getMethodName() = "inspect" }
+}
+
 /**
  * A call to an HTML escape method is considered to sanitize its input.
  */

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

@@ -34,15 +34,26 @@ actionDispatchRoutes
 | app/config/routes.rb:49:5:49:95 | call to delete | delete | users/:user/notifications | users/notifications | destroy |
 | app/config/routes.rb:50:5:50:94 | call to post | post | users/:user/notifications/:notification_id/mark_as_read | users/notifications | mark_as_read |
 actionDispatchControllerMethods
+| app/config/routes.rb:2:3:8:5 | call to resources | action_controller/controllers/posts_controller.rb:2:3:3:5 | index |
+| app/config/routes.rb:2:3:8:5 | call to resources | action_controller/controllers/posts_controller.rb:5:3:6:5 | show |
 | app/config/routes.rb:2:3:8:5 | call to resources | app/controllers/posts_controller.rb:2:3:3:5 | index |
 | app/config/routes.rb:2:3:8:5 | call to resources | app/controllers/posts_controller.rb:5:3:6:5 | show |
+| app/config/routes.rb:3:5:6:7 | call to resources | action_controller/controllers/comments_controller.rb:2:3:36:5 | index |
+| app/config/routes.rb:3:5:6:7 | call to resources | action_controller/controllers/comments_controller.rb:38:3:44:5 | show |
+| app/config/routes.rb:3:5:6:7 | call to resources | action_controller/controllers/comments_controller.rb:50:3:52:5 | destroy |
 | app/config/routes.rb:3:5:6:7 | call to resources | app/controllers/comments_controller.rb:2:3:36:5 | index |
 | app/config/routes.rb:3:5:6:7 | call to resources | app/controllers/comments_controller.rb:38:3:39:5 | show |
+| app/config/routes.rb:7:5:7:37 | call to post | action_controller/controllers/posts_controller.rb:8:3:9:5 | upvote |
 | app/config/routes.rb:7:5:7:37 | call to post | app/controllers/posts_controller.rb:8:3:9:5 | upvote |
+| app/config/routes.rb:27:3:27:48 | call to match | action_controller/controllers/photos_controller.rb:2:3:3:5 | show |
 | app/config/routes.rb:27:3:27:48 | call to match | app/controllers/photos_controller.rb:2:3:3:5 | show |
+| app/config/routes.rb:28:3:28:50 | call to match | action_controller/controllers/photos_controller.rb:2:3:3:5 | show |
 | app/config/routes.rb:28:3:28:50 | call to match | app/controllers/photos_controller.rb:2:3:3:5 | show |
+| app/config/routes.rb:29:3:29:69 | call to match | action_controller/controllers/photos_controller.rb:2:3:3:5 | show |
 | app/config/routes.rb:29:3:29:69 | call to match | app/controllers/photos_controller.rb:2:3:3:5 | show |
+| app/config/routes.rb:30:3:30:50 | call to match | action_controller/controllers/photos_controller.rb:2:3:3:5 | show |
 | app/config/routes.rb:30:3:30:50 | call to match | app/controllers/photos_controller.rb:2:3:3:5 | show |
+| app/config/routes.rb:50:5:50:94 | call to post | action_controller/controllers/users/notifications_controller.rb:3:5:4:7 | mark_as_read |
 | app/config/routes.rb:50:5:50:94 | call to post | app/controllers/users/notifications_controller.rb:3:5:4:7 | mark_as_read |
 underscore
 | Foo | foo |

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

@@ -9,18 +9,35 @@ rawCalls
 | app/views/foo/bars/show.html.erb:5:5:5:21 | call to raw |
 | app/views/foo/bars/show.html.erb:7:5:7:19 | call to raw |
 renderCalls
+| action_controller/controllers/comments_controller.rb:42:21:42:64 | call to render |
+| action_controller/controllers/foo/bars_controller.rb:6:5:6:37 | call to render |
+| action_controller/controllers/foo/bars_controller.rb:23:5:23:76 | call to render |
+| action_controller/controllers/foo/bars_controller.rb:35:5:35:33 | call to render |
+| action_controller/controllers/foo/bars_controller.rb:38:5:38:50 | call to render |
+| action_controller/controllers/foo/bars_controller.rb:44:5:44:17 | call to render |
 | app/controllers/foo/bars_controller.rb:6:5:6:37 | call to render |
 | app/controllers/foo/bars_controller.rb:23:5:23:76 | call to render |
 | app/controllers/foo/bars_controller.rb:35:5:35:33 | call to render |
 | app/controllers/foo/bars_controller.rb:38:5:38:50 | call to render |
 | app/controllers/foo/bars_controller.rb:44:5:44:17 | call to render |
 | app/views/foo/bars/show.html.erb:31:5:31:89 | call to render |
 renderToCalls
+| action_controller/controllers/foo/bars_controller.rb:15:16:15:97 | call to render_to_string |
+| action_controller/controllers/foo/bars_controller.rb:36:12:36:67 | call to render_to_string |
 | app/controllers/foo/bars_controller.rb:15:16:15:97 | call to render_to_string |
 | app/controllers/foo/bars_controller.rb:36:12:36:67 | call to render_to_string |
 linkToCalls
 | app/views/foo/bars/show.html.erb:33:5:33:41 | call to link_to |
 httpResponses
+| action_controller/controllers/comments_controller.rb:11:5:11:17 | call to body= | action_controller/controllers/comments_controller.rb:11:21:11:34 | ... = ... | text/http |
+| action_controller/controllers/comments_controller.rb:21:5:21:37 | call to send_file | action_controller/controllers/comments_controller.rb:21:24:21:36 | "my-file.ext" | application/octet-stream |
+| action_controller/controllers/comments_controller.rb:47:5:47:20 | call to send_data | action_controller/controllers/comments_controller.rb:47:15:47:20 | @photo | application/octet-stream |
+| action_controller/controllers/foo/bars_controller.rb:15:16:15:97 | call to render_to_string | action_controller/controllers/foo/bars_controller.rb:15:33:15:47 | "foo/bars/show" | text/html |
+| action_controller/controllers/foo/bars_controller.rb:23:5:23:76 | call to render | action_controller/controllers/foo/bars_controller.rb:23:12:23:26 | "foo/bars/show" | text/html |
+| action_controller/controllers/foo/bars_controller.rb:35:5:35:33 | call to render | action_controller/controllers/foo/bars_controller.rb:35:18:35:33 | call to [] | application/json |
+| action_controller/controllers/foo/bars_controller.rb:36:12:36:67 | call to render_to_string | action_controller/controllers/foo/bars_controller.rb:36:29:36:33 | @user | application/json |
+| action_controller/controllers/foo/bars_controller.rb:38:5:38:50 | call to render | action_controller/controllers/foo/bars_controller.rb:38:12:38:22 | call to backtrace | text/plain |
+| action_controller/controllers/foo/bars_controller.rb:44:5:44:17 | call to render | action_controller/controllers/foo/bars_controller.rb:44:12:44:17 | "show" | text/html |
 | app/controllers/comments_controller.rb:11:5:11:17 | call to body= | app/controllers/comments_controller.rb:11:21:11:34 | ... = ... | text/http |
 | app/controllers/comments_controller.rb:21:5:21:37 | call to send_file | app/controllers/comments_controller.rb:21:24:21:36 | "my-file.ext" | application/octet-stream |
 | app/controllers/foo/bars_controller.rb:15:16:15:97 | call to render_to_string | app/controllers/foo/bars_controller.rb:15:33:15:47 | "foo/bars/show" | text/html |