-
Notifications
You must be signed in to change notification settings - Fork 485
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
Resolve false positives with Dangerous-Workflow check #1311
Comments
how about the scenario where |
maybe i'm missing something but isn't even read perms dangerous? PR checkout would still allow reading secrets, e.g. with some malicious script in the untrusted checkout |
you're right, absolutely. I think I was assuming there's no secrets besides the default (permission-restricted) GitHub secret used in the PR. |
I would say another instance that will likely be a false positive is when the job is gated by an environment (like in scorecard's integration.yml). If the environment requires reviews, then it's definitely a false positive. I expect, though, that we can only see environment protection rules with an admin API token. Maybe when we're not using an admin token and the dangerous job has an environment, we give it a high but not perfect score. When using an admin token, we can check the protection rules for the environment, give it a 0 if it doesn't require reviews, and give it a perfect score if it does. |
spot on, we got an alert in the scanning dashboard now
@chrismcgehee would looking for |
Yup, this is what I'm seeing with Scorecard and https://github.com/digitalocean/terraform-vault-github-oidc/blob/main/.github/workflows/pr_target.yml . Dangerous |
this should not be a very one to fix. Interested in giving it a try? |
Would that be around https://github.com/ossf/scorecard/blob/main/checks/raw/dangerous_workflow.go#L164 - check if |
I don't believe repo environments are exposed in the REST or graphql apis yet, which may preclude writing a check. |
You should be able to use https://pkg.go.dev/github.com/rhysd/actionlint#Environment API to retrieve the environment. |
That will just give you the name set on the environment. We have to take that name and query repo settings to determine if the required reviewer setting is enabled for that environment - only that configuration turns the dangerous workflow into a protected one. |
you're right. We could maybe assume that the presence of an environment is enough. But if we think this is dangerous, then maybe leaving it as a risk that users should manually check is the way to go. I'll create a feature request for GitHub for this. |
:nod: Yeah definitely worth being cautious and flagging a workflow as dangerous until we can prove that it is not. Assuming I had missed setting that check on my workflow linked above, I'd definitely expect scorecard to flag it to help me set the appropriate security control. |
Describe the bug
See #1283 (comment)
Dangerous-Workflow doesn't handle false positives where a
pull_request_target
is used with untrusted code checkout BUT token permissions are set and environment protection rules or gating labels are set.https://dev.to/petrsvihlik/using-environment-protection-rules-to-secure-secrets-when-building-external-forks-with-pullrequesttarget-hci
Reproduction steps
Steps to reproduce the behavior:
1.
2.
3.
4.
Expected behavior
A clear and concise description of what you expected to happen.
Additional context
Add any other context about the problem here.
The text was updated successfully, but these errors were encountered: