Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: New check "Secure runners" #3801

Open
pnacht opened this issue Jan 17, 2024 · 3 comments
Open

Feature: New check "Secure runners" #3801

pnacht opened this issue Jan 17, 2024 · 3 comments
Labels
good first issue Good for newcomers kind/enhancement New feature or request kind/new-check New check for scorecard Stale

Comments

@pnacht
Copy link
Contributor

pnacht commented Jan 17, 2024

Is your feature request related to a problem? Please describe.
GitHub allows workflows to run either on their own runners or on "self-hosted" runners, which are entirely controlled by the maintainer.

While GitHub's runners have implicit security guarantees such as ephemerality, self-hosted runners have no such guarantees. If maintainers set up a "trivial" self-hosted runner, it likely won't be ephemeral or adequately sandboxed. As such, a malicious PR may be able to poison the runner.

The damage from a poisoned runner depends on what it's used for: if it is used for workflows with credentials (including a not-read-only GITHUB_TOKEN), they can be stolen. If it is used for release workflows, it can modify the release artifact, generating a malicious artifact from seemingly-safe source code. Even if it is only used for testing PRs with no credentials, it can falsify test results, hiding a nuanced vulnerability that could be detected by tests but possibly not by distracted humans.

Describe the solution you'd like
A new check that evaluates whether self-hosted runners are exposed to pull-requests, with scoring something like:

  • 10: No self-hosted runners
  • 5: self-hosted runners exposed to pull-requests, but are only used in non-privileged workflows (no secrets, no permissions), so can "only" be used to modify check results
  • 0: self-hosted runners exposed to pull-requests and also used in privileged workflows. The privileged workflows may be the same file (i.e. pull-request-target using secrets or without restricted permissions; or the workflow has multiple triggers beyond pull-request) or a different workflow that uses the same runner (identified by the runner labels).

Remediation would be basically "make sure you really need a self-hosted runner; if you're using it because setup takes too long on GH runners, consider using a GitHub runner with a docker image with the setup done, [etc]". However, there are valid reasons to use a self-hosted runner (i.e. GitHub runners don't use AARCH64), so there may be cases that can't be "solved" other than using a (TBD) maintainer annotation.

Describe alternatives you've considered

  • Embed in Dangerous-Workflow: Alternatively, something like this can be added to the Dangerous-Workflow check.

  • If we want to avoid penalizing projects for using self-hosted runners with no remediation for the valid use-cases with proper precautions, we can also give a -1 instead. So projects are rewarded (10/10) for avoiding self-hosted runners, but aren't actively penalized for using them.

  • The proposal above only penalizes projects for using self-hosted runners in privileged workflows if the same runner is exposed to PRs. The proposed way of identifying the same runner is through the runner labels (i.e. runs-on: [self-hosted, testing] vs. runs-on: [self-hosted, release]). This logic, however, is flawed: maintainers could give the same runner multiple labels, such that everything runs on the same runner even if the labels don't match. This is simply a rough approximation by assuming that different labels imply different runners.

    It's a false-positive/negative trade-off: if we want to make sure to catch all dangerous situations, we should penalize projects for using runners on PRs and privileged workflows, regardless of labels. This will ensure a 0% false-negative rate, but a "large" false-positive rate. The proposal is a compromise that will likely lead to non-zero false-negative and false-positive rates.

@pnacht pnacht added the kind/enhancement New feature or request label Jan 17, 2024
@spencerschrock spencerschrock added the kind/new-check New check for scorecard label Jan 17, 2024
@afmarcum afmarcum moved this to Backlog - Checks in Scorecard - NEW Mar 5, 2024
Copy link

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

@github-actions github-actions bot added the Stale label Mar 21, 2024
@raghavkaul raghavkaul added the good first issue Good for newcomers label Apr 15, 2024
@rakshitgondwal
Copy link

Hey folks, is this open for grabs? If yes, I'd like to work on this.

@spencerschrock
Copy link
Member

@rakshitgondwal I think some discussion would still be needed for a check with scoring. But generally we have no problem accepting probes. I would recommend chatting with @raghavkaul or @pnacht on what sort of probe would make sense in this case.

#4052 was merged last week to enable these sort of analyses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers kind/enhancement New feature or request kind/new-check New check for scorecard Stale
Projects
Status: Backlog - New Checks
Development

No branches or pull requests

4 participants