Open wlynch opened 1 year ago
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 :)
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?
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.
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.
Given GitHub's interest in the Dangerous-Workflow and Pinned-Dependency checks (https://github.com/ossf/scorecard-action/issues/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.
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 https://github.com/systemd/systemd/pull/27329#issuecomment-1513138205 (in that particular case it wasn't malicious or anything like that) but stuff like that should be detected.
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.
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.
Stale issue message - this issue will be closed in 7 days
This issue is stale because it has been open for 60 days with no activity.
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 https://github.com/stefanbuck/github-issue-parser/issues/48#issuecomment-2028919552 but it would be nice if scorecard was able to flag things like that.
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.
This issue has been marked stale because it has been open for 60 days with no activity.
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 theUntrusted 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
Additional context
I'm happy to contribute this!
WIP branch already started here - https://github.com/ossf/scorecard/compare/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.