How would we define / detect privilege?

How would we define / detect privilege?

Could that be left as an exercise for the repo owner deploying Scorecard? Or make an assumption one way or the other with an allowlist/denylist?

One indicator of privilege is any action in workflow-b uses a secret (e.g. GITHUB_TOKEN). Even if we matched whether secrets.* passed to any step that's part of target workflow, I think we'd mostly catch writing to the main branch/writing releases.

Question: if workflow_run sets branches: [main], will it still run if workflow_a is modified to run on a PR?

Basically, does it consider that workflow_a is running on the PR branch or the main branch?

I'm in doubt because I never fully understood which commit is used when CI runs on a PR – it's not the PR's HEAD, but the merge commit between the PR and main, or something like that.

If setting branches: [main] (or a glob) blocks the workflow from running on PRs, that'd be a simple enough remediation.

Scorecard already checks for workflow_run trigger + RCE https://github.com/ossf/scorecard/blob/main/checks/raw/dangerous_workflow.go#L62.

The question here isn't so much RCE, but getting a workflow_run to trigger at an unexpected time.

Fleshing out the example given above, let's say we have a repository that follows Continuous Delivery, where all commits to main are immediately released. It has two workflows:

• one that builds and uploads a release artifact as a workflow artifact and then runs tests to ensure main isn't broken
• one that downloads the workflow artifact and publishes it

# build-and-test.yml
on:
  push:

jobs:
  test:
    steps:
      - # ... build ...
      # upload artifact for release or manual debugging if necessary
      - uses: actions/upload-artifact
        with:
          - artifact-name: release-artifact
      - run: ./test.sh

and

# release.yml
on:
  workflow-run: [build-and-test]
  types: [completed]
  branches: [main]

jobs:
  release:
    if: ${{ github.event.workflow_run.conclusion == 'success' }}
    # ...
    steps:
      # ...
      - run: ./download_artifact
        env:
          WORKFLOW_RUN_ID: ${{github.event.workflow_run.id }}
      - run: ./release.sh

If a PR is sent that changes build-and-test to also run on: pull-request, then build-and-test will run on that very same PR and release.yml will download its artifact and publish it.

So release.yml itself is doing exactly what it's meant to do. However, it has simply been "tricked" into running in a different context than expected.

As for my previous comment, I did some digging and this does indeed seem to work, even though release.yml sets branches: [main]. This is very weird to me and honestly feels like a bug...

Yes, precisely. The problem is what happens when the PR described in Raghav's original post is sent:

  # build-and-test.yml 
  on:
    push:
      branches: [ main ]
+   pull_request:
+     branches: [ main ]

When such a PR is sent, build-and-test.yml will run on that PR, and release.yml will be triggered and operate on the artifact sent by an untrusted source.

Thanks, I get it now. Great finding. Essentially it does not keep track of the original context at the main branch; if the workflow is defined to run on the main branch, it should only be run in response to other workflows defined on the same branch. Everything in the on should be used as context I suppose. This seems like a security vuln to me, because it violates the trusted context / branch. The doc say this trigger runs in the context of the HEAD, and I think that's a security decision like for pull_request_target, ie that you can trust what you run (not just the source of the second workflow, but the context of the caller, ie the source code of the first workflow).

@raghavkaul did you see this pattern used in several repos?

@josepalafox @steiza what do you think?

I think that today if you're a public repository you need to choose between:

• having highly privileged Action workflows in the same repository as the code you want the public to be able to contribute to
• not requiring approval to run Action workflows on pull requests for outside contributors

Today, if you do both, you are likely to be bitten by a scenario like what is listed above.

I realize this is not ideal! This is something we're aware of and thinking about, but I don't have a better solution to offer you today.

Hey @steiza, thanks for replying. Just to clarify, though: is the fact that workflow_run: with branches set runs on unmerged PRs intended behavior?

That is, is the fact that the workflow below runs when the triggering workflows runs on a new PR "working as intended"?

on:
  workflow_run:
    workflows: [Run Tests on PRs and Main]
    branches: [main]

In my opinion, that's quite unintuitive and should likely be made clear in the docs.

Oh, maybe this is a syntax/indentation issue?

Again, I want to re-iterate that (as illustrated by this thread) you have to be very careful when relying on Actions syntax to secure access to highly privileged Actions workflows. If you have a public repository with privileged workflows, I would strongly recommend enabling "Require approval for all outside collaborators".

That said, I was not able to reproduce what you described here. Here were my two workflows:

name: ci
on:
  workflow_dispatch:
  push:

jobs:
  ci:
    runs-on: ubuntu-latest
    steps:
      - run: echo "running CI"

name: release
on:
  workflow_run:
    workflows: [ci]
    types: [completed]
    branches: [main]

jobs:
  release:
    runs-on: ubuntu-latest
    if: ${{ github.event.workflow_run.conclusion == 'success' }}
    steps:
      - run: echo "running release"

In a non-main branch I added this diff:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index a8956a7..687a32a 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -2,6 +2,8 @@ name: ci
 on:
   workflow_dispatch:
   push:
+  pull_request:
+    branches: [main]
 

And then I pushed (which was enough to trigger ci but not release) and also made a pull request (which again triggered ci but not release).

The indentation is really important here! At the top of this issue you listed:

on:
  workflow_run:
    workflows: [workflow-a]
    types: [completed]
    branches: [main]

... but then on #3861 (comment) you listed:

