From a401619494fae8563fd1efb7567f05b383ebfc3d Mon Sep 17 00:00:00 2001 From: pawan Date: Tue, 15 Sep 2020 12:24:02 +0530 Subject: [PATCH 1/3] Comments --- beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs b/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs index 94fd5d4c598..4567c3b84f8 100644 --- a/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs +++ b/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs @@ -99,6 +99,7 @@ impl Decoder for SSZSnappyInboundCodec { fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { if self.len.is_none() { // Decode the length of the uncompressed bytes from an unsigned varint + // Note: length-prefix of > 10 bytes(uint64) would be a decoding error match self.inner.decode(src).map_err(RPCError::from)? { Some(length) => { self.len = Some(length); @@ -194,8 +195,7 @@ impl Decoder for SSZSnappyInboundCodec { } } Err(e) => match e.kind() { - // Haven't received enough bytes to decode yet - // TODO: check if this is the only Error variant where we return `Ok(None)` + // Haven't received enough bytes to decode yet, wait for more ErrorKind::UnexpectedEof => Ok(None), _ => Err(e).map_err(RPCError::from), }, @@ -277,6 +277,7 @@ impl Decoder for SSZSnappyOutboundCodec { fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { if self.len.is_none() { // Decode the length of the uncompressed bytes from an unsigned varint + // Note: length-prefix of > 10 bytes(uint64) would be a decoding error match self.inner.decode(src).map_err(RPCError::from)? { Some(length) => { self.len = Some(length as usize); @@ -364,8 +365,7 @@ impl Decoder for SSZSnappyOutboundCodec { } } Err(e) => match e.kind() { - // Haven't received enough bytes to decode yet - // TODO: check if this is the only Error variant where we return `Ok(None)` + // Haven't received enough bytes to decode yet, wait for more ErrorKind::UnexpectedEof => Ok(None), _ => Err(e).map_err(RPCError::from), }, @@ -409,8 +409,7 @@ impl OutboundCodec> for SSZSnappyOutboundCodec )?))) } Err(e) => match e.kind() { - // Haven't received enough bytes to decode yet - // TODO: check if this is the only Error variant where we return `Ok(None)` + // Haven't received enough bytes to decode yet, wait for more ErrorKind::UnexpectedEof => Ok(None), _ => Err(e).map_err(RPCError::from), }, From cf4b040676e2636c6a9722662188711719fcf9ce Mon Sep 17 00:00:00 2001 From: pawan Date: Wed, 16 Sep 2020 15:10:57 +0530 Subject: [PATCH 2/3] Add a custom `read_exact implementation` --- .../eth2_libp2p/src/rpc/codec/ssz_snappy.rs | 49 +++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs b/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs index 4567c3b84f8..6697bf4acf4 100644 --- a/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs +++ b/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs @@ -114,10 +114,11 @@ impl Decoder for SSZSnappyInboundCodec { if length > self.max_packet_size { return Err(RPCError::InvalidData); } + let max_encoded_length = snap::raw::max_compress_len(length) as u64; let mut reader = FrameDecoder::new(Cursor::new(&src)); let mut decoded_buffer = vec![0; length]; - match reader.read_exact(&mut decoded_buffer) { + match read_exact(&mut reader, &mut decoded_buffer, max_encoded_length) { Ok(()) => { // `n` is how many bytes the reader read in the compressed stream let n = reader.get_ref().position(); @@ -292,9 +293,10 @@ impl Decoder for SSZSnappyOutboundCodec { if length > self.max_packet_size { return Err(RPCError::InvalidData); } + let max_encoded_length = snap::raw::max_compress_len(length) as u64; let mut reader = FrameDecoder::new(Cursor::new(&src)); let mut decoded_buffer = vec![0; length]; - match reader.read_exact(&mut decoded_buffer) { + match read_exact(&mut reader, &mut decoded_buffer, max_encoded_length) { Ok(()) => { // `n` is how many bytes the reader read in the compressed stream let n = reader.get_ref().position(); @@ -396,9 +398,10 @@ impl OutboundCodec> for SSZSnappyOutboundCodec if length > self.max_packet_size { return Err(RPCError::InvalidData); } + let max_encoded_length = snap::raw::max_compress_len(length) as u64; let mut reader = FrameDecoder::new(Cursor::new(&src)); let mut decoded_buffer = vec![0; length]; - match reader.read_exact(&mut decoded_buffer) { + match read_exact(&mut reader, &mut decoded_buffer, max_encoded_length) { Ok(()) => { // `n` is how many bytes the reader read in the compressed stream let n = reader.get_ref().position(); @@ -416,3 +419,43 @@ impl OutboundCodec> for SSZSnappyOutboundCodec } } } + +/// Wrapper over `read` implementation of `FrameDecoder`. +/// +/// Works like the standard `read_exact` implementation, except that it returns an error if length of bytes +/// read from the underlying reader is greater than `max_compressed_length`. +fn read_exact>( + reader: &mut FrameDecoder>, + mut buf: &mut [u8], + max_compressed_len: u64, +) -> Result<(), std::io::Error> { + let mut i = reader.get_ref().position(); + let mut count = 0; + while !buf.is_empty() { + match reader.read(buf) { + Ok(0) => break, + Ok(n) => { + let tmp = buf; + buf = &mut tmp[n..]; + } + Err(ref e) if e.kind() == ErrorKind::Interrupted => {} + Err(e) => return Err(e), + } + count += reader.get_ref().position() - i; + i = reader.get_ref().position(); + if count > max_compressed_len { + return Err(std::io::Error::new( + ErrorKind::InvalidData, + "compressed data is > max_compressed_len", + )); + } + } + if !buf.is_empty() { + Err(std::io::Error::new( + ErrorKind::UnexpectedEof, + "failed to fill whole buffer", + )) + } else { + Ok(()) + } +} From 2faa572e758c24d8e2f431f116782819832073f1 Mon Sep 17 00:00:00 2001 From: pawan Date: Fri, 18 Sep 2020 17:51:47 +0530 Subject: [PATCH 3/3] Add underflow checks --- .../eth2_libp2p/src/rpc/codec/ssz_snappy.rs | 40 +++++++++++++------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs b/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs index 6697bf4acf4..8b35a4c6855 100644 --- a/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs +++ b/beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs @@ -114,11 +114,10 @@ impl Decoder for SSZSnappyInboundCodec { if length > self.max_packet_size { return Err(RPCError::InvalidData); } - let max_encoded_length = snap::raw::max_compress_len(length) as u64; let mut reader = FrameDecoder::new(Cursor::new(&src)); let mut decoded_buffer = vec![0; length]; - match read_exact(&mut reader, &mut decoded_buffer, max_encoded_length) { + match read_exact(&mut reader, &mut decoded_buffer, length) { Ok(()) => { // `n` is how many bytes the reader read in the compressed stream let n = reader.get_ref().position(); @@ -293,10 +292,9 @@ impl Decoder for SSZSnappyOutboundCodec { if length > self.max_packet_size { return Err(RPCError::InvalidData); } - let max_encoded_length = snap::raw::max_compress_len(length) as u64; let mut reader = FrameDecoder::new(Cursor::new(&src)); let mut decoded_buffer = vec![0; length]; - match read_exact(&mut reader, &mut decoded_buffer, max_encoded_length) { + match read_exact(&mut reader, &mut decoded_buffer, length) { Ok(()) => { // `n` is how many bytes the reader read in the compressed stream let n = reader.get_ref().position(); @@ -398,10 +396,9 @@ impl OutboundCodec> for SSZSnappyOutboundCodec if length > self.max_packet_size { return Err(RPCError::InvalidData); } - let max_encoded_length = snap::raw::max_compress_len(length) as u64; let mut reader = FrameDecoder::new(Cursor::new(&src)); let mut decoded_buffer = vec![0; length]; - match read_exact(&mut reader, &mut decoded_buffer, max_encoded_length) { + match read_exact(&mut reader, &mut decoded_buffer, length) { Ok(()) => { // `n` is how many bytes the reader read in the compressed stream let n = reader.get_ref().position(); @@ -422,14 +419,18 @@ impl OutboundCodec> for SSZSnappyOutboundCodec /// Wrapper over `read` implementation of `FrameDecoder`. /// -/// Works like the standard `read_exact` implementation, except that it returns an error if length of bytes -/// read from the underlying reader is greater than `max_compressed_length`. +/// Works like the standard `read_exact` implementation, except that it returns an error if length of +// compressed bytes read from the underlying reader is greater than worst case compression length for snappy. fn read_exact>( reader: &mut FrameDecoder>, mut buf: &mut [u8], - max_compressed_len: u64, + uncompressed_length: usize, ) -> Result<(), std::io::Error> { - let mut i = reader.get_ref().position(); + // Calculate worst case compression length for given uncompressed length + let max_compressed_len = snap::raw::max_compress_len(uncompressed_length) as u64; + + // Initialize the position of the reader + let mut pos = reader.get_ref().position(); let mut count = 0; while !buf.is_empty() { match reader.read(buf) { @@ -441,12 +442,25 @@ fn read_exact>( Err(ref e) if e.kind() == ErrorKind::Interrupted => {} Err(e) => return Err(e), } - count += reader.get_ref().position() - i; - i = reader.get_ref().position(); + // Get current position of reader + let curr_pos = reader.get_ref().position(); + // Note: reader should always advance forward. However, this behaviour + // depends on the implementation of `snap::FrameDecoder`, so it is better + // to check to avoid underflow panic. + if curr_pos > pos { + count += reader.get_ref().position() - pos; + pos = curr_pos; + } else { + return Err(std::io::Error::new( + ErrorKind::InvalidData, + "snappy: reader is not advanced forward while reading", + )); + } + if count > max_compressed_len { return Err(std::io::Error::new( ErrorKind::InvalidData, - "compressed data is > max_compressed_len", + "snappy: compressed data is > max_compressed_len", )); } }