Merge pull request #3 from GitHubSecurityLab/extensible_predicates

Add support for external definitions

Author: Alvaro Muñoz

ql/lib/codeql/actions/dataflow/ExternalFlow.qll

@@ -0,0 +1,31 @@
+private import internal.ExternalFlowExtensions as Extensions
+import codeql.actions.DataFlow
+import actions
+
+/** Holds if a source model exists for the given parameters. */
+predicate sourceModel(string action, string version, string output, string kind) {
+  Extensions::sourceModel(action, version, output, kind)
+}
+
+/** Holds if a sink model exists for the given parameters. */
+predicate summaryModel(string action, string version, string input, string output, string kind) {
+  Extensions::summaryModel(action, version, input, output, kind)
+}
+
+/** Holds if a sink model exists for the given parameters. */
+predicate sinkModel(string action, string version, string input, string kind) {
+  Extensions::sinkModel(action, version, input, kind)
+}
+
+predicate sinkNode(DataFlow::ExprNode sink, string kind) {
+  exists(UsesExpr uses, string action, string version, string input |
+    uses.getArgumentExpr(input.splitAt(",").trim()) = sink.asExpr() and
+    sinkModel(action, version, input, kind) and
+    uses.getCallee() = action and
+    (
+      if version.trim() = "*"
+      then uses.getVersion() = any(string v)
+      else uses.getVersion() = version.splitAt(",").trim()
+    )
+  )
+}

ql/lib/codeql/actions/dataflow/FlowSources.qll

@@ -1,5 +1,6 @@
 import actions
 import codeql.actions.DataFlow
+import codeql.actions.dataflow.ExternalFlow
 
 /**
  * A data flow source.
@@ -124,14 +125,22 @@ private class EventSource extends RemoteFlowSource {
   override string getSourceType() { result = "User-controlled events" }
 }
 
-private class ChangedFilesSource extends RemoteFlowSource {
-  ChangedFilesSource() {
-    exists(UsesExpr uses |
-      uses.getCallee() = "tj-actions/changed-files" and
-      uses.getVersion() = ["v10", "v20", "v30", "v40"] and
-      uses = this.asExpr()
+private class ExternallyDefinedSource extends RemoteFlowSource {
+  string soutceType;
+
+  ExternallyDefinedSource() {
+    exists(UsesExpr uses, string action, string version, /*string output,*/ string kind |
+      sourceModel(action, version, _, kind) and
+      uses.getCallee() = action and
+      (
+        if version.trim() = "*"
+        then uses.getVersion() = any(string v)
+        else uses.getVersion() = version.splitAt(",").trim()
+      ) and
+      uses = this.asExpr() and
+      soutceType = kind
     )
   }
 
-  override string getSourceType() { result = "User-controlled list of changed files" }
+  override string getSourceType() { result = soutceType }
 }

ql/lib/codeql/actions/dataflow/FlowSteps.qll

@@ -5,6 +5,7 @@
 import actions
 private import codeql.util.Unit
 private import codeql.actions.DataFlow
+import codeql.actions.dataflow.ExternalFlow
 
 /**
  * A unit class for adding additional taint steps.
@@ -20,16 +21,23 @@ class AdditionalTaintStep extends Unit {
   abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
 }
 
-/**
- * Holds if actions-find-and-replace-string step is used.
- */
-private class ActionsFindAndReplaceStringStep extends AdditionalTaintStep {
+predicate externallyDefinedSummary(DataFlow::Node pred, DataFlow::Node succ) {
+  exists(UsesExpr uses, string action, string version, string input |
+    /*, string output */ summaryModel(action, version, input, _, "taint") and
+    uses.getCallee() = action and
+    (
+      if version.trim() = "*"
+      then uses.getVersion() = any(string v)
+      else uses.getVersion() = version.splitAt(",").trim()
+    ) and
+    pred.asExpr() = uses.getArgumentExpr(input.splitAt(",").trim()) and
+    succ.asExpr() = uses
+  )
+}
+
+private class ExternallyDefinedSummary extends AdditionalTaintStep {
   override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(UsesExpr u |
-      u.getCallee() = "mad9000/actions-find-and-replace-string" and
-      pred.asExpr() = u.getArgumentExpr(["source", "replace"]) and
-      succ.asExpr() = u
-    )
+    externallyDefinedSummary(pred, succ)
   }
 }
 
@@ -46,10 +54,12 @@ private class ActionsFindAndReplaceStringStep extends AdditionalTaintStep {
  *      echo "::set-output name=initial_url::$INITIAL_URL"
  */
 private class RunEnvToScriptStep extends AdditionalTaintStep {
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) { test(pred, succ) }
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    runEnvToScriptstep(pred, succ)
+  }
 }
 