on:
  workflow-run: [build-and-test]
  types: [completed]
  branches: [main]

The first example will give you the protection you're looking for, but the second example will not apply the branches constraint to workflow-run, like you intended.

Hey @steiza, here's an MRE of the issue: https://github.com/pnacht/test-workflow-run-pr

This is a toy project with two workflows: workflow_run.yml and other.yml

name: workflow-run
on:
  workflow_run:
    workflows: [other]
    types: [completed]
    branches: [main]

# ...

name: other
on: push

# ...

Looking at the Actions tab, we can see that other causes workflow_run to run, as expected.

I sent a "local PR" that modified other.yml to run on: push: pnacht/test-workflow-run-pr#1

-on: push
+on: pull_request

Looking at the Actions tab, we can see that other runs, but workflow_run doesn't.

However, when Spencer creates a fork and sends an identical external PR directly from his main branch, we can see that other does trigger workflow_run:

So it seems like on.workflow_run.branches doesn't validate whether it's the "real" main (pnacht:main, in this example), or a fork's (spencer:main).

Oh, thank you @pnacht! What I missed earlier was that the pull request was coming from the fork network, not a branch in the originating repository. I'll escalate internally.

@steiza Any update on whether the branch field is supposed to match any branch on any repo with the specified name, or just the named branch from the source repository? The latter means this behavior can only be exploited if a contributor already has write/branch creation access.

I think it's meant to match only the branch on the source repo. But there's still logic we can add in scorecard to also test the part where the indentation makes it vulnerable to external users sending PR:

on:
  workflow-run: [build-and-test]
  types: [completed]
  branches: [main]

I don't know how feasible it'd be, but that seems like something that GitHub should perform while validating a workflow's schema, not something Scorecard should have to check...

I don't know how feasible it'd be, but that seems like something that GitHub should perform while validating a workflow's schema, not something Scorecard should have to check...

It's not unreasonable for Scorecard to check for this anyway though. Consider the case where a workflow_run trigger doesn't have a branches filter. There's no indentation mistake to catch, as it's simply not there. But adding a branches filter may be something we may want to recommend.

yes that was my thinking. It's fairly easy to use the filter at the wrong indentation level by mistake. Curious how many workflows are configured this way on GitHub :)

Consider the case where a workflow_run trigger doesn't have a branches filter. There's no indentation mistake to catch, as it's simply not there. But adding a branches filter may be something we may want to recommend.

Ah, agreed, checking whether workflow_run has a branches filter is pretty reasonable.

I just don't think Scorecard should also evaluate for the specific "got the indentation wrong" error, which seems like something GH should simply reject as an invalid workflow (since the on: block doesn't need to accept arbitrarily-named fields like env: or jobs:).

Consider the case where a workflow_run trigger doesn't have a branches filter. There's no indentation mistake to catch, as it's simply not there. But adding a branches filter may be something we may want to recommend.

Ah, agreed, checking whether workflow_run has a branches filter is pretty reasonable.

I just don't think Scorecard should also evaluate for the specific "got the indentation wrong" error, which seems like something GH should simply reject as an invalid workflow (since the on: block doesn't need to accept arbitrarily-named fields like env: or jobs:).

Sorry. I don't mean the workflow is invalid. I mean that indentation makes the workflow vulnerable:

on:
  workflow_run:
    workflows: [Run Tests on PRs and Main]
  branches: [main] // Or no branch at all

instead of:

on:
  workflow_run:
    workflows: [Run Tests on PRs and Main]
    branches: [main]

Yes, I understand. I simply believe that the first case should cause the workflow to crash, with GH rejecting the workflow as invalid; the on: block's schema should reject a branches field (or any other field other than those it actually recognizes: push, pull_request, pull_request_target, etc).

@steiza Any update on whether the branch field is supposed to match any branch on any repo with the specified name, or just the named branch from the source repository? The latter means this behavior can only be exploited if a contributor already has write/branch creation access.

The branch field should be for the named branch in the source repository - not a branch on any repository in the fork network.

Consider the case where a workflow_run trigger doesn't have a branches filter. There's no indentation mistake to catch, as it's simply not there. But adding a branches filter may be something we may want to recommend.

Ah, agreed, checking whether workflow_run has a branches filter is pretty reasonable.

I just don't think Scorecard should also evaluate for the specific "got the indentation wrong" error, which seems like something GH should simply reject as an invalid workflow (since the on: block doesn't need to accept arbitrarily-named fields like env: or jobs:).

as far as our parser is concerned these are the same thing though. But yes for anyone not using Scorecard that's another story

Not necessarily. Scorecard's parser can simply care about whether on.workflow_run.branches exists. If it doesn't, then the parser doesn't need to check whether on.branches exists (the workflow is vulnerable regardless - we'd maybe just slightly change the remediation advice).

That being said, there's also the issue of whether we believe GitHub will actually make the workflow fail if it doesn't follow on:'s schema. If we don't believe that'll happen, then it arguably makes sense for Scorecard to check for this sort of mistake as a band-aid.

Not necessarily. Scorecard's parser can simply care about whether on.workflow_run.branches exists. If it doesn't, then the parser doesn't need to check whether on.branches exists (the workflow is vulnerable regardless).

I just meant that not having a branches filter and having the wrong indentation are the same thing if all we're doing is checking if on.workflow_run.branches exists. I dont think we need to have a special message/check for indentation, although some people may be confused if they think its a false positive