abstract away some ActiveRecord specific parts of XSS.qll

Author: alexrford

ql/lib/codeql/ruby/Concepts.qll

@@ -538,3 +538,28 @@ module XmlParserCall {
     abstract predicate externalEntitiesEnabled();
   }
 }
+
+/**
+ * A data-flow node that may represent a database object in an ORM system.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * 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) }
+}
+
+/** Provides a class for modeling new ORM object instantiation APIs. */
+module OrmInstantiation {
+  /**
+   * A data-flow node that may represent a database object in an ORM system.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * 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);
+  }
+}

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

@@ -43,6 +43,26 @@ class ActiveRecordModelClass extends ClassDeclaration {
       other.getModule() = resolveScopeExpr(this.getSuperclassExpr())
     )
   }
+
+  /**
+   * Returns true if `call` may refer to a method that returns a database value
+   * if invoked against an instance of this class.
+   */
+  predicate methodCallMayAccessField(MethodCall call) {
+    not (
+      // Methods whose names can be hardcoded
+      isCallToBuiltInMethod(call)
+      or
+      // Methods defined in the ActiveRecord model class that do not return database fields
+      exists(Method m | m = this.getMethod(call.getMethodName()) |
+        forall(DataFlow::Node returned, ActiveRecordInstanceMethodCall c |
+          exprNodeReturnedFrom(returned, m) and c.flowsTo(returned)
+        |
+          not this.methodCallMayAccessField(returned.asExpr().getExpr())
+        )
+      )
+    )
+  }
 }
 
 /** A class method call whose receiver is an `ActiveRecordModelClass`. */
@@ -181,8 +201,12 @@ private string constantQualifiedName(ConstantWriteAccess w) {
 /**
  * A node that may evaluate to one or more `ActiveRecordModelClass` instances.
  */
-abstract class ActiveRecordModelInstantiation extends DataFlow::Node {
+abstract class ActiveRecordModelInstantiation extends OrmInstantiation::Range {
   abstract ActiveRecordModelClass getClass();
+
+  override predicate methodCallMayAccessField(MethodCall call) {
+    this.getClass().methodCallMayAccessField(call)
+  }
 }
 
 // Names of class methods on ActiveRecord models that may return one or more
@@ -278,23 +302,3 @@ private predicate isCallToBuiltInMethod(MethodCall c) {
   c instanceof BasicObjectInstanceMethodCall or
   c instanceof ObjectInstanceMethodCall
 }
-
-/**
- * Returns true if `call` may refer to a method that returns a database value
- * if invoked against a `sourceClass` instance.
- */
-predicate activeRecordMethodMayAccessField(ActiveRecordModelClass sourceClass, MethodCall call) {
-  not (
-    // Methods whose names can be hardcoded
-    isCallToBuiltInMethod(call)
-    or
-    // Methods defined in `sourceClass` that do not return database fields
-    exists(Method m | m = sourceClass.getMethod(call.getMethodName()) |
-      forall(DataFlow::Node returned, ActiveRecordInstanceMethodCall c |
-        exprNodeReturnedFrom(returned, m) and c.flowsTo(returned)
-      |
-        not activeRecordMethodMayAccessField(sourceClass, returned.asExpr().getExpr())
-      )
-    )
-  )
-}

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

@@ -10,7 +10,6 @@ private import codeql.ruby.Concepts
 private import codeql.ruby.Frameworks
 private import codeql.ruby.frameworks.ActionController
 private import codeql.ruby.frameworks.ActionView
-private import codeql.ruby.frameworks.ActiveRecord
 private import codeql.ruby.dataflow.RemoteFlowSources
 private import codeql.ruby.dataflow.BarrierGuards
 private import codeql.ruby.dataflow.internal.DataFlowDispatch
@@ -246,10 +245,7 @@ private module OrmTracking {
   class Configuration extends DataFlow2::Configuration {
     Configuration() { this = "OrmTracking" }
 
-    // TODO: abstract to ORMFinderCall concept
-    override predicate isSource(DataFlow2::Node source) {
-      source instanceof ActiveRecordModelInstantiation
-    }
+    override predicate isSource(DataFlow2::Node source) { source instanceof OrmInstantiation }
 
     // Select any call node and narrow down later
     override predicate isSink(DataFlow2::Node sink) { sink instanceof DataFlow2::CallNode }
@@ -291,12 +287,11 @@ module StoredXSS {
 
   predicate isAdditionalXSSTaintStep = Shared::isAdditionalXSSFlowStep/2;
 
-  private class ActiveRecordModelFieldAccessAsSource extends Source {
-    ActiveRecordModelFieldAccessAsSource() {
+  private class OrmFieldAsSource extends Source {
+    OrmFieldAsSource() {
       exists(OrmTracking::Configuration subConfig, DataFlow2::Node subSrc |
         subConfig.hasFlow(subSrc, this) and
-        activeRecordMethodMayAccessField(subSrc.(ActiveRecordModelInstantiation).getClass(),
-          this.asExpr().getExpr())
+        subSrc.(OrmInstantiation).methodCallMayAccessField(this.asExpr().getExpr())
       )
     }
   }