Ruby: split out rb/sensitive-get-query using query/customizations pattern

Author: alexrford

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

@@ -0,0 +1,53 @@
+/**
+ * Provides default sources and sinks for reasoning about sensitive data sourced
+ * from the query string of a GET request, as well as extension points for
+ * adding your own.
+ */
+
+private import codeql.ruby.security.SensitiveActions
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+
+/**
+ * Provides default sources and sinks for reasoning about sensitive data sourced
+ * from the query string of a GET request, as well as extension points for
+ * adding your own.
+ */
+module SensitiveGetQuery {
+  /**
+   * A data flow source representing data sourced from the query string in a
+   * GET request handler.
+   */
+  abstract class Source extends DataFlow::Node {
+    /** Gets the request handler corresponding to this data source. */
+    abstract Http::Server::RequestHandler getHandler();
+  }
+
+  /**
+   * An access to data from the query string of a GET request as a data flow
+   * source.
+   */
+  private class RequestInputAccessSource extends Source instanceof Http::Server::RequestInputAccess {
+    private Http::Server::RequestHandler handler;
+
+    RequestInputAccessSource() {
+      handler = this.asExpr().getExpr().getEnclosingMethod() and
+      handler.getAnHttpMethod() = "get"
+    }
+
+    override Http::Server::RequestHandler getHandler() { result = handler }
+  }
+
+  /**
+   * A data flow sink suggesting a use of sensitive data.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /** A sensitive data node as a data flow sink. */
+  private class SensitiveNodeSink extends Sink instanceof SensitiveNode {
+    SensitiveNodeSink() {
+      // User names and other similar information is not sensitive in this context.
+      not this.getClassification() = SensitiveDataClassification::id()
+    }
+  }
+}

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

@@ -0,0 +1,32 @@
+/**
+ * Provides a taint-tracking configuration for detecting flow of query string
+ * data to sensitive actions in GET query request handlers.
+ *
+ * Note, for performance reasons: only import this file if `Configuration` is
+ * needed, otherwise `SensitiveGetQueryCustomizations` should be imported
+ * instead.
+ */
+
+private import ruby
+private import codeql.ruby.DataFlow
+private import codeql.ruby.TaintTracking
+
+/**
+ * Provides a taint-tracking configuration for detecting flow of query string
+ * data to sensitive actions in GET query request handlers.
+ */
+module SensitiveGetQuery {
+  import SensitiveGetQueryCustomizations::SensitiveGetQuery
+
+  /**
+   * A taint-tracking configuration for reasoning about use of sensitive data
+   * from a GET request query string.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "SensitiveGetQuery" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+  }
+}

ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql

@@ -12,37 +12,13 @@
  */
 
 import ruby
-private import codeql.ruby.DataFlow
-private import codeql.ruby.TaintTracking
-private import codeql.ruby.security.SensitiveActions
-private import codeql.ruby.Concepts
-private import codeql.ruby.frameworks.ActionDispatch
-private import codeql.ruby.frameworks.ActionController
-private import codeql.ruby.frameworks.core.Array
+import DataFlow::PathGraph
+import codeql.ruby.DataFlow
+import codeql.ruby.security.SensitiveGetQueryQuery
+import codeql.ruby.security.SensitiveActions
 
-class Source extends Http::Server::RequestInputAccess {
-  private Http::Server::RequestHandler handler;
-
-  Source() {
-    handler = this.asExpr().getExpr().getEnclosingMethod() and
-    handler.getAnHttpMethod() = "get"
-  }
-
-  Http::Server::RequestHandler getHandler() { result = handler }
-}
-
-class Configuration extends TaintTracking::Configuration {
-  Configuration() { this = "SensitiveGetQuery" }
-
-  override predicate isSource(DataFlow::Node source) { source instanceof Source }
-
-  override predicate isSink(DataFlow::Node sink) { sink instanceof SensitiveNode }
-}
-
-from DataFlow::PathNode source, DataFlow::PathNode sink, Configuration config
-where
-  config.hasFlowPath(source, sink) and
-  not sink.getNode().(SensitiveNode).getClassification() = SensitiveDataClassification::id()
+from DataFlow::PathNode source, DataFlow::PathNode sink, SensitiveGetQuery::Configuration config
+where config.hasFlowPath(source, sink)
 select source.getNode(), source, sink,
   "$@ for GET requests uses query parameter as sensitive data.",
-  source.getNode().(Source).getHandler(), "Route handler"
+  source.getNode().(SensitiveGetQuery::Source).getHandler(), "Route handler"

ruby/ql/test/query-tests/security/cwe-598/SensitiveGetQuery.expected

@@ -1,2 +1,13 @@
+edges
+| app/controllers/users_controller.rb:4:16:4:21 | call to params :  | app/controllers/users_controller.rb:4:16:4:32 | ...[...] |
+| app/controllers/users_controller.rb:4:16:4:21 | call to params :  | app/controllers/users_controller.rb:4:16:4:32 | ...[...] :  |
+| app/controllers/users_controller.rb:4:16:4:32 | ...[...] :  | app/controllers/users_controller.rb:5:42:5:49 | password |
+nodes
+| app/controllers/users_controller.rb:4:16:4:21 | call to params :  | semmle.label | call to params :  |
+| app/controllers/users_controller.rb:4:16:4:32 | ...[...] | semmle.label | ...[...] |
+| app/controllers/users_controller.rb:4:16:4:32 | ...[...] :  | semmle.label | ...[...] :  |
+| app/controllers/users_controller.rb:5:42:5:49 | password | semmle.label | password |
+subpaths
+#select
 | app/controllers/users_controller.rb:4:16:4:21 | call to params | app/controllers/users_controller.rb:4:16:4:21 | call to params :  | app/controllers/users_controller.rb:4:16:4:32 | ...[...] | $@ for GET requests uses query parameter as sensitive data. | app/controllers/users_controller.rb:3:3:6:5 | login_get | Route handler |
 | app/controllers/users_controller.rb:4:16:4:21 | call to params | app/controllers/users_controller.rb:4:16:4:21 | call to params :  | app/controllers/users_controller.rb:5:42:5:49 | password | $@ for GET requests uses query parameter as sensitive data. | app/controllers/users_controller.rb:3:3:6:5 | login_get | Route handler |