Fix performance issues related to a x-product between ActiveRecordModelInstantiation and MethodCall

Author: alexrford

ql/lib/codeql/ruby/Concepts.qll

@@ -546,8 +546,9 @@ module XmlParserCall {
  * extend `OrmInstantiation::Range` instead.
  */
 class OrmInstantiation extends DataFlow::Node instanceof OrmInstantiation::Range {
-  /** Holds if `call` may return a field of this ORM object. */
-  predicate methodCallMayAccessField(MethodCall call) { super.methodCallMayAccessField(call) }
+  /** Holds if a call to `methodName` on this instance may return a field of this ORM object. */
+  bindingset[methodName]
+  predicate methodCallMayAccessField(string methodName) { super.methodCallMayAccessField(methodName) }
 }
 
 /** Provides a class for modeling new ORM object instantiation APIs. */
@@ -559,7 +560,8 @@ module OrmInstantiation {
    * extend `OrmInstantiation` instead.
    */
   abstract class Range extends DataFlow::Node {
-    /** Holds if `call` may return a field of this ORM object. */
-    abstract predicate methodCallMayAccessField(MethodCall call);
+    /** Holds if a call to `methodName` on this instance may return a field of this ORM object. */
+    bindingset[methodName]
+    abstract predicate methodCallMayAccessField(string methodName);
   }
 }

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

@@ -21,6 +21,25 @@ private class ApplicationRecordAccess extends ConstantReadAccess {
   ApplicationRecordAccess() { this.getName() = "ApplicationRecord" }
 }
 
+/// See https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html
+private string activeRecordPersistenceInstanceMethodName() {
+  result =
+    [
+      "becomes", "becomes!", "decrement", "decrement!", "delete", "delete!", "destroy", "destroy!",
+      "destroyed?", "increment", "increment!", "new_record?", "persisted?",
+      "previously_new_record?", "reload", "save", "save!", "toggle", "toggle!", "touch", "update",
+      "update!", "update_attribute", "update_column", "update_columns"
+    ]
+}
+
+// Methods with these names are defined for all active record model instances,
+// so they are unlikely to refer to a database field.
+private predicate isBuiltInMethodForActiveRecordModelInstance(string methodName) {
+  methodName = activeRecordPersistenceInstanceMethodName() or
+  methodName = basicObjectInstanceMethodName() or
+  methodName = objectInstanceMethodName()
+}
+
 /**
  * A `ClassDeclaration` for a class that extends `ActiveRecord::Base`. For example,
  *
@@ -52,11 +71,14 @@ class ActiveRecordModelClass extends ClassDeclaration {
     // There is a value that can be returned by this method which may include field data
     exists(DataFlow::Node returned, ActiveRecordInstanceMethodCall cNode, MethodCall c |
       exprNodeReturnedFrom(returned, result) and cNode.flowsTo(returned) and c = cNode.asExpr().getExpr() |
-      // The referenced method is not built-in, and
-      not isCallToBuiltInMethod(c) and (
-        // There is no matching method definition in the class, or
+      // The referenced method is not built-in, and...
+      not isBuiltInMethodForActiveRecordModelInstance(c.getMethodName()) and (
+        // TODO: this would be more accurate if we also checked methods defined in
+        // super classes and mixins
+
+        // ...There is no matching method definition in the class, or...
         not exists(cNode.getInstance().getClass().getMethod(c.getMethodName())) or
-        // The called method can access a field
+        // ...the called method can access a field
         c.getATarget() = cNode.getInstance().getClass().methodMayAccessField()
       )
     )
@@ -202,18 +224,17 @@ private string constantQualifiedName(ConstantWriteAccess w) {
 abstract class ActiveRecordModelInstantiation extends OrmInstantiation::Range, DataFlow::LocalSourceNode {
   abstract ActiveRecordModelClass getClass();
 
-  override predicate methodCallMayAccessField(MethodCall call) {
-    // The method is not a built-in
-    not isCallToBuiltInMethod(call) and (
-      // There is no matching method definition in the class, or
-      not exists(this.getClass().getMethod(call.getMethodName())) or
-      // The called method can access a field
+  bindingset[methodName]
+  override predicate methodCallMayAccessField(string methodName) {
+    // The method is not a built-in, and...
+    not isBuiltInMethodForActiveRecordModelInstance(methodName) and (
+      // ...There is no matching method definition in the class, or...
+      not exists(this.getClass().getMethod(methodName)) or
+      // ...the called method can access a field.
       exists(Method m |
         m = this.getClass().methodMayAccessField() |
-        // TODO: this may be too broad - we haven't limited the call target
-        // It's likely that the call graph isn't sufficient here, as resolution
-        // e.g. from ActionView views won't catch everything
-        m.getName() = call.getMethodName()
+        // We rely on matching by name here as the call graph might not have
+        m.getName() = methodName
       )
     )
   }
@@ -298,20 +319,4 @@ private class ActiveRecordInstanceMethodCall extends DataFlow::CallNode {
   private ActiveRecordInstance instance;
   ActiveRecordInstanceMethodCall() { this.getReceiver() = instance }
   ActiveRecordInstance getInstance() { result = instance }
-}
-
-private string activeRecordPersistenceInstanceMethodName() {
-  result =
-    [
-      "becomes", "becomes!", "decrement", "decrement!", "delete", "delete!", "destroy", "destroy!",
-      "destroyed?", "increment", "increment!", "new_record?", "persisted?",
-      "previously_new_record?", "reload", "save", "save!", "toggle", "toggle!", "touch", "update",
-      "update!", "update_attribute", "update_column", "update_columns"
-    ]
-}
-
-private predicate isCallToBuiltInMethod(MethodCall c) {
-  c.getMethodName() = activeRecordPersistenceInstanceMethodName() or
-  c instanceof BasicObjectInstanceMethodCall or
-  c instanceof ObjectInstanceMethodCall
-}
+}

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

@@ -65,34 +65,42 @@ private predicate isPrivateKernelMethod(string method) {
     ]
 }
 
+string basicObjectInstanceMethodName() {
+  result in [
+    "equal?", "instance_eval", "instance_exec", "method_missing", "singleton_method_added",
+    "singleton_method_removed", "singleton_method_undefined"
+  ]
+}
+
 /**
  * Instance methods on `BasicObject`, which are available to all classes.
  */
 class BasicObjectInstanceMethodCall extends UnknownMethodCall {
   BasicObjectInstanceMethodCall() {
-    this.getMethodName() in [
-        "equal?", "instance_eval", "instance_exec", "method_missing", "singleton_method_added",
-        "singleton_method_removed", "singleton_method_undefined"
-      ]
+    this.getMethodName() = basicObjectInstanceMethodName()
   }
 }
 
+string objectInstanceMethodName() {
+  result in [
+    "!~", "<=>", "===", "=~", "callable_methods", "define_singleton_method", "display",
+    "do_until", "do_while", "dup", "enum_for", "eql?", "extend", "f", "freeze", "h", "hash",
+    "inspect", "instance_of?", "instance_variable_defined?", "instance_variable_get",
+    "instance_variable_set", "instance_variables", "is_a?", "itself", "kind_of?",
+    "matching_methods", "method", "method_missing", "methods", "nil?", "object_id",
+    "private_methods", "protected_methods", "public_method", "public_methods", "public_send",
+    "remove_instance_variable", "respond_to?", "respond_to_missing?", "send",
+    "shortest_abbreviation", "singleton_class", "singleton_method", "singleton_methods",
+    "taint", "tainted?", "to_enum", "to_s", "trust", "untaint", "untrust", "untrusted?"
+  ]
+}
+
 /**
  * Instance methods on `Object`, which are available to all classes except `BasicObject`.
  */
 class ObjectInstanceMethodCall extends UnknownMethodCall {
   ObjectInstanceMethodCall() {
-    this.getMethodName() in [
-        "!~", "<=>", "===", "=~", "callable_methods", "define_singleton_method", "display",
-        "do_until", "do_while", "dup", "enum_for", "eql?", "extend", "f", "freeze", "h", "hash",
-        "inspect", "instance_of?", "instance_variable_defined?", "instance_variable_get",
-        "instance_variable_set", "instance_variables", "is_a?", "itself", "kind_of?",
-        "matching_methods", "method", "method_missing", "methods", "nil?", "object_id",
-        "private_methods", "protected_methods", "public_method", "public_methods", "public_send",
-        "remove_instance_variable", "respond_to?", "respond_to_missing?", "send",
-        "shortest_abbreviation", "singleton_class", "singleton_method", "singleton_methods",
-        "taint", "tainted?", "to_enum", "to_s", "trust", "untaint", "untrust", "untrusted?"
-      ]
+    this.getMethodName() = objectInstanceMethodName()
   }
 }
 

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

@@ -123,7 +123,7 @@ private module Shared {
   }
 
   /**
-   * An additional step that is preserves dataflow in the context of reflected XSS.
+   * An additional step that is preserves dataflow in the context of XSS.
    */
   predicate isAdditionalXSSFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     // node1 is a `locals` argument to a render call...
@@ -229,7 +229,9 @@ module ReflectedXSS {
     }
   }
 
