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

[feature]: make fuzz tests first-class citizens #7452

Closed
morehouse opened this issue Feb 24, 2023 · 5 comments
Closed

[feature]: make fuzz tests first-class citizens #7452

morehouse opened this issue Feb 24, 2023 · 5 comments
Assignees
Labels
enhancement Improvements to existing features / behaviour fuzzing security General label for issues/PRs related to the security of the software

Comments

@morehouse
Copy link
Collaborator

Problem

LND's fuzz tests failed to build last year for at least 4 months, maybe longer. This wasn't noticed until @Crypt-iQ ran the tests locally.

This bit rot was possible because LND's fuzz tests do not run in CI.

Solution

It is good practice to run each fuzz target on its seed corpus in CI (just a test run, no fuzzing). This turns the seed corpus into a set of regression tests that should be kept up-to-date with the best coverage found by each fuzz target. Especially when bugs are found, the triggering input should be added to the seed corpus.

The seed corpora could be stored in the LND repo or a separate repo similar to bitcoin-core.

Bonus

It would also be very good to set up continuous fuzzing somewhere offline if this isn't already being done. We don't want an attacker to be able to discover and exploit a bug simply because they ran our existing fuzz tests more than us.

#5965 is one continuous fuzzing option, or we could do something homegrown.

@morehouse morehouse added the enhancement Improvements to existing features / behaviour label Feb 24, 2023
@Roasbeef Roasbeef added wire protocol Encoding of messages for the communication between nodes security General label for issues/PRs related to the security of the software fuzzing labels Feb 24, 2023
@Crypt-iQ Crypt-iQ removed the wire protocol Encoding of messages for the communication between nodes label Feb 27, 2023
@Crypt-iQ
Copy link
Collaborator

The seed corpora could be stored in the LND repo or a separate repo similar to bitcoin-core.

These should be stored in a separate repo so that git clone github.com/lightningnetwork/lnd doesn't download GBs of seeds. Do you want to work on the open-source CI part?

@morehouse
Copy link
Collaborator Author

I'm happy to work on the CI. Just let me know once you have a GitHub repo set up for the corpora.

@morehouse
Copy link
Collaborator Author

After some tinkering, I think we should use libFuzzer as our fuzzing engine instead of Go's native engine.

As of Go 1.20, the native engine has practically no configuration options, and no ability to minimize a corpus or merge new inputs into an existing corpus. The latter makes maintaining the https://github.com/lightninglabs/lnd-fuzz repo more difficult: we'd need to build some new tool to do our own minimization.

Using libFuzzer instead gets us easy minimization and corpus merging, plus all the config options, at the cost of slightly more complexity to build and run the fuzz tests. The complexity is similar (or slightly less) to our previous method using go-fuzz.

Note that other projects serious about fuzzing also use libFuzzer mode for similar reasons (e.g., OSS-Fuzz).

@morehouse
Copy link
Collaborator Author

After some tinkering, I think we should use libFuzzer as our fuzzing engine instead of Go's native engine.

I played around with https://github.com/AdamKorcz/go-118-fuzz-build as a way of using libFuzzer with Go native fuzz targets, and I'm pretty disappointed by it. I ran into bugs (e.g., input bytes dropped) and limitations (e.g., only 1 byte integers), giving a rather unpolished feeling to the whole project.

It seems native Go fuzzing will be less of a headache for us. I've implemented a corpus merging/minimization script that we can use until Go supports merging natively. It seems to be working pretty well so far.

@morehouse
Copy link
Collaborator Author

We now have

This satisfies the main purpose of this issue. A continuous fuzzing solution would still be good, but we can use #5965 or a new issue to track that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features / behaviour fuzzing security General label for issues/PRs related to the security of the software
Projects
None yet
Development

No branches or pull requests

3 participants