taiki-e / install-action

GitHub Action for installing development tools (mainly from GitHub Releases).
Apache License 2.0
245 stars 33 forks source link

Catching "xz" style problems #488

Open jayvdb opened 1 month ago

jayvdb commented 1 month ago

https://en.wikipedia.org/wiki/XZ_Utils_backdoor for context.

install-action verifies checksums & signatures, to detect edits to old releases. The readme explains this, but it doesnt clearly describe what happens if that verification fails. What if I am installing 4 tools using this action, and one fails verification? Does/Should it fall back to a prior release, if it can find one which does pass verification?

Going one step further, we could detect when the identity of the signer/creator of a release changes from the identity of previous releases, and not include those releases until some manual verification has occurred.

https://github.com/taiki-e/install-action/pull/182 would have detected yanked crates while running this action - i.e. during installation.

Given a cron schedule of updating manifests every three hours per https://github.com/taiki-e/install-action/blob/main/.github/workflows/ci.yml#L13

One way to avoid the urgent yanking type problem is to have two streams for this action, @latest / @main , and @stable/@v2 . Then for stable users of this action, the action would not install the latest version of a crate/dep until 12 hrs after it was released, allowing plenty of time for maintainers to yank the release.

We could also use semver to further delay minor and major releases being used by @stable; e.g. a few days for minor releases, and a week or two for major releases.

IMO we should be doing more indepth analysis of binaries during the creation of the manifests , i.e. doing active vulnerability scans on the binaries.

For Rust crates, we could be checking if binaries were built using https://github.com/rust-secure-code/cargo-auditable and adding a column to the tools markdown table to give these binaries a :heavy_check_mark: if they are auditable, and using the embedded SBOM to check for vulns. This type of functionality is going to be incorporated into cargo itself - currently https://github.com/rust-lang/rfcs/pull/3553 , so encouraging tool creators to use cargo-auditable will help surface problems that should be considered in the RFC .

For Go, iirc osv-scanner already detects vulns in the binaries.

jayvdb commented 1 month ago

https://github.com/cargo-bins/cargo-binstall/issues/1 also has some good thoughts on this, but didnt predict the xz vector, and that tool doesnt have previous state (like the manifests in install-action), so is limited in how they can approach the problem.

https://github.com/cargo-bins/cargo-binstall/issues/1683 and https://github.com/cargo-bins/cargo-binstall/issues/1425 and other signing and verification mechanisms are all useful, but missing the point of "xz" - binaries cant be trust simply because provenance to the maintainer has been verified.

NobodyXu commented 1 month ago

For binstall, honestly, I'm not sure how to prevent malware when the maintainer is malicious.

cargo-binstall is meant to be a dropped in replacement of cargo-install, and it doesn't have much mechanism to avoid malware.

Maybe crates-io can forcibly yank malware release, but currently it doesn't even sandbox proc-macro and build-script.

jayvdb commented 1 month ago

I explained a few ways that we can make this action be more aware of questionable behaviour from upstreams, and either block the newer versions of upstream, or at least not install them if the user is requesting a more "stable" set of tools rather than putting their CI on the bleeding edge.

taiki-e commented 1 month ago

First, the xz problem is really difficult to deal with because it is a very elaborate attack by someone who has spent a long time getting trust and becoming the maintainer. It is a problem that all Linux distro packagers and Homebrew maintainers failed to handle well until reported. Realistically, we need to start thinking in more simple cases.

Also, in the xz problem, GitHub first blocked the affected repos and eventually removed the affected GitHub releases. This action also benefits from such a security response by GitHub, except when binstall is used to installation and a non-GitHub-release URL is specified in binstall config in Cargo.toml of the tool.

And since the update mechanism for this action is basically similar to popular package managers like homebrew cask and scoop (as said in https://github.com/taiki-e/install-action/pull/182#issuecomment-1665600245, https://github.com/taiki-e/install-action/issues/348#issuecomment-1909197273), I think it would make sense for a reasonable discussion here to know what security they can or cannot provide.

See also https://github.com/taiki-e/install-action/issues/1 for some of what the current security logics of this action can and cannot address.


install-action verifies checksums & signatures, to detect edits to old releases. The readme explains this, but it doesnt clearly describe what happens if that verification fails. What if I am installing 4 tools using this action, and one fails verification? Does/Should it fall back to a prior release, if it can find one which does pass verification?

Currently, it simply fails, but I think a fallback to the older version would be better. I think this could be implemented by extracting some of the https://github.com/taiki-e/install-action/pull/182 implementation.

FYI, https://github.com/taiki-e/install-action/issues/167#issuecomment-1653516441 is an actual case where the maintainer has edited the release in the past. In this case I tracked the maintainer's activity and determined that it was ok, so I created a new release with the updated hash. Now it is a bit more automated and the hash updates are automated, but the release itself is still something I do manually.


Going one step further, we could detect when the identity of the signer/creator of a release changes from the identity of previous releases, and not include those releases until some manual verification has occurred.

I think this is potentially useful in some cases, that said...

See also https://github.com/taiki-e/install-action/issues/1.


IMO we should be doing more indepth analysis of binaries during the creation of the manifests , i.e. doing active vulnerability scans on the binaries.

For Rust crates, we could be checking if binaries were built using https://github.com/rust-secure-code/cargo-auditable and adding a column to the tools markdown table to give these binaries a ✔️ if they are auditable, and using the embedded SBOM to check for vulns. This type of functionality is going to be incorporated into cargo itself - currently rust-lang/rfcs#3553 , so encouraging tool creators to use cargo-auditable will help surface problems that should be considered in the RFC .

For Go, iirc osv-scanner already detects vulns in the binaries.

This is a reasonable idea, but we probably need to think how we should evaluate vulnerabilities when they are discovered. Especially with Rust, there is a tendency for advisories to be submitted for completely unimportant issues...

NobodyXu commented 1 month ago

I explained a few ways that we can make this action be more aware of questionable behaviour from upstreams

Sorry I was talking about cargo-binstall.

Just don't think there's more we can do other than signing.

jayvdb commented 1 month ago

The main proposal is to have two modes of this action. One that installs the latest versions of tools, and one that installs more "stable" versions of tools, where stable means more than just semver - it also means our heuristics about the binary itself indicate there are no red flags, and how much time has elapsed since the release was done.

Rather than using two branches of this action (initial proposal above), which feels like more work and messy merging across the two branches, maybe a new input would be better.

Name: "channel" / "stream" ? Values: latest vs stable.

Or a bool input of "stable", default true?

Or the tools input supporting foo@stable,bar@latest, and switching to @stable being the default.

Once this action has an ability to choose more stable versions, the metadata for newer less trusted versions can be committed into the manifests and released automatically, as these wont be so quickly consumed by action users.

Red flags can be stored in the manifests, and the cron job can auto-create github issues to be investigated by the team here.

This repo does have a limited scope. The list of tools isnt large, so we can keep a close eye on them and it sure seems like @taiki-e you already are a great investigator. Given that these tools are quite important ones, anonalies are worth investigaying. And I'm putting up my hand to help. Prior recent experience was figuring out the real identity of a new maintainer https://github.com/zip-rs/zip-old/issues/446#issuecomment-2075724338 , which has had a turbulent period recently cf https://github.com/rustsec/advisory-db/issues/1956