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

lnwire: signature parsing/conversion fuzz tests #7649

Merged
merged 3 commits into from
May 12, 2023

Conversation

morehouse
Copy link
Collaborator

We call lnwire's signature parsing and conversion functions from many places in LND, often with untrusted inputs, and existing fuzz tests had incomplete coverage of these functions.

No crashes found after 100+ CPU-hours of fuzzing for each fuzz target.

@Roasbeef Roasbeef requested review from Crypt-iQ and ellemouton April 28, 2023 18:04
@lightninglabs-deploy
Copy link

@Crypt-iQ: review reminder
@ellemouton: review reminder

Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

LGTM! 🔥

@@ -822,3 +822,24 @@ func FuzzCustomMessage(f *testing.F) {
harness(t, data)
})
}

// FuzzParseRawSignature tests that our DER-encoded signature parsing does not
// panic for arbitrary inputs and that serializing and reparsing the signatures
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: s/reparsing/re-parsing

Comment on lines +837 to +843
if err != nil {
t.Fatalf("failed to reparse signature: %v", err)
}

if !reflect.DeepEqual(sig, sig2) {
t.Fatalf("signature mismatch: %v != %v", sig, sig2)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason not to use the require package for these? I see that all the other fuzz tests also dont use it... not sure if there was a specific reason for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have a good reason. I'll send a follow-up PR making the change for all fuzz tests.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 👑

@morehouse morehouse force-pushed the fuzz_lnwire_signature branch from 3d2677a to 7b17a4a Compare May 11, 2023 21:59
@morehouse
Copy link
Collaborator Author

Rebased and added release note.

@Roasbeef
Copy link
Member

Looks like something else snuck in before and added another conflict in the release notes.

@morehouse morehouse force-pushed the fuzz_lnwire_signature branch from 7b17a4a to 650f4f3 Compare May 11, 2023 22:22
@morehouse
Copy link
Collaborator Author

Looks like something else snuck in before and added another conflict in the release notes.

Fixed.

@guggero
Copy link
Collaborator

guggero commented May 12, 2023

Again, sorry... I promise, this PR is next in the merge queue!

morehouse added 3 commits May 12, 2023 11:07
Test parsing and serialization of raw DER-encoded signatures.
Test conversion of fixed 64-byte signatures to DER-encoded signatures.
@morehouse morehouse force-pushed the fuzz_lnwire_signature branch from 650f4f3 to 55aa7a7 Compare May 12, 2023 16:09
@morehouse
Copy link
Collaborator Author

Rebase. Third time's the charm.

@guggero guggero merged commit d8eb6da into lightningnetwork:master May 12, 2023
@morehouse morehouse deleted the fuzz_lnwire_signature branch May 12, 2023 17:01
morehouse added a commit to morehouse/lnd-fuzz that referenced this pull request May 12, 2023
Adds seeds for the FuzzParseRawSignature target recently added to LND
[1]. Generated from 100+ CPU-hours of fuzzing and minimized using the
script from lightninglabs#4.

[1] lightningnetwork/lnd#7649
morehouse added a commit to morehouse/lnd-fuzz that referenced this pull request May 12, 2023
Adds seeds for the FuzzConvertFixedSignature target recently added to
LND [1]. Generated from 100+ CPU-hours of fuzzing and minimized using
the script from lightninglabs#4.

[1] lightningnetwork/lnd#7649
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