Skip to content
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

Catching "xz" style problems #488

Open
jayvdb opened this issue May 19, 2024 · 6 comments
Open

Catching "xz" style problems #488

jayvdb opened this issue May 19, 2024 · 6 comments

Comments

@jayvdb
Copy link
Contributor

jayvdb commented May 19, 2024

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.

#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 ✔️ 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.

@jayvdb
Copy link
Contributor Author

jayvdb commented May 19, 2024

cargo-bins/cargo-binstall#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.

cargo-bins/cargo-binstall#1683 and cargo-bins/cargo-binstall#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
Copy link
Collaborator

NobodyXu commented May 20, 2024

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
Copy link
Contributor Author

jayvdb commented May 20, 2024

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
Copy link
Owner

taiki-e commented May 20, 2024

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 #182 (comment), #348 (comment)), I think it would make sense for a reasonable discussion here to know what security they can or cannot provide.

See also #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 #182 implementation.

FYI, #167 (comment) 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...

  • How valid is it in projects where the release process is automated? (I guess the releaser would be the bot account or the provider of the PAT used for the release.)
  • The final conclusion will most likely be "the new releaser is a new maintainer trusted by the existing maintainer" as happened in xz. (Of course, we don't have time to neither audit changes to every project nor the extraordinary investigative capabilities to identify the identity behind the anonymity of the Internet.)

See also #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
Copy link
Collaborator

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
Copy link
Contributor Author

jayvdb commented May 20, 2024

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 zip-rs/zip-old#446 (comment) , which has had a turbulent period recently cf rustsec/advisory-db#1956

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants