Resolve false positives with Dangerous-Workflow check

[asraa]

Describe the bug See https://github.com/ossf/scorecard/pull/1283#issuecomment-974227644

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:

Expected behavior A clear and concise description of what you expected to happen.

Additional context Add any other context about the problem here.

[laurentsimon]

how about the scenario where ref is used and permissions are set to XXX:write but XXX is not contents or packages?

[asraa]

how about the scenario where ref is used and permissions are set to XXX:write but XXX is not contents or packages?

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

[laurentsimon]

you're right, absolutely. I think I was assuming there's no secrets besides the default (permission-restricted) GitHub secret used in the PR.

[ristomcgehee]

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.

[laurentsimon]

spot on, we got an alert in the scanning dashboard now https://github.com/ossf/scorecard/blob/58865e959e3a782d3f3cd5a5ae952ac308c11a46/.github/workflows/integration.yml#L35-L35

@chrismcgehee would looking for needs field be enough to reduce false positives in this case? Or would it introduce false negatives?

[artis3n]

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.

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 pull_request_target and checking out the pr ref, but it is gated by an Environment that requires a manual approval trigger before running which should be regarded a safe way to do this.

[laurentsimon]

this should not be a very one to fix. Interested in giving it a try?

[artis3n]

Would that be around https://github.com/ossf/scorecard/blob/main/checks/raw/dangerous_workflow.go#L164 - check if environment: is present in job, and if so, check repo settings for a required reviewer protection rule on the environment?

[artis3n]

I don't believe repo environments are exposed in the REST or graphql apis yet, which may preclude writing a check.

[laurentsimon]

You should be able to use https://pkg.go.dev/github.com/rhysd/actionlint#Environment API to retrieve the environment.

[artis3n]

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.

https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment#required-reviewers

[laurentsimon]

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.

[artis3n]

: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.