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

Onion messages: add fuzz testing #1648

Merged

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Aug 3, 2022

Adds fuzz testing for onion messages.

@TheBlueMatt
Copy link
Collaborator

Test LGTM, giving this some CPU cycles now.

@TheBlueMatt
Copy link
Collaborator

Hmmm, I gave this a few weeks of CPU time and it wasn't able to find its way into a valid onion message decryption at all. This tells me we need to seed the fuzzer, like the way we do full_stack_target, at least, if not find a way to simplify the target here. I'm not sure quite how to simplify the target greatly, though, so maybe just seed it and move on.

@TheBlueMatt
Copy link
Collaborator

Specifically, the fuzzer did not fail with this patch:

diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs
index 7eba3cdd..617ed434 100644
--- a/lightning/src/onion_message/messenger.rs
+++ b/lightning/src/onion_message/messenger.rs
@@ -211,11 +211,13 @@ impl<Signer: Sign, K: Deref, L: Deref> OnionMessenger<Signer, K, L>
                        Ok((Payload::Receive {
                                control_tlvs: ReceiveControlTlvs::Unblinded(ReceiveTlvs { path_id })
                        }, None)) => {
+panic!("1");
                                log_info!(self.logger, "Received an onion message with path_id: {:02x?}", path_id);
                        },
                        Ok((Payload::Forward(ForwardControlTlvs::Unblinded(ForwardTlvs {
                                next_node_id, next_blinding_override
                        })), Some((next_hop_hmac, new_packet_bytes)))) => {
+panic!("2");
                                // TODO: we need to check whether `next_node_id` is our node, in which case this is a dummy
                                // blinded hop and this onion message is destined for us. In this situation, we should keep
                                // unwrapping the onion layers to get to the final payload. Since we don't have the option

@valentinewallace valentinewallace force-pushed the 2022-08-fuzz-onion-msgs branch from a3423c9 to 5c80aaa Compare August 8, 2022 22:06
@valentinewallace
Copy link
Contributor Author

I added some seeding but could add more onion message seeds if it helps.

Unsure if it would be good to add msg_targets for onion_message::Payload and/or BlindedRoute?

@valentinewallace
Copy link
Contributor Author

Able to hit the obvious panics now. I put the seeds in test_no_onion_message_breakage -- not sure if they should be checked into git as well?

fuzz/src/onion_message.rs Outdated Show resolved Hide resolved
lightning/src/util/chacha20poly1305rfc.rs Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2022-08-fuzz-onion-msgs branch from e693109 to 611d901 Compare August 15, 2022 16:04
TheBlueMatt
TheBlueMatt previously approved these changes Aug 15, 2022
fuzz/src/onion_message.rs Outdated Show resolved Hide resolved
fuzz/src/onion_message.rs Show resolved Hide resolved
Also update the fuzz ChaCha20Poly1305 to not mark as finished after a single
encrypt_in_place.  This is because more bytes may still need to be encrypted,
causing us to panic at the assertion that finished == false when we go to
encrypt more.

Also fix unused_mut warning in messenger + add log on OM forward for testing
@valentinewallace valentinewallace merged commit 558c460 into lightningdevkit:main Aug 17, 2022
@jkczyz jkczyz mentioned this pull request May 10, 2023
60 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants