diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll index a9967a72ee6..b79a86ce27a 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll @@ -295,3 +295,24 @@ class Xt0rtedSlashCommandSource extends RemoteFlowSource { override string getSourceType() { result = "text" } } + +class OctokitRequestActionSource extends RemoteFlowSource { + OctokitRequestActionSource() { + exists(UsesStep u, string route | + u.getCallee() = "octokit/request-action" and + route = u.getArgument("route").trim() and + route.indexOf("GET") = 0 and + ( + route.matches("%/commits%") or + route.matches("%/comments%") or + route.matches("%/pulls%") or + route.matches("%/issues%") or + route.matches("%/users%") or + route.matches("%github.event.issue.pull_request.url%") + ) and + this.asExpr() = u + ) + } + + override string getSourceType() { result = "text" } +} diff --git a/ql/lib/codeql/actions/dataflow/TaintSteps.qll b/ql/lib/codeql/actions/dataflow/TaintSteps.qll index e9d5a44c929..80858df909b 100644 --- a/ql/lib/codeql/actions/dataflow/TaintSteps.qll +++ b/ql/lib/codeql/actions/dataflow/TaintSteps.qll @@ -91,11 +91,45 @@ predicate xt0rtedSlashCommandActionTaintStep(DataFlow::Node pred, DataFlow::Node ) } +/** + * A read of user-controlled field of the octokit/request-action action. + */ +predicate octokitRequestActionTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(StepsExpression o | + pred instanceof OctokitRequestActionSource and + o.getTarget() = pred.asExpr() and + o.getStepId() = pred.asExpr().(UsesStep).getId() and + succ.asExpr() = o and + ( + not o instanceof JsonReferenceExpression and + o.getFieldName() = "data" + or + o instanceof JsonReferenceExpression and + o.(JsonReferenceExpression).getInnerExpression().matches("%.data") and + o.(JsonReferenceExpression) + .getAccessPath() + .matches([ + "%.title", + "%.user.login", + "%.body", + "%.head.ref", + "%.head.repo.full_name", + "%.commit.author.email", + "%.commit.commiter.email", + "%.commit.message", + "%.email", + "%.name", + ]) + ) + ) +} + class TaintSteps extends AdditionalTaintStep { override predicate step(DataFlow::Node node1, DataFlow::Node node2) { dornyPathsFilterTaintStep(node1, node2) or tjActionsChangedFilesTaintStep(node1, node2) or tjActionsVerifyChangedFilesTaintStep(node1, node2) or - xt0rtedSlashCommandActionTaintStep(node1, node2) + xt0rtedSlashCommandActionTaintStep(node1, node2) or + octokitRequestActionTaintStep(node1, node2) } } diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/test17.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/test17.yml new file mode 100644 index 00000000000..559c69c4710 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/test17.yml @@ -0,0 +1,74 @@ +name: Test + +on: + issue_comment: + +permissions: + contents: read + pull-requests: write + +jobs: + setup: + runs-on: ubuntu-latest + steps: + - name: Get PR details + id: get-pr + if: github.event_name == 'issue_comment' + uses: octokit/request-action@v2.x + with: + route: GET /repos/${{ github.repository }}/pulls/${{ github.event.issue.number }} + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Set PR source branch as env variable + if: github.event_name == 'issue_comment' + run: | + PR_SOURCE_BRANCH=$(echo '${{ steps.get-pr.outputs.data }}' | jq -r '.head.ref') + echo "BRANCH=$PR_SOURCE_BRANCH" >> $GITHUB_ENV + setup2: + runs-on: ubuntu-latest + steps: + - name: Get PR details + uses: octokit/request-action@v2.x + id: get-pr-details + with: + route: GET /repos/{repository}/pulls/{pull_number} + repository: ${{ github.repository }} + pull_number: ${{ github.event.issue.number }} + env: + GITHUB_TOKEN: ${{ secrets.GH_TOKEN }} + - name: Set environment variables + run: | + MERGE_STATUS=${{ fromJson(steps.get-pr-details.outputs.data).mergeable }} + if $MERGE_STATUS; then echo "COMMENT=\[Fast Forward CI\] ${{ env.HEAD_REF }} cannot be merged into ${{ env.BASE_REF }} at the moment." >> $GITHUB_ENV; fi + echo "MERGE_STATUS=$MERGE_STATUS" >> $GITHUB_ENV + echo "BASE_REF=${{ fromJson(steps.get-pr-details.outputs.data).base.ref }}" >> $GITHUB_ENV + echo "HEAD_REF=${{ fromJson(steps.get-pr-details.outputs.data).head.ref }}" >> $GITHUB_ENV + setup3: + runs-on: ubuntu-latest + steps: + - id: issues + uses: octokit/request-action@v2.x + with: + route: GET /repos/${{ github.repository_owner }}/${{ github.repository }}/issues?state=open + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN}} + - run: | + echo '${{ steps.issues.outputs.data }}' > issues.json + setup4: + runs-on: ubuntu-latest + steps: + - id: get-pull-request + uses: octokit/request-action@v2.x + with: + route: GET /repos/{owner}/{repo}/pulls/{pull_number} + owner: foo + repo: bar + pull_number: ${{ github.event.issue.number }} + + - run: >- + echo "Pull request title is \"${{ + fromJson(steps.get-pull-request.outputs.data).title }}\" but expected + \"Updated test pull request\"" && exit 1 + + + diff --git a/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected b/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected index 699d53da9cc..1ad0d498791 100644 --- a/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected +++ b/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected @@ -155,6 +155,10 @@ edges | .github/workflows/test16.yml:100:30:100:70 | steps.commit-message.outputs.value | .github/workflows/test16.yml:99:13:102:8 | Job outputs node [commit-message] | provenance | | | .github/workflows/test16.yml:123:15:128:12 | Run Step: commit-message [value] | .github/workflows/test16.yml:100:30:100:70 | steps.commit-message.outputs.value | provenance | | | .github/workflows/test16.yml:125:20:125:75 | echo "value=$(git log -1 --pretty=%s)" >> $GITHUB_OUTPUT | .github/workflows/test16.yml:123:15:128:12 | Run Step: commit-message [value] | provenance | | +| .github/workflows/test17.yml:14:13:22:10 | Uses Step: get-pr | .github/workflows/test17.yml:25:41:25:72 | steps.get-pr.outputs.data | provenance | | +| .github/workflows/test17.yml:30:13:39:10 | Uses Step: get-pr-details | .github/workflows/test17.yml:45:30:45:88 | fromJson(steps.get-pr-details.outputs.data).head.ref | provenance | | +| .github/workflows/test17.yml:49:13:55:10 | Uses Step: issues | .github/workflows/test17.yml:56:22:56:53 | steps.issues.outputs.data | provenance | | +| .github/workflows/test17.yml:60:13:68:10 | Uses Step: get-pull-request | .github/workflows/test17.yml:69:13:71:55 | fromJson(steps.get-pull-request.outputs.data).title | provenance | | | .github/workflows/test.yml:11:7:13:4 | Job outputs node [job_output] | .github/workflows/test.yml:52:20:52:56 | needs.job1.outputs['job_output'] | provenance | | | .github/workflows/test.yml:11:20:11:50 | steps.step5.outputs.MSG5 | .github/workflows/test.yml:11:7:13:4 | Job outputs node [job_output] | provenance | | | .github/workflows/test.yml:17:9:23:6 | Uses Step: step0 [value] | .github/workflows/test.yml:25:18:25:48 | steps.step0.outputs.value | provenance | | @@ -472,6 +476,14 @@ nodes | .github/workflows/test16.yml:215:19:230:24 | github.event.workflow_run.head_commit.author.name | semmle.label | github.event.workflow_run.head_commit.author.name | | .github/workflows/test16.yml:215:19:230:24 | needs.build-demo.outputs.commit-message | semmle.label | needs.build-demo.outputs.commit-message | | .github/workflows/test16.yml:215:19:230:24 | needs.setup.outputs.ref | semmle.label | needs.setup.outputs.ref | +| .github/workflows/test17.yml:14:13:22:10 | Uses Step: get-pr | semmle.label | Uses Step: get-pr | +| .github/workflows/test17.yml:25:41:25:72 | steps.get-pr.outputs.data | semmle.label | steps.get-pr.outputs.data | +| .github/workflows/test17.yml:30:13:39:10 | Uses Step: get-pr-details | semmle.label | Uses Step: get-pr-details | +| .github/workflows/test17.yml:45:30:45:88 | fromJson(steps.get-pr-details.outputs.data).head.ref | semmle.label | fromJson(steps.get-pr-details.outputs.data).head.ref | +| .github/workflows/test17.yml:49:13:55:10 | Uses Step: issues | semmle.label | Uses Step: issues | +| .github/workflows/test17.yml:56:22:56:53 | steps.issues.outputs.data | semmle.label | steps.issues.outputs.data | +| .github/workflows/test17.yml:60:13:68:10 | Uses Step: get-pull-request | semmle.label | Uses Step: get-pull-request | +| .github/workflows/test17.yml:69:13:71:55 | fromJson(steps.get-pull-request.outputs.data).title | semmle.label | fromJson(steps.get-pull-request.outputs.data).title | | .github/workflows/test.yml:11:7:13:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] | | .github/workflows/test.yml:11:20:11:50 | steps.step5.outputs.MSG5 | semmle.label | steps.step5.outputs.MSG5 | | .github/workflows/test.yml:17:9:23:6 | Uses Step: step0 [value] | semmle.label | Uses Step: step0 [value] | @@ -623,6 +635,10 @@ subpaths | .github/workflows/test16.yml:215:19:230:24 | needs.build-demo.outputs.commit-message | .github/workflows/test16.yml:125:20:125:75 | echo "value=$(git log -1 --pretty=%s)" >> $GITHUB_OUTPUT | .github/workflows/test16.yml:215:19:230:24 | needs.build-demo.outputs.commit-message | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/test16.yml:215:19:230:24 | needs.build-demo.outputs.commit-message | ${{ needs.build-demo.outputs.commit-message }} | | .github/workflows/test16.yml:215:19:230:24 | needs.setup.outputs.ref | .github/workflows/test16.yml:26:15:33:12 | Uses Step | .github/workflows/test16.yml:215:19:230:24 | needs.setup.outputs.ref | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/test16.yml:215:19:230:24 | needs.setup.outputs.ref | ${{ needs.setup.outputs.ref }} | | .github/workflows/test16.yml:215:19:230:24 | needs.setup.outputs.ref | .github/workflows/test16.yml:38:15:45:12 | Uses Step | .github/workflows/test16.yml:215:19:230:24 | needs.setup.outputs.ref | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/test16.yml:215:19:230:24 | needs.setup.outputs.ref | ${{ needs.setup.outputs.ref }} | +| .github/workflows/test17.yml:25:41:25:72 | steps.get-pr.outputs.data | .github/workflows/test17.yml:14:13:22:10 | Uses Step: get-pr | .github/workflows/test17.yml:25:41:25:72 | steps.get-pr.outputs.data | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/test17.yml:25:41:25:72 | steps.get-pr.outputs.data | ${{ steps.get-pr.outputs.data }} | +| .github/workflows/test17.yml:45:30:45:88 | fromJson(steps.get-pr-details.outputs.data).head.ref | .github/workflows/test17.yml:30:13:39:10 | Uses Step: get-pr-details | .github/workflows/test17.yml:45:30:45:88 | fromJson(steps.get-pr-details.outputs.data).head.ref | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/test17.yml:45:30:45:88 | fromJson(steps.get-pr-details.outputs.data).head.ref | ${{ fromJson(steps.get-pr-details.outputs.data).head.ref }} | +| .github/workflows/test17.yml:56:22:56:53 | steps.issues.outputs.data | .github/workflows/test17.yml:49:13:55:10 | Uses Step: issues | .github/workflows/test17.yml:56:22:56:53 | steps.issues.outputs.data | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/test17.yml:56:22:56:53 | steps.issues.outputs.data | ${{ steps.issues.outputs.data }} | +| .github/workflows/test17.yml:69:13:71:55 | fromJson(steps.get-pull-request.outputs.data).title | .github/workflows/test17.yml:60:13:68:10 | Uses Step: get-pull-request | .github/workflows/test17.yml:69:13:71:55 | fromJson(steps.get-pull-request.outputs.data).title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/test17.yml:69:13:71:55 | fromJson(steps.get-pull-request.outputs.data).title | ${{ fromJson(steps.get-pull-request.outputs.data).title }} | | .github/workflows/test.yml:52:20:52:56 | needs.job1.outputs['job_output'] | .github/workflows/test.yml:20:20:20:62 | github.event['pull_request']['body'] | .github/workflows/test.yml:52:20:52:56 | needs.job1.outputs['job_output'] | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/test.yml:52:20:52:56 | needs.job1.outputs['job_output'] | ${{needs.job1.outputs['job_output']}} | | .github/workflows/untrusted_checkout1.yml:15:20:15:58 | steps.artifact.outputs.pr_number | .github/workflows/untrusted_checkout1.yml:8:9:11:6 | Uses Step | .github/workflows/untrusted_checkout1.yml:15:20:15:58 | steps.artifact.outputs.pr_number | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/untrusted_checkout1.yml:15:20:15:58 | steps.artifact.outputs.pr_number | ${{ steps.artifact.outputs.pr_number }} | | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | ${{ github.event.workflow_run.display_title }} | diff --git a/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected b/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected index 6d33d3cc569..eb852fdd4d2 100644 --- a/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected +++ b/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected @@ -155,6 +155,10 @@ edges | .github/workflows/test16.yml:100:30:100:70 | steps.commit-message.outputs.value | .github/workflows/test16.yml:99:13:102:8 | Job outputs node [commit-message] | provenance | | | .github/workflows/test16.yml:123:15:128:12 | Run Step: commit-message [value] | .github/workflows/test16.yml:100:30:100:70 | steps.commit-message.outputs.value | provenance | | | .github/workflows/test16.yml:125:20:125:75 | echo "value=$(git log -1 --pretty=%s)" >> $GITHUB_OUTPUT | .github/workflows/test16.yml:123:15:128:12 | Run Step: commit-message [value] | provenance | | +| .github/workflows/test17.yml:14:13:22:10 | Uses Step: get-pr | .github/workflows/test17.yml:25:41:25:72 | steps.get-pr.outputs.data | provenance | | +| .github/workflows/test17.yml:30:13:39:10 | Uses Step: get-pr-details | .github/workflows/test17.yml:45:30:45:88 | fromJson(steps.get-pr-details.outputs.data).head.ref | provenance | | +| .github/workflows/test17.yml:49:13:55:10 | Uses Step: issues | .github/workflows/test17.yml:56:22:56:53 | steps.issues.outputs.data | provenance | | +| .github/workflows/test17.yml:60:13:68:10 | Uses Step: get-pull-request | .github/workflows/test17.yml:69:13:71:55 | fromJson(steps.get-pull-request.outputs.data).title | provenance | | | .github/workflows/test.yml:11:7:13:4 | Job outputs node [job_output] | .github/workflows/test.yml:52:20:52:56 | needs.job1.outputs['job_output'] | provenance | | | .github/workflows/test.yml:11:20:11:50 | steps.step5.outputs.MSG5 | .github/workflows/test.yml:11:7:13:4 | Job outputs node [job_output] | provenance | | | .github/workflows/test.yml:17:9:23:6 | Uses Step: step0 [value] | .github/workflows/test.yml:25:18:25:48 | steps.step0.outputs.value | provenance | | @@ -472,6 +476,14 @@ nodes | .github/workflows/test16.yml:215:19:230:24 | github.event.workflow_run.head_commit.author.name | semmle.label | github.event.workflow_run.head_commit.author.name | | .github/workflows/test16.yml:215:19:230:24 | needs.build-demo.outputs.commit-message | semmle.label | needs.build-demo.outputs.commit-message | | .github/workflows/test16.yml:215:19:230:24 | needs.setup.outputs.ref | semmle.label | needs.setup.outputs.ref | +| .github/workflows/test17.yml:14:13:22:10 | Uses Step: get-pr | semmle.label | Uses Step: get-pr | +| .github/workflows/test17.yml:25:41:25:72 | steps.get-pr.outputs.data | semmle.label | steps.get-pr.outputs.data | +| .github/workflows/test17.yml:30:13:39:10 | Uses Step: get-pr-details | semmle.label | Uses Step: get-pr-details | +| .github/workflows/test17.yml:45:30:45:88 | fromJson(steps.get-pr-details.outputs.data).head.ref | semmle.label | fromJson(steps.get-pr-details.outputs.data).head.ref | +| .github/workflows/test17.yml:49:13:55:10 | Uses Step: issues | semmle.label | Uses Step: issues | +| .github/workflows/test17.yml:56:22:56:53 | steps.issues.outputs.data | semmle.label | steps.issues.outputs.data | +| .github/workflows/test17.yml:60:13:68:10 | Uses Step: get-pull-request | semmle.label | Uses Step: get-pull-request | +| .github/workflows/test17.yml:69:13:71:55 | fromJson(steps.get-pull-request.outputs.data).title | semmle.label | fromJson(steps.get-pull-request.outputs.data).title | | .github/workflows/test.yml:11:7:13:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] | | .github/workflows/test.yml:11:20:11:50 | steps.step5.outputs.MSG5 | semmle.label | steps.step5.outputs.MSG5 | | .github/workflows/test.yml:17:9:23:6 | Uses Step: step0 [value] | semmle.label | Uses Step: step0 [value] |