Correct EOF handling in stream read in `ChaChaDualPolyReadAdapter`
What the developer wrote
Authored by Matt Corallo
Correct EOF handling in stream read in `ChaChaDualPolyReadAdapter`
When `ChaChaDualPolyReadAdapter` encounters an EOF (`Read::read`
returns `Ok(0)`) while trying to drain the stream (even though the
`FixedLengthReader` thinks it has available space) we'll end up
infinite-looping trying to drain the stream looking for
`Read::read` to return an `Err` (which it won't).
The fix is, of course, simple, to detect the EOF signal.
Found by the `onion_message_target` fuzzer which @dergoegge ran.
Thanks to @morehouse for digging deeper on the specific fuzz test
case and thoroughly reporting the underlying causes.
Fixes #4139.
What changed, and why it matters
This commit fixes a bug in rust-lightning's encrypted stream reader where reaching the end of a data stream could cause the program to get stuck in an endless loop instead of stopping cleanly. The fix makes the reader recognize the end-of-stream signal and return a 'short read' error. The bug was found by a fuzz tester, not reported as an active attack.
Upgrade to a rust-lightning release containing this commit. If running a node that processes untrusted onion messages or encrypted streams, prioritize the patch to avoid CPU exhaustion from maliciously truncated inputs.
Security signals we found
Infinite loop / denial-of-service condition on malformed or truncated encrypted input
Missing EOF handling in a stream-draining loop
Discovered by fuzzing (`onion_message_target`)
Fix returns explicit `DecodeError::ShortRead` rather than hanging
Evidence from the diff
In ChaChaDualPolyReadAdapter::read, after decrypting the main object, a loop drains any remaining bytes from the underlying FixedLengthReader. The loop called chacha_stream.read(&mut buf)?, which propagates I/O errors but ignored a normal Ok(0) EOF return. Because FixedLengthReader believes bytes remain, the loop would spin forever once the real underlying reader reached EOF. The patch checks for Ok(0) and returns DecodeError::ShortRead. A regression test verifies the behavior.
Changed components
lightning/src/crypto/streams.rsChaChaDualPolyReadAdapterFixedLengthReader-backed ChaCha stream readsInspect captured patch +20 / −1
diff --git a/lightning/src/crypto/streams.rs b/lightning/src/crypto/streams.rs
index b631b7b..c406e93 100644
--- a/lightning/src/crypto/streams.rs
+++ b/lightning/src/crypto/streams.rs
@@ -130,7 +130,10 @@ impl<T: Readable> LengthReadableArgs<([u8; 32], [u8; 32])> for ChaChaDualPolyRea
let readable: T = Readable::read(&mut chacha_stream)?;
while chacha_stream.read.bytes_remain() {
let mut buf = [0; 256];
- chacha_stream.read(&mut buf)?;
+ if chacha_stream.read(&mut buf)? == 0 {
+ // Reached EOF
+ return Err(DecodeError::ShortRead);
+ }
}
let read_len = chacha_stream.read_len;
@@ -344,4 +347,20 @@ mod tests {
// This also serves to test the `option: $trait` variant of the `_decode_tlv` ser macro.
do_chacha_stream_adapters_ser_macros().unwrap()
}
+
+ #[test]
+ fn short_read_chacha_dual_read_adapter() {
+ // Previously, if we attempted to read from a ChaChaDualPolyReadAdapter but the object
+ // being read is shorter than the available buffer while the buffer passed to
+ // ChaChaDualPolyReadAdapter itself always thinks it has room, we'd end up
+ // infinite-looping as we didn't handle `Read::read`'s 0 return values at EOF.
+ let mut stream = &[0; 1024][..];
+ let mut too_long_stream = FixedLengthReader::new(&mut stream, 2048);
+ let keys = ([42; 32], [99; 32]);
+ let res = super::ChaChaDualPolyReadAdapter::<u8>::read(&mut too_long_stream, keys);
+ match res {
+ Ok(_) => panic!(),
+ Err(e) => assert_eq!(e, DecodeError::ShortRead),
+ }
+ }
}
Why this scored 60/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.