-predicate test(DataFlow::Node pred, DataFlow::Node succ) {
+predicate runEnvToScriptstep(DataFlow::Node pred, DataFlow::Node succ) {
   exists(RunExpr r, string varName |
     r.getEnvExpr(varName) = pred.asExpr() and
     exists(string script, string line |

ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll

@@ -0,0 +1,20 @@
+/**
+ * This module provides extensible predicates for defining MaD models.
+ */
+
+/**
+ * Holds if a source model exists for the given parameters.
+ */
+extensible predicate sourceModel(string action, string version, string output, string kind);
+
+/**
+ * Holds if a summary model exists for the given parameters.
+ */
+extensible predicate summaryModel(
+  string action, string version, string input, string output, string kind
+);
+
+/**
+ * Holds if a sink model exists for the given parameters.
+ */
+extensible predicate sinkModel(string action, string version, string input, string kind);

ql/lib/ext/sinks.model.yml

@@ -0,0 +1,11 @@
+extensions:
+  - addsTo:
+      pack: codeql/actions-all
+      extensible: sinkModel
+    data:
+      - [
+          "FAKE-mad9000/actions-find-and-replace-string",
+          "*",
+          "source",
+          "expression-injection",
+        ]

ql/lib/ext/sources.model.yml

@@ -0,0 +1,11 @@
+extensions:
+  - addsTo:
+      pack: codeql/actions-all
+      extensible: sourceModel
+    data:
+      - [
+          "tj-actions/changed-files",
+          "v10, v20, v30, v40",
+          "all_changed_file",
+          "PR",
+        ]

ql/lib/ext/summaries.model.yml

@@ -0,0 +1,19 @@
+extensions:
+  - addsTo:
+      pack: codeql/actions-all
+      extensible: summaryModel
+    data:
+      - [
+          "mad9000/actions-find-and-replace-string",
+          "*",
+          "source, replace",
+          "value",
+          "taint",
+        ]
+      - [
+          "frabert/replace-string-action",
+          "*",
+          "string, replace-with",
+          "replaced",
+          "taint",
+        ]

ql/lib/qlpack.yml

@@ -5,11 +5,13 @@ name: codeql/actions-all
 version: 0.0.1-dev
 dependencies:
   codeql/controlflow: ^0.1.7
-  codeql/yaml: '*'
-  codeql/util: '*'
+  codeql/yaml: "*"
+  codeql/util: "*"
   codeql/dataflow: ^0.1.7
 dbscheme: yaml.dbscheme
 extractor: yaml
 tests: test
 groups:
- - yaml
+  - yaml
+dataExtensions:
+  - ext/*.model.yml

ql/lib/test/test.ql

@@ -3,6 +3,7 @@ import codeql.actions.Ast
 import codeql.actions.Cfg as Cfg
 import codeql.actions.DataFlow
 import codeql.Locations
+import codeql.actions.dataflow.ExternalFlow
 
 query predicate files(File f) { any() }
 
@@ -19,7 +20,7 @@ query predicate stepUsesNodes(StepUsesExpr s) { any() }
 query predicate jobUsesNodes(JobUsesExpr s) { any() }
 
 query predicate usesSteps(UsesExpr call, string argname, Expression arg) {
-  call.getArgument(argname) = arg
+  call.getArgumentExpr(argname) = arg
 }
 
 query predicate runSteps1(RunExpr run, string body) { run.getScript() = body }
@@ -48,7 +49,7 @@ query predicate parentNodes(AstNode child, AstNode parent) { child.getParentNode
 
 query predicate cfgNodes(Cfg::Node n) {
   //any()
-  n.getAstNode() instanceof OutputsStmt
+  n.getAstNode() instanceof ReusableWorkflowOutputsStmt
 }
 
 query predicate dfNodes(DataFlow::Node e) {
@@ -68,3 +69,11 @@ query predicate varIds(StepOutputAccessExpr s, string a) { s.getStepId() = a }
 query predicate nodeLocations(DataFlow::Node n, Location l) { n.getLocation() = l }
 
 query predicate scopes(Cfg::CfgScope c) { any() }
+
+query predicate sources(string action, string version, string output, string kind) {
+  sourceModel(action, version, output, kind)
+}
+
+query predicate summaries(string action, string version, string input, string output, string kind) {
+  summaryModel(action, version, input, output, kind)
+}

ql/src/Security/CWE-094/ExpressionInjection.ql

@@ -15,9 +15,13 @@
 import actions
 import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
+import codeql.actions.dataflow.ExternalFlow
 
 private class ExpressionInjectionSink extends DataFlow::Node {
-  ExpressionInjectionSink() { exists(RunExpr e | e.getScriptExpr() = this.asExpr()) }
+  ExpressionInjectionSink() {
+    exists(RunExpr e | e.getScriptExpr() = this.asExpr()) or
+    sinkNode(this, "expression-injection")
+  }
 }
 
 private module MyConfig implements DataFlow::ConfigSig {