From 8ea418216c4cc997d6f73289fb37c41203310cf0 Mon Sep 17 00:00:00 2001 From: jarlob Date: Mon, 3 Apr 2023 23:13:28 +0200 Subject: [PATCH] Look for script injections in actions/github-script --- .../ql/lib/semmle/javascript/Actions.qll | 49 +++++++++++++------ .../Security/CWE-094/ExpressionInjection.ql | 22 +++++++-- .../.github/workflows/comment_issue.yml | 15 +++++- .../ExpressionInjection.expected | 3 ++ 4 files changed, 69 insertions(+), 20 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll index b1ab674924d..3d1d4c589d1 100644 --- a/javascript/ql/lib/semmle/javascript/Actions.qll +++ b/javascript/ql/lib/semmle/javascript/Actions.qll @@ -244,6 +244,40 @@ module Actions { With getWith() { result = with } } + /** + * Holds if `${{ e }}` is a GitHub Actions expression evaluated within this YAML string. + * See https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions. + * Only finds simple expressions like `${{ github.event.comment.body }}`, where the expression contains only alphanumeric characters, underscores, dots, or dashes. + * Does not identify more complicated expressions like `${{ fromJSON(env.time) }}`, or ${{ format('{{Hello {0}!}}', github.event.head_commit.author.name) }} + */ + string getASimpleReferenceExpression(YamlString node) { + // We use `regexpFind` to obtain *all* matches of `${{...}}`, + // not just the last (greedy match) or first (reluctant match). + result = + node.getValue() + .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9_\\[\\]\\*\\(\\)\\.\\-]+\\s*\\}\\}", _, _) + .regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+)\\s*\\}\\}", 1) + } + + /** + * A `script:` field within an Actions `with:` specific to `actions/github-script` action. + * + * For example: + * ``` + * uses: actions/github-script@v3 + * with: + * script: console.log('${{ github.event.pull_request.head.sha }}') + * ``` + */ + class Script extends YamlNode, YamlString { + With with; + + Script() { with.lookup("script") = this } + + /** Gets the `with` field this field belongs to. */ + With getWith() { result = with } + } + /** * A `run` field within an Actions job step, which runs command-line programs using an operating system shell. * See https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstepsrun. @@ -255,20 +289,5 @@ module Actions { /** Gets the step that executes this `run` command. */ Step getStep() { result = step } - - /** - * Holds if `${{ e }}` is a GitHub Actions expression evaluated within this `run` command. - * See https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions. - * Only finds simple expressions like `${{ github.event.comment.body }}`, where the expression contains only alphanumeric characters, underscores, dots, or dashes. - * Does not identify more complicated expressions like `${{ fromJSON(env.time) }}`, or ${{ format('{{Hello {0}!}}', github.event.head_commit.author.name) }} - */ - string getASimpleReferenceExpression() { - // We use `regexpFind` to obtain *all* matches of `${{...}}`, - // not just the last (greedy match) or first (reluctant match). - result = - this.getValue() - .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9_\\[\\]\\*\\(\\)\\.\\-]+\\s*\\}\\}", _, _) - .regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+)\\s*\\}\\}", 1) - } } } diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql index ce815c8e11d..84e7215837e 100644 --- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql +++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql @@ -103,10 +103,24 @@ private predicate isExternalUserControlledWorkflowRun(string context) { ) } -from Actions::Run run, string context, Actions::On on +from YamlNode node, string context, Actions::On on where - run.getASimpleReferenceExpression() = context and - run.getStep().getJob().getWorkflow().getOn() = on and + ( + exists(Actions::Run run | + node = run and + Actions::getASimpleReferenceExpression(run) = context and + run.getStep().getJob().getWorkflow().getOn() = on + ) + or + exists(Actions::Script script, Actions::Step step, Actions::Uses uses | + node = script and + script.getWith().getStep() = step and + uses.getStep() = step and + uses.getGitHubRepository() = "actions/github-script" and + Actions::getASimpleReferenceExpression(script) = context and + script.getWith().getStep().getJob().getWorkflow().getOn() = on + ) + ) and ( exists(on.getNode("issues")) and isExternalUserControlledIssue(context) @@ -138,6 +152,6 @@ where exists(on.getNode("workflow_run")) and isExternalUserControlledWorkflowRun(context) ) -select run, +select node, "Potential injection from the " + context + " context, which may be controlled by an external user." diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml index fec20d7272f..17ead9fdd20 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml +++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml @@ -12,4 +12,17 @@ jobs: steps: - run: echo '${{ github.event.comment.body }}' - run: echo '${{ github.event.issue.body }}' - - run: echo '${{ github.event.issue.title }}' \ No newline at end of file + - run: echo '${{ github.event.issue.title }}' + + echo-chamber3: + runs-on: ubuntu-latest + steps: + - uses: actions/github-script@v3 + with: + script: console.log('${{ github.event.comment.body }}') + - uses: actions/github-script@v3 + with: + script: console.log('${{ github.event.issue.body }}') + - uses: actions/github-script@v3 + with: + script: console.log('${{ github.event.issue.title }}') \ No newline at end of file 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 index b89948010b8..775ec3ba640 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected @@ -2,6 +2,9 @@ | .github/workflows/comment_issue.yml:13:12:13:50 | echo '$ ... ody }}' | Potential injection from the github.event.comment.body context, which may be controlled by an external user. | | .github/workflows/comment_issue.yml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the github.event.issue.body context, which may be controlled by an external user. | | .github/workflows/comment_issue.yml:15:12:15:49 | echo '$ ... tle }}' | Potential injection from the github.event.issue.title context, which may be controlled by an external user. | +| .github/workflows/comment_issue.yml:22:17:22:63 | console ... dy }}') | Potential injection from the github.event.comment.body context, which may be controlled by an external user. | +| .github/workflows/comment_issue.yml:25:17:25:61 | console ... dy }}') | Potential injection from the github.event.issue.body context, which may be controlled by an external user. | +| .github/workflows/comment_issue.yml:28:17:28:62 | console ... le }}') | Potential injection from the github.event.issue.title context, which may be controlled by an external user. | | .github/workflows/comment_issue_newline.yml:9:14:10:50 | \| | Potential injection from the github.event.comment.body context, which may be controlled by an external user. | | .github/workflows/discussion.yml:7:12:7:54 | echo '$ ... tle }}' | Potential injection from the github.event.discussion.title context, which may be controlled by an external user. | | .github/workflows/discussion.yml:8:12:8:53 | echo '$ ... ody }}' | Potential injection from the github.event.discussion.body context, which may be controlled by an external user. |