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

cargo-vet: Exclude fuzzing-only dependencies #8488

Merged
merged 1 commit into from May 2, 2024

Conversation

jameysharp
Copy link
Contributor

We can't meaningfully audit the other WebAssembly implementations that we use for differential fuzzing, such as wasmi and especially v8. Let's acknowledge that the effort to do so is not practical for us, and focus our vetting efforts on crates that developers and users are more likely to build.

This reduces our estimated audit backlog by over three million lines, according to cargo vet suggest.

Note that our crates which depend on those engines, such as wasmtime-fuzzing, are not published to crates.io, so if we fall victim to a supply chain attack against dependencies of these crates, the folks who might be impacted are limited.

Although there is value in also auditing code that might be run by people who clone our git repository, in this case I propose that anyone who is concerned about the risks of supply chain attacks against their development systems should be running fuzzers inside a sandbox. After all, it's a fuzzer: it's specifically designed to try to do anything.

I'd like to especially seek comment from folks who've expressed interest in our use of cargo-vet, like @alexcrichton, @bholley, @cfallin, and @tschneidereit. I'm open to being persuaded that we shouldn't make this change, but I can't currently see that we get any value from auditing these particular dependencies.

@jameysharp jameysharp requested a review from a team as a code owner April 26, 2024 07:54
@jameysharp jameysharp requested review from alexcrichton and removed request for a team April 26, 2024 07:54
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a reasonable change to make yeah, we already don't vet these dependencies much and reducing our backlog seems reasonable to me

@fitzgen
Copy link
Member

fitzgen commented Apr 26, 2024

Makes sense to me too.

@tschneidereit
Copy link
Member

Same for me, yeah. It'd be good to have this mentioned somewhere in documentation people running fuzzing would be likely to see

@jameysharp
Copy link
Contributor Author

It'd be good to have this mentioned somewhere in documentation people running fuzzing would be likely to see

Good point. I'll add some text to fuzz/README.md and docs/contributing-fuzzing.md to this PR before merging it. I'm not sure we have anywhere else that anyone is likely to notice.

Also, do we have text somewhere that I should update describing our cargo-vet policy?

We can't meaningfully audit the other WebAssembly implementations that
we use for differential fuzzing, such as wasmi and especially v8. Let's
acknowledge that the effort to do so is not practical for us, and focus
our vetting efforts on crates that developers and users are more likely
to build.

This reduces our estimated audit backlog by over three million lines,
according to `cargo vet suggest`.

Note that our crates which depend on those engines, such as
wasmtime-fuzzing, are not published to crates.io, so if we fall victim
to a supply chain attack against dependencies of these crates, the folks
who might be impacted are limited.

Although there is value in also auditing code that might be run by
people who clone our git repository, in this case I propose that anyone
who is concerned about the risks of supply chain attacks against their
development systems should be running fuzzers inside a sandbox. After
all, it's a fuzzer: it's specifically designed to try to do anything.
@jameysharp jameysharp requested a review from a team as a code owner April 30, 2024 01:19
@jameysharp jameysharp requested review from elliottt and removed request for a team April 30, 2024 01:19
@jameysharp
Copy link
Contributor Author

I've pushed some new text for fuzz/README.md. @tschneidereit, does this text work for you?

I considered adding text to docs/contributing-fuzzing.md but it's really about developing fuzz targets. For people who just want to run them it points to fuzz/README.md, so I think the notice above suffices.

I also considered adding text to docs/contributing-coding-guidelines.md, where we talk about cargo vet, but couldn't decide if there's anything that would be useful to say there.

Finally, I considered trying to make suggestions about sandboxes people could try, and got as far as verifying that cargo fuzz run works under unshare --map-current-user --net --ipc --mount on Linux (but not with --pid for some reason). But then I remembered that I don't want to be in the business of recommending security tools.

@github-actions github-actions bot added the fuzzing Issues related to our fuzzing infrastructure label Apr 30, 2024
Copy link

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@elliottt elliottt removed their request for review May 1, 2024 00:03
@alexcrichton
Copy link
Member

I think this is pretty reasonable so I'm gonna go ahead and flag for merge given the consensus here. @tschneidereit if you've got further thoughts though on the docs though we can always have follow-ups too

@alexcrichton alexcrichton added this pull request to the merge queue May 2, 2024
Merged via the queue into bytecodealliance:main with commit d1014fa May 2, 2024
21 checks passed
@jameysharp jameysharp deleted the no-vet-fuzzers branch May 3, 2024 00:24
@tschneidereit
Copy link
Member

Thank you for doing this, @jameysharp! I agree that these doc changes are good, and also with your reasoning for not making other doc changes :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants