Skip to content

Import old Wasmtime security advisories #2254

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

Merged
merged 1 commit into from
May 2, 2025

Conversation

alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Mar 17, 2025

Wasmtime recently got a request to have our security advisories
published on the RustSec database as well. We've got a few old
advisories on here but we haven't been keeping up-to-date with later
advisories. In lieu of automatic imports from GitHub to RustSec we
figured we'd in the interim manually fill in some fields.

In this PR I'm back-filling security advisories we've had in Wasmtime
into the RustSec database here. The oldest advisory here is 3 years old
and the goal is to have this serve as a template for importing future
advisories that Wasmtime gets. It's not expected for this to cause any
churn or undue warnings but instead is intended to bring RustSec
up-to-date with the advisories we have for this crate.

@alexcrichton
Copy link
Contributor Author

For maintainers here, two questions:

  • Is there a way to import multiple advisories at once? Or should I make a separate PR-per-advisory for the initial backfill?
  • Is it ok to have the body of the advisory here in RustSec be basically empty and point to the "official" source which for us is the GitHub-hosted version?

@djc
Copy link
Contributor

djc commented Mar 22, 2025

IMO (as a recently joined maintainer):

For maintainers here, two questions:

  • Is there a way to import multiple advisories at once? Or should I make a separate PR-per-advisory for the initial backfill?

We don't need a PR-per-advisory, I'm fine with adding many of these in the same PR or even in the same commit.

  • Is it ok to have the body of the advisory here in RustSec be basically empty and point to the "official" source which for us is the GitHub-hosted version?

I think that's probably okay, as long as there's a decent title summarizing the issue. I'd suggest reducing the boilerplate:

This is an entry in the RustSec database for the Wasmtime security advisory located at GHSA-88xq-w8cq-xfg7. For more information see the GitHub-hosted security advisory.

Maybe like:

For more information, review the Wasmtime security advisory at GHSA-88xq-w8cq-xfg7.

@tarcieri
Copy link
Member

Is there a way to import multiple advisories at once? Or should I make a separate PR-per-advisory for the initial backfill?

@alexcrichton you can submit several advisories in a single PR, and the ID assigner will make a single PR to assign them all incrementing IDs

@alexcrichton
Copy link
Contributor Author

Ok thanks! When adding multiple advisories should I use RUSTSEC-0000-0000.XX.md as a naming scheme? It looked like the renamer specifically looked for "RUSTSEC-0000-0000" and otherwise discarded the filename.

Happy to include the renames, but I'm a bit hesitant to import too much if this will end up pinging many other contributors asking for licensing information. Is that something standard to require? Even for just a link to GHA? Is there something we can do to avoid the licensing back-and-forth?

@djc
Copy link
Contributor

djc commented Mar 27, 2025

Ok thanks! When adding multiple advisories should I use RUSTSEC-0000-0000.XX.md as a naming scheme? It looked like the renamer specifically looked for "RUSTSEC-0000-0000" and otherwise discarded the filename.

That sounds about right, let's try it?

Happy to include the renames, but I'm a bit hesitant to import too much if this will end up pinging many other contributors asking for licensing information. Is that something standard to require? Even for just a link to GHA? Is there something we can do to avoid the licensing back-and-forth?

The full guidance:

  • Either use only data published in the associated CVE, since the CVE database is in the public domain
  • Or ask the submitter of the GHSA advisory (not the RustSec pull request author) to release the advisory contents into the public domain

Are wasmtime vulnerabilities assigned a CVE? If so, using just the title + link should be fine assuming that the title is also used in the CVE. Otherwise, it looks like there are just 5 GHSA authors for wasmtime, so getting those releases should be fairly straightforward? (Also presumably if they were employed by a single organization at the time they submitted, it might be enough to get a copy right release from that organization?)

@alexcrichton
Copy link
Contributor Author

It'll take me some time to get around to filling out old advisories. I'm happy to have this merged as-is and I can follow-up with a later batch, but if y'all would prefer this can hold off on merging until I've imported other old advisories.

All Wasmtime vulnerabilities are assigned a CVE, and for example this advisory has a CVE listed in it which you can see on the GHSA as well. The CVE itself doesn't seem to be a 1:1 copy of what's on GHSA.

Basically if licensing is going to be a drag on this I think we'll probably skip the step of manually publishing here and wait for the auto-importing tool to work (or something like that). If we can sidestep licensing issues by having the report be a URL that's fine though from our end, the goal here is to just get notifications out to users who exclusively use cargo audit that a vulnerability has happened.

@tarcieri
Copy link
Member

@alexcrichton you can also add a license = "CC0-1.0" field to each advisory

@alexcrichton
Copy link
Contributor Author

Ok found some time on my hands and I've uploaded other advisories we've had. I've also added license = "CC0-1.0" to each entry

@alexcrichton
Copy link
Contributor Author

Ah alas the RUSTSEC-0000-0000.XX.md trick didn't work since then the filename doesn't match the id. In lieu of 16 PRs is it possible to merge without CI passing? Or should I trim this back to one advisory and send more PRs for other advisories?

@djc
Copy link
Contributor

djc commented Mar 29, 2025

We should probably make sure the underlying tools are aligned on their understanding of what constitutes a valid new advisory filename. I'll see if I can have a look at that sometime soon (or maybe you can make a PR to the rustsec repo?).

@tarcieri
Copy link
Member

To my knowledge it’s files named RUSTSEC-0000-0000.md

@djc
Copy link
Contributor

djc commented Mar 29, 2025

Here's a proposal to improve the tooling to make it possible to have multiple advisories for the same crate in one PR:

rustsec/rustsec#1351

@alexcrichton
Copy link
Contributor Author

I tweaked CI a bit to test out rustsec-admin from git and looks like it's green and working, so I'll drop those bits once a new version is published and update to that as well.

@djc
Copy link
Contributor

djc commented Apr 8, 2025

This needs to wait for rustsec-admin 0.8.10, which is blocked on getting a new rustsec release out.

@djc
Copy link
Contributor

djc commented May 1, 2025

@alexcrichton sorry for the delay -- the updated rustsec-admin should be integrated into the CI workflow here now, so can you rebase on top of current main?

[Wasmtime] recently got a [request] to have our security advisories
published on the RustSec database as well. We've got a few old
advisories on here but we haven't been keeping up-to-date with later
advisories. In lieu of automatic imports from GitHub to RustSec we
figured we'd in the interim manually fill in some fields.

In this PR I'm back-filling security advisories we've had in Wasmtime
into the RustSec database here. The oldest advisory here is 3 years old
and the goal is to have this serve as a template for importing future
advisories that Wasmtime gets. It's not expected for this to cause any
churn or undue warnings but instead is intended to bring RustSec
up-to-date with the advisories we have for this crate.

[Wasmtime]: https://crates.io/crates/wasmtime
[request]: bytecodealliance/wasmtime#10344
@alexcrichton alexcrichton force-pushed the old-wasmtime-advisory branch from 6ddd7fb to 4cdfb82 Compare May 1, 2025 15:13
@alexcrichton alexcrichton changed the title Import an old Wasmtime security advisory Import old Wasmtime security advisories May 1, 2025
@alexcrichton
Copy link
Contributor Author

Sure thing, rebased and updated the commit message/PR title too

@djc djc merged commit 669a958 into rustsec:main May 2, 2025
1 check passed
@alexcrichton alexcrichton deleted the old-wasmtime-advisory branch May 2, 2025 14:42
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

Successfully merging this pull request may close these issues.

5 participants