ossf / scorecard

OpenSSF Scorecard - Security health metrics for Open Source
https://scorecard.dev
Apache License 2.0
4.58k stars 498 forks source link

BUG: High-risk write permissions only punished at top-level, low-risk permissions punished everywhere #3045

Closed pnacht closed 1 year ago

pnacht commented 1 year ago

Describe the bug Scorecard currently only punishes the high-risk permissions contents/packages/actions: write if they are at the top-level. At the job-level, a warning is shown but no score penalty is applied.

However, all the other lower-risk permissions (i.e. checks) apply a score penalty wherever they are.

Reproduction steps Steps to reproduce the behavior:

  1. See https://github.com/ossf/scorecard/blob/main/checks/evaluation/permissions/permissions.go#L325-L374
  2. Note that all low-risk permissions are checked with permissionIsPresent while high-risk permissions use permissionIsPresentInTopLevel.

Expected behavior The current situation is the opposite of what'd be expected: high-risk permissions should be punished more severely than low-risk permissions (or they should be punished equally).

So, I'd expect either:

  1. all write permissions are always punished, regardless of where they are defined. However, this goes against the conclusion in #2338.
  2. all write permissions should be forgiven at the job-level. This seems to be closest to the spirit of #2338.
  3. all permissions have two levels of punishment: a high value if set at the top-level, a low value if set at the job-level. (The values don't have to be the same for all permissions).

(2) seems closest to the spirit of #2338, but I'd also see value in (3), with something like:

permission Penalty
@ top-level
Penalty
@ job-level
High-risk (contents,
packages, actions)
-10 -1
Low-risk (other) -1 0

This would somewhat capture the risk of adding high-risk permissions even if only at the job-level (which could be further reduced to no loss if hash-pinned, as suggested by #3022).

pnacht commented 1 year ago

If there's interest in this, feel free to assign it to me. Unless someone suggests otherwise, I'd move forward with (2), forgiving all write permissions set at job level.

spencerschrock commented 1 year ago

Scorecard currently only punishes the high-risk permissions contents/packages/actions: write if they are at the top-level. At the job-level, a warning is shown but no score penalty is applied.

However, all the other lower-risk permissions (i.e. checks) apply a score penalty wherever they are.

Agree this is paradoxical, and came about because the higher-risk permissions were more broadly used (and therefore got more complaints about the penalty), leading to #2338 .

I have no problem with 2.

heitorlessa commented 1 year ago

Just hit this when implementing scorecard in our project: https://github.com/aws-powertools/powertools-lambda-python/blob/3f2498545a5f207bff7fc283956235d99858b306/.github/workflows/release-drafter.yml#L28

In this instance, we need contents: write only to create a release in draft mode and keep it up to date.

In other cases, we use contents: write to create temporary branches + create a PR from trusted workflows like building a changelog on every merged PR -- this allows us to keep a protected branch (trunk) and yet allow automation to keep us safe.