Merge pull request #213 from github/aibaars/api-graphs2

First version of ApiGraphs

Author: aibaars

ql/src/codeql_ruby/ApiGraphs.qll

@@ -240,6 +240,11 @@ module ExprNodes {
     /** Gets the `n`th argument of this call. */
     final ExprCfgNode getArgument(int n) { e.hasCfgChild(e.getArgument(n), this, result) }
 
+    /** Gets the the keyword argument whose key is `keyword` of this call. */
+    final ExprCfgNode getKeywordArgument(string keyword) {
+      e.hasCfgChild(e.getKeywordArgument(keyword), this, result)
+    }
+
     /** Gets the number of arguments of this call. */
     final int getNumberOfArguments() { result = e.getNumberOfArguments() }
 
@@ -296,6 +301,20 @@ module ExprNodes {
     final ExprCfgNode getBranch(boolean cond) { e.hasCfgChild(e.getBranch(cond), this, result) }
   }
 
+  private class ConstantAccessChildMapping extends ExprChildMapping, ConstantAccess {
+    override predicate relevantChild(Expr e) { e = this.getScopeExpr() }
+  }
+
+  /** A control-flow node that wraps a `ConditionalExpr` AST expression. */
+  class ConstantAccessCfgNode extends ExprCfgNode {
+    override ConstantAccessChildMapping e;
+
+    final override ConstantAccess getExpr() { result = super.getExpr() }
+
+    /** Gets the scope expression. */
+    final ExprCfgNode getScopeExpr() { e.hasCfgChild(e.getScopeExpr(), this, result) }
+  }
+
   private class StmtSequenceChildMapping extends ExprChildMapping, StmtSequence {
     override predicate relevantChild(Expr e) { e = this.getLastStmt() }
   }

ql/src/codeql_ruby/controlflow/CfgNodes.qll

@@ -39,6 +39,22 @@ class Node extends TNode {
   }
 }
 
+/** A data-flow node corresponding to a call in the control-flow graph. */
+class CallNode extends LocalSourceNode {
+  private CfgNodes::ExprNodes::CallCfgNode node;
+
+  CallNode() { node = this.asExpr() }
+
+  /** Gets the data-flow node corresponding to the receiver of the call corresponding to this data-flow node */
+  Node getReceiver() { result.asExpr() = node.getReceiver() }
+
+  /** Gets the data-flow node corresponding to the `n`th argument of the call corresponding to this data-flow node */
+  Node getArgument(int n) { result.asExpr() = node.getArgument(n) }
+
+  /** Gets the data-flow node corresponding to the named argument of the call corresponding to this data-flow node */
+  Node getKeywordArgument(string name) { result.asExpr() = node.getKeywordArgument(name) }
+}
+
 /**
  * An expression, viewed as a node in a data flow graph.
  *

ql/src/codeql_ruby/dataflow/internal/DataFlowPublic.qll

@@ -12,22 +12,7 @@
 import ruby
 import codeql_ruby.DataFlow
 import DataFlow::PathGraph
-private import codeql_ruby.dataflow.SSA
-
-// TODO: account for flows through tuple assignments
-/** An expression referencing the File or FileUtils module */
-class FileModuleAccess extends Expr {
-  FileModuleAccess() {
-    this.(ConstantAccess).getName() = "File"
-    or
-    this.(ConstantAccess).getName() = "FileUtils"
-    or
-    exists(FileModuleAccess fma, Ssa::WriteDefinition def |
-      def.getARead() = this.getAControlFlowNode() and
-      def.getWriteAccess().getParent().(Assignment).getRightOperand() = fma
-    )
-  }
-}
+import codeql_ruby.ApiGraphs
 
 bindingset[p]
 int world_permission(int p) { result = p.bitAnd(7) }
@@ -58,14 +43,13 @@ class PermissivePermissionsExpr extends Expr {
 }
 
 /** A permissions argument of a call to a File/FileUtils method that may modify file permissions */
-class PermissionArgument extends Expr {
-  private MethodCall call;
-  private string methodName;
+class PermissionArgument extends DataFlow::Node {
+  private DataFlow::CallNode call;
 
   PermissionArgument() {
-    call.getReceiver() instanceof FileModuleAccess and
-    call.getMethodName() = methodName and
-    (
+    exists(string methodName |
+      call = API::moduleImport(["File", "FileUtils"]).getAMethodCall(methodName)
+    |
       methodName in ["chmod", "chmod_R", "lchmod"] and this = call.getArgument(0)
       or
       methodName = "mkfifo" and this = call.getArgument(1)
@@ -78,7 +62,7 @@ class PermissionArgument extends Expr {
     )
   }
 
-  MethodCall getCall() { result = call }
+  MethodCall getCall() { result = call.asExpr().getExpr() }
 }
 
 class PermissivePermissionsConfig extends DataFlow::Configuration {
@@ -88,14 +72,12 @@ class PermissivePermissionsConfig extends DataFlow::Configuration {
     exists(PermissivePermissionsExpr ppe | source.asExpr().getExpr() = ppe)
   }
 
-  override predicate isSink(DataFlow::Node sink) {
-    exists(PermissionArgument arg | sink.asExpr().getExpr() = arg)
-  }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof PermissionArgument }
 }
 
 from
   DataFlow::PathNode source, DataFlow::PathNode sink, PermissivePermissionsConfig conf,
   PermissionArgument arg
-where conf.hasFlowPath(source, sink) and arg = sink.getNode().asExpr().getExpr()
+where conf.hasFlowPath(source, sink) and arg = sink.getNode()
 select source.getNode(), source, sink, "Overly permissive mask in $@ sets file to $@.",
   arg.getCall(), arg.getCall().toString(), source.getNode(), source.getNode().toString()