Encapsulate composite actions

JarLob · JarLob · commit d80c541da640 · 2023-04-14T10:06:35.000+02:00
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -28,7 +28,12 @@ module Actions {
    * A custom composite action. This is a mapping at the top level of an Actions YAML action file.
    * See https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions.
    */
-  class Action extends Node, YamlDocument, YamlMapping {
+  class CompositeAction extends Node, YamlDocument, YamlMapping {
+    CompositeAction() {
+      this.getFile().getBaseName() = "action.yml" and
+      this.lookup("runs").(YamlMapping).lookup("using").(YamlScalar).getValue() = "composite"
+    }
+
     /** Gets the `runs` mapping. */
     Runs getRuns() { result = this.lookup("runs") }
   }
@@ -38,12 +43,15 @@ module Actions {
    * See https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#runs
    */
   class Runs extends StepsContainer {
-    Action action;
+    CompositeAction action;
 
     Runs() { action.lookup("runs") = this }
 
     /** Gets the action that this `runs` mapping is in. */
-    Action getAction() { result = action }
+    CompositeAction getAction() { result = action }
+
+    /** Gets the `using` mapping. */
+    Using getUsing() { result = this.lookup("using") }
   }
 
   /**
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -149,9 +149,8 @@ predicate isScriptInjectable(Actions::Script script, string injection, string co
 
 from YamlNode node, string injection, string context
 where
-  exists(Actions::Using u, Actions::Runs runs |
-    u.getValue() = "composite" and
-    u.getRuns() = runs and
+  exists(Actions::CompositeAction action, Actions::Runs runs |
+    action.getRuns() = runs and
     (
       exists(Actions::Run run |
         isRunInjectable(run, injection, context) and
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
@@ -62,4 +62,4 @@
 | .github/workflows/workflow_run.yml:14:12:14:77 | echo '$ ... ame }}' | Potential injection from the ${ github.event.workflow_run.head_commit.committer.name }, which may be controlled by an external user. |
 | .github/workflows/workflow_run.yml:15:12:15:62 | echo '$ ... nch }}' | Potential injection from the ${ github.event.workflow_run.head_branch }, which may be controlled by an external user. |
 | .github/workflows/workflow_run.yml:16:12:16:78 | echo '$ ... ion }}' | Potential injection from the ${ github.event.workflow_run.head_repository.description }, which may be controlled by an external user. |
-| action.yml:14:12:14:50 | echo '$ ... ody }}' | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |
+| action1/action.yml:14:12:14:50 | echo '$ ... ody }}' | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/action1/action.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/action1/action.yml
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/action2/action.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/action2/action.yml
@@ -0,0 +1,17 @@
+name: 'Hello World'
+description: 'Greet someone and record the time'
+inputs:
+  who-to-greet:  # id of input
+    description: 'Who to greet'
+    required: true
+    default: 'World'
+outputs:
+  time: # id of output
+    description: 'The time we greeted you'
+runs:
+  using: 'docker'
+  steps: # this is actually invalid, used to test we correctly identify composite actions
+    - run: echo '${{ github.event.comment.body }}'
+  image: 'Dockerfile'
+  args:
+    - ${{ inputs.who-to-greet }}
