Ruby: make SensitiveExpr a dataflow node rather than an Expr

Author: alexrford

ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

@@ -684,6 +684,15 @@ module ExprNodes {
     final ExprCfgNode getValue() { e.hasCfgChild(e.getValue(), this, result) }
   }
 
+  /** A control-flow node that wraps a `VariableAccess` AST expression. */
+  class VariableAccessCfgNode extends ExprCfgNode {
+    override string getAPrimaryQlClass() { result = "VariableAccessCfgNode" }
+
+    override VariableAccess e;
+
+    final override VariableAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+  }
+
   /** A control-flow node that wraps a `VariableReadAccess` AST expression. */
   class VariableReadAccessCfgNode extends ExprCfgNode {
     override string getAPrimaryQlClass() { result = "VariableReadAccessCfgNode" }

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

@@ -13,10 +13,11 @@ private import codeql.ruby.AST
 private import codeql.ruby.DataFlow
 import codeql.ruby.security.internal.SensitiveDataHeuristics
 private import HeuristicNames
+private import codeql.ruby.CFG
 
 /** An expression that might contain sensitive data. */
 cached
-abstract class SensitiveExpr extends Expr {
+abstract class SensitiveNode extends DataFlow::Node {
   /** Gets a human-readable description of this expression for use in alert messages. */
   cached
   abstract string describe();
@@ -27,32 +28,36 @@ abstract class SensitiveExpr extends Expr {
 }
 
 /** A method call that might produce sensitive data. */
-class SensitiveCall extends SensitiveExpr, MethodCall {
+class SensitiveCall extends SensitiveNode instanceof DataFlow::CallNode {
   SensitiveDataClassification classification;
 
   SensitiveCall() {
     classification = this.getMethodName().(SensitiveDataMethodName).getClassification()
     or
     // This is particularly to pick up methods with an argument like "password", which
     // may indicate a lookup.
-    exists(string s | this.getAnArgument().getConstantValue().isStringlikeValue(s) |
+    exists(string s | super.getArgument(_).asExpr().getConstantValue().isStringlikeValue(s) |
       nameIndicatesSensitiveData(s, classification)
     )
   }
 
-  override string describe() { result = "a call to " + this.getMethodName() }
+  override string describe() { result = "a call to " + super.getMethodName() }
 
   override SensitiveDataClassification getClassification() { result = classification }
 }
 
 /** An access to a variable or hash value that might contain sensitive data. */
-abstract class SensitiveVariableAccess extends SensitiveExpr {
+abstract class SensitiveVariableAccess extends SensitiveNode {
   string name;
 
   SensitiveVariableAccess() {
-    this.(VariableAccess).getVariable().hasName(name)
+    this.asExpr().(CfgNodes::ExprNodes::VariableAccessCfgNode).getExpr().getVariable().hasName(name)
     or
-    this.(ElementReference).getAnArgument().getConstantValue().isStringlikeValue(name)
+    this.asExpr()
+        .(CfgNodes::ExprNodes::ElementReferenceCfgNode)
+        .getAnArgument()
+        .getConstantValue()
+        .isStringlikeValue(name)
   }
 
   override string describe() { result = "an access to " + name }

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

@@ -34,11 +34,10 @@ private predicate localFlowWithElementReference(DataFlow::LocalSourceNode src, D
 
 from
   HTTP::Server::RequestHandler handler, HTTP::Server::RequestInputAccess input,
-  DataFlow::Node sensitive
+  SensitiveNode sensitive
 where
   handler.getAnHttpMethod() = "get" and
   input.asExpr().getExpr().getEnclosingMethod() = handler and
-  sensitive.asExpr().getExpr() instanceof SensitiveExpr and
   localFlowWithElementReference(input, sensitive)
 select input, "$@ for GET requests uses query parameter as sensitive data.", handler,
   "Route handler"