Audit rule: Pinned commit hash not pointing to tag

[Marcono1234]

(I am creating this as Discussion because I am not sure if this is worth an audit rule; but feel free to convert this to an Issue if you want.)

Related:

• Feature: detect mismatches between pinned full-length commit SHA and version tag #643
• Existing impostor-commit and known-vulnerable-actions audit rules

Problem

An action pinned by commit hash could point to a commit which is not a tag. That could be a problem because:

• the commit might be an incomplete / work in progress commit which contains bugs or vulnerabilities
• the commit might contain a bug or vulnerability which was fixed before the subsequent release

In both cases there will likely not be a security advisory because that broken / vulnerable commit was never part of a release.

So there are two cases where this could become a problem:

• you use that commit because it contains a not yet released bug fix / feature you are waiting for, and then you forget to update the reference to the subsequent release (and don't use Dependabot or similar)
• a malicious user tries to trick you into downgrading to a vulnerable version (intermediate commit which was never released)

Suggested audit rule

The audit rule should check if an action is pinned by commit hash, and if the hash does not point to a Git tag, emit a finding.

[woodruffw]

Hey @Marcono1234, thanks for filing this!

This is an interesting idea, but it's got a few challenges:

1. For better or worse, a lot of actions do use Git objects that aren't tags. pypa/gh-action-pypi-publish for example uses the release/v1 branch as its "rolling v1" reference, which this audit would flag as suspicious.
2. Git's own data model here makes things funky: there are commit objects that correspond to a tag, but there are also tag object SHAs that can be distinct from the commit object's SHA while pointing to the same state in history. I'm not sure whether Git only allows a fixed number of these, or whether they're conceptually unbounded.

TL;DR: I think this would be a good "pedantic" or "auditor"-only audit, but it'd be hard to expose by default since there will be a decent amount of real-world innocent noise that'll trip it up 🙂

[woodruffw]

Or do you think users use the SHA of arbitrary / the latest commits on release/v1?

I think this ends up happening -- a common thing is that someone will do @release/v1, hash-pin it automatically with something like frizbee, and then have Dependabot or Renovate periodically update that hash. So we'd end up flagging that hash during the window between updates, which might cause a lot of noise for users (effectively defeating the point of their longer update cycle schedules).

Ah, that would be similar to #151, right? Based on your comments there, it sounds as if it would be possible to detect that the SHA points to a Git tag though?

Yep, exactly 🙂 -- I haven't had the time to revisit it, but I would really appreciate someone looking into that and seeing how much complexity/variation in object types is possible there.

Sorry also for all my recent Issues, Discussions and comments. If you consider at any point my actions to be disruptive or excessive, please let me know.

No problem at all! Multiple tracks make me slower to respond, but I appreciate all the detail and thought you've put into each so far.

[Marcono1234]

Given the risk of false positives, do you think this would however still be useful as 'pedantic' or 'auditor' audit rule?

If you want I can give it a try.

[woodruffw]

Given the risk of false positives, do you think this would however still be useful as 'pedantic' or 'auditor' audit rule?

I think so! 'pedantic' makes sense to me.

If you want I can give it a try.

Please do! I think this dovetails somewhat with #643 as well -- they're different checks, but IMO both make sense under a new stale-refs or similar audit.

(Not that you have to do both, just that I'll likely home them under the same name eventually 🙂)

[Marcono1234]

Have created #713 now, but it does not include #643. I hope the audit name is ok like that, but feel free to adjust it.

[woodruffw]

Thanks! I'll do a review tonight most likely.

[woodruffw]

Done with #713. Thanks a ton @Marcono1234!