-  // Consider all arbitrary XSS taint steps to be reflected XSS taint steps
+  /**
+   * An additional step that is preserves dataflow in the context of reflected XSS.
+   */
   predicate isAdditionalXSSTaintStep = Shared::isAdditionalXSSFlowStep/2;
 
   /**
@@ -238,7 +240,7 @@ module ReflectedXSS {
   class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
 }
 
-private module OrmTracking {
+module OrmTracking {
   /**
    * A data flow configuration to track flow from finder calls to field accesses.
    */
@@ -285,13 +287,17 @@ module StoredXSS {
     }
   }
 
+  /**
+   * An additional step that is preserves dataflow in the context of stored XSS.
+   */
   predicate isAdditionalXSSTaintStep = Shared::isAdditionalXSSFlowStep/2;
 
-  private class OrmFieldAsSource extends Source {
+  private class OrmFieldAsSource extends Source instanceof DataFlow2::CallNode {
     OrmFieldAsSource() {
-      exists(OrmTracking::Configuration subConfig, DataFlow2::Node subSrc |
+      exists(OrmTracking::Configuration subConfig, DataFlow2::CallNode subSrc, MethodCall call |
         subConfig.hasFlow(subSrc, this) and
-        subSrc.(OrmInstantiation).methodCallMayAccessField(this.asExpr().getExpr())
+        call = this.asExpr().getExpr() and
+        subSrc.(OrmInstantiation).methodCallMayAccessField(call.getMethodName())
       )
     }
   }