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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Snappy additional sanity checks #1625

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Snappy additional sanity checks #1625

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a401619
Comments
pawanjay176 Sep 15, 2020
cf4b040
Add a custom `read_exact implementation`
pawanjay176 Sep 16, 2020
2faa572
Add underflow checks
pawanjay176 Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 51 additions & 9 deletions beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ impl<TSpec: EthSpec> Decoder for SSZSnappyInboundCodec<TSpec> {
fn decode(&mut self, src: &mut BytesMut) -> Result<Option<Self::Item>, 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);
Expand All @@ -113,10 +114,11 @@ impl<TSpec: EthSpec> Decoder for SSZSnappyInboundCodec<TSpec> {
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();
Expand Down Expand Up @@ -194,8 +196,7 @@ impl<TSpec: EthSpec> Decoder for SSZSnappyInboundCodec<TSpec> {
}
}
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),
},
Expand Down Expand Up @@ -277,6 +278,7 @@ impl<TSpec: EthSpec> Decoder for SSZSnappyOutboundCodec<TSpec> {
fn decode(&mut self, src: &mut BytesMut) -> Result<Option<Self::Item>, 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);
Expand All @@ -291,9 +293,10 @@ impl<TSpec: EthSpec> Decoder for SSZSnappyOutboundCodec<TSpec> {
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();
Expand Down Expand Up @@ -364,8 +367,7 @@ impl<TSpec: EthSpec> Decoder for SSZSnappyOutboundCodec<TSpec> {
}
}
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),
},
Expand Down Expand Up @@ -396,9 +398,10 @@ impl<TSpec: EthSpec> OutboundCodec<RPCRequest<TSpec>> 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();
Expand All @@ -409,11 +412,50 @@ impl<TSpec: EthSpec> OutboundCodec<RPCRequest<TSpec>> 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),
},
}
}
}

/// 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<T: std::convert::AsRef<[u8]>>(
reader: &mut FrameDecoder<Cursor<T>>,
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;
Copy link
Member

Choose a reason for hiding this comment

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

can the position here ever be less than the previous position and underflow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Checked the snappy read implementation and it always advances the reader forward as far as I can tell. However, added an underflow check to be safe.

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(())
}
}