Feature: Dangerous Workflows - Imposter Commit Check

[wlynch]

Is your feature request related to a problem? Please describe.

We disclosed an issue with GitHub Actions today that lets users bypass workflow settings by using forked commits - https://www.chainguard.dev/unchained/what-the-fork-imposter-commits-in-github-actions-and-ci-cd

I'd like to contribute a check to Scorecards based on our proof-of-concept tool - https://github.com/chainguard-dev/clank

Describe the solution you'd like

I'd like to add an additional check to Dangerous-Workflows based on https://github.com/chainguard-dev/clank (this check is most similar to the Untrusted Code Checkout check).

It roughly works by inspecting each uses statement then seeing if the commit is reachable by a branch or tag in the repo.

Describe alternatives you've considered

• An early version of clank cloned the repo to a local tmp to avoid extra network calls. I ended up having some difficulty trying to handle untracked branches, but we can revisit this if the reachability queries end up being too costly.
• Tried experimenting with the GraphQL API to detect reachability, but had some difficulty forming a query that wasn't a form of "GraphQL get every reachable commit SHA then check if it's in the list". See https://github.com/orgs/community/discussions/24501 for related discussion. If anyone has ideas for how to get GraphQL to work in a reasonable way, lmk!

Additional context

I'm happy to contribute this!

WIP branch already started here - main...wlynch:scorecard:imposter-commits
Functionally, but need to add tests. Open to early feedback! 🙏

If anyone has any suggestions/examples for how to create sub-clients of a githubclient RepoClient, let me know. Wasn't sure how to tackle this for the mock clients in the tests, and I don't want to start refactoring clients without consulting maintainers first.

[azeemshaikh38]

Very interesting find! Thanks for bringing this to Scorecard and offering to contribute this check.

Agree that we should include this within Scorecard. I'm ok adding this to the Dangerous-Workflow check, although would this be a better fit within Pinned-Dependency check given how this exploit is a type of dependency confusion? Curious to hear your thoughts @ossf/scorecard-maintainers @wlynch.

An early version of clank cloned the repo to a local tmp to avoid extra network calls. I ended up having some difficulty trying to handle untracked branches, but we can revisit this if the reachability queries end up being too costly.

Checking for the commit/tag within just the default branch seems like a reasonable approximation to start with? Especially given that GitHub Actions typically are published using default branches and searching across branches would be very costly API-wise.

If anyone has any suggestions/examples for how to create sub-clients of a githubclient RepoClient, let me know.

Not sure what you mean by sub-clients. Could you explain what it is you are trying to do?

I'm happy to contribute this!

Thank you so much, that would be very much appreciated :)

[laurentsimon]

Super excited about this feature! For mock repo client, maybe https://github.com/ossf/scorecard/blob/main/checks/raw/branch_protection_test.go#L259-L284 can help?

[wlynch]

Checking for the commit/tag within just the default branch seems like a reasonable approximation to start with? Especially given that GitHub Actions typically are published using default branches and searching across branches would be very costly API-wise.

Starting with the default branch SGTM!

Not sure what you mean by sub-clients. Could you explain what it is you are trying to do?

So with this check, we need to query the reachability of commits in other repos. e.g. If I have a workflow in repo A with uses: b@abc123, we fetch the workflow content in repo A but then need to check the commit reachability in repo B.

AFAICT checks start with a RepoClient configured to repo A, so we need a way to create a new client configured for repo B, ideally just taking all the preconfigured settings that were already set from the first client (auth, host, etc.).

We can't re-call InitRepo, since that will mutate state for the entire shared client - we need a way to do the equivalent of InitRepo, but create a new client instance. My initial thoughts were to create a RepoClient.NewClient method that would handle this, but I wasn't sure if I missed something and if there is another way to do this already in scorecards, so I wanted to check in first.

[azeemshaikh38]

Sorry for dropping the ball on this @wlynch. There is no nice way of doing what you are looking for. Creating a new RepoClient within the check is ok for now. Hope that helps. Will look forward to the PR.

[spencerschrock]

Given GitHub's interest in the Dangerous-Workflow and Pinned-Dependency checks (ossf/scorecard-action#1107), I'm curious if there are plans / if we can request an API to query if a commit belongs to a repo similar to the banner they display in the web GUI

This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

[evverx]

I'm not sure why this issue stalled but I think the part of the "Pinned-Dependencies" check responsible for GHActions isn't particularly helpful without making sure that actions are pinned to the original repositories. One example where scorecard should have complained would be systemd/systemd#27329 (comment) (in that particular case it wasn't malicious or anything like that) but stuff like that should be detected.

[spencerschrock]

I believe the main blocker is the efficiency of the implementation, as it uses 2 API calls per action used which will negatively impact our weekly scan.

Perhaps the quick win here is to write it in a way that Scorecard / github action will check for it, but the check is disabled in the cron while we figure something out.

[evverx]

Got it.

I think GitHub shouldn't allow this in the first place (but assuming it can't be fully fixed because of backward compatibility there should be a way to opt-out of this). As far as I understand it isn't going to happen though given that as far as I can tell the only thing GitHub fixed in response to that report was the documentation.

[github-actions]

Stale issue message - this issue will be closed in 7 days

[github-actions]

This issue is stale because it has been open for 60 days with no activity.

[evverx]

FWIW right now one of the systemd GH Actions points to a commit that doesn't exists in the original action repository and somehow that commit came from Dependabot. I'm trying to figure out what it is in stefanbuck/github-issue-parser#48 (comment) but it would be nice if scorecard was able to flag things like that.

[spencerschrock]

FWIW right now one of the systemd GH Actions points to a commit that doesn't exists in the original action repository and somehow that commit came from Dependabot. I'm trying to figure out what it is in stefanbuck/github-issue-parser#48 (comment) but it would be nice if scorecard was able to flag things like that.

This is an interesting case. My first thought of where scorecard would catch/prevent is during a pull_request trigger (ignoring that we haven't prioritized that work for scorecard-action). But as you point out it likely was part of the repo when dependabot raised the PR, and then history was later changed.

[github-actions]

This issue has been marked stale because it has been open for 60 days with no activity.