Infinite onion message parse

[morehouse]

While fuzzing onion_message_target, @dergoegge found the following (base64-encoded) input which triggers an infinite loop in the onion parsing logic:

AuT//////////52dIwCdnZ2dAgr///////j///8ACAAAAGL/AgBWAf0AgP3/AAD/Av//AAAAYv8CAgCGAQAA/39zAJ0C//8AAAAAAAEABPv//wP///8AAf8A////AAAAAAADAQABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=

---

From my investigation, the root cause is something like this:

1. The hop_data for the onion packet is wrapped in a Cursor and sent off for TLV parsing here:

   rust-lightning/lightning/src/ln/onion_utils.rs

   Lines 2640 to 2641 in 81495c6

   	let mut chacha_stream = ChaChaReader { chacha: &mut chacha, read: Cursor::new(&hop_data[..]) };
   	match R::read(&mut chacha_stream, read_args) {

2. The type and length for the first TLV are parsed. In this case, the type is encrypted_recipient_data and the specified length is larger than the remaining data in hop_data. Nevertheless, the remaining hop_data is wrapped in a FixedLengthReader of the larger length and passed off for decryption here:

   rust-lightning/lightning/src/util/ser_macros.rs

   Lines 635 to 639 in 81495c6

   	let length: ser::BigSize = $crate::util::ser::Readable::read(stream_ref)?;
   	let mut s = ser::FixedLengthReader::new(stream_ref, length.0);
   	match typ.0 {
   	$(_t if $crate::_decode_tlv_stream_match_check!(_t, $type, $fieldty) => {
   	$crate::_decode_tlv!($stream, s, $field, $fieldty);

3. The FixedLengthReader is read and decrypted, and any extra bytes are handled in this loop:

   rust-lightning/lightning/src/crypto/streams.rs

   Lines 131 to 134 in 81495c6

   	while chacha_stream.read.bytes_remain() {
   	let mut buf = [0; 256];
   	chacha_stream.read(&mut buf)?;
   	}

   In that loop, once the underlying hop_data is exhausted, every read from the FixedLengthReader becomes a no-op since Cursor::read returns 0. At the same time, FixedLengthReader::bytes_remaining always returns true because there's still bytes remaining from its point of view. The loop continues forever.

---

It looks like this bug was introduced here: 8ee02eb

Prior to that commit, chacha_stream.read.eat_remaining() would return an error once hop_data was exhausted.

[TheBlueMatt]

Ouch, thanks for running the fuzzer! Now I need to figure out why my fuzzer runs didn't hit this...

[dergoegge]

Now I need to figure out why my fuzzer runs didn't hit this...

I'm just guessing but it could any of the following:

• At the moment, I only use libFuzzer for fuzzing ldk. If you're using a different fuzzer, it may explain why you haven't found the issue. That is not to say that libFuzzer is the best, just that different engines work differently and occasionally find things that other engines don't (rare but it happens).
• A few cores use libFuzzer's -use_value_profile=1 option (enabling additional guidance for the fuzzer for solving comparisons), which can lead to finding bugs that you won't find without the option. Depending on the harness, it can add quite a bit of overhead though, which is why I don't use it on all cores.
• This bug will show up as a timeout and not a crash, perhaps your setup doesn't take those into account? (they are saved as files named timeout-<hash> as opposed to crash-<hash>)
• The issue might be really hard to find and my setup just got lucky 🤷

[tnull]

Thanks @dergoegge and @morehouse! ❤️