Add explicit bounds for ssz decoding in rpc (#1250)

* Update `milagro_bls` to new release (#1183)

* Update milagro_bls to new release

Signed-off-by: Kirk Baird <baird.k@outlook.com>

* Tidy up fake cryptos

Signed-off-by: Kirk Baird <baird.k@outlook.com>

* move SecretHash to bls and put plaintext back

Signed-off-by: Kirk Baird <baird.k@outlook.com>

* Update v0.12.0 to v0.12.1

* Use ssz types for Request and error types

* Fix errors

* Constrain BlocksByRangeRequest count to MAX_REQUEST_BLOCKS

* Fix issues after rebasing

* Compute bounds for variable ssz containers

* Check ssz bounds before decoding

* Add clarifying comment; fix BlocksByRootRequest min/max

Co-authored-by: Kirk Baird <baird.k@outlook.com>
Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: Age Manning <Age@AgeManning.com>

beacon_node/eth2-libp2p/src/rpc/codec/ssz.rs

@@ -1,7 +1,10 @@
 use crate::rpc::methods::*;
 use crate::rpc::{
     codec::base::OutboundCodec,
-    protocol::{Encoding, Protocol, ProtocolId, RPCError, Version},
+    protocol::{
+        Encoding, Protocol, ProtocolId, RPCError, Version, BLOCKS_BY_ROOT_REQUEST_MAX,
+        BLOCKS_BY_ROOT_REQUEST_MIN, SIGNED_BEACON_BLOCK_MAX, SIGNED_BEACON_BLOCK_MIN,
+    },
 };
 use crate::rpc::{RPCCodedResponse, RPCRequest, RPCResponse};
 use libp2p::bytes::{BufMut, Bytes, BytesMut};
@@ -84,29 +87,61 @@ impl<TSpec: EthSpec> Decoder for SSZInboundCodec<TSpec> {
         match self.inner.decode(src).map_err(RPCError::from) {
             Ok(Some(packet)) => match self.protocol.message_name {
                 Protocol::Status => match self.protocol.version {
-                    Version::V1 => Ok(Some(RPCRequest::Status(StatusMessage::from_ssz_bytes(
+                    Version::V1 => {
+                        if packet.len() == <StatusMessage as Encode>::ssz_fixed_len() {
+                            Ok(Some(RPCRequest::Status(StatusMessage::from_ssz_bytes(
                                 &packet,
-                    )?))),
+                            )?)))
+                        } else {
+                            Err(RPCError::InvalidData)
+                        }
+                    }
                 },
                 Protocol::Goodbye => match self.protocol.version {
-                    Version::V1 => Ok(Some(RPCRequest::Goodbye(GoodbyeReason::from_ssz_bytes(
+                    Version::V1 => {
+                        if packet.len() == <GoodbyeReason as Encode>::ssz_fixed_len() {
+                            Ok(Some(RPCRequest::Goodbye(GoodbyeReason::from_ssz_bytes(
                                 &packet,
-                    )?))),
+                            )?)))
+                        } else {
+                            Err(RPCError::InvalidData)
+                        }
+                    }
                 },
                 Protocol::BlocksByRange => match self.protocol.version {
-                    Version::V1 => Ok(Some(RPCRequest::BlocksByRange(
+                    Version::V1 => {
+                        if packet.len() == <BlocksByRangeRequest as Encode>::ssz_fixed_len() {
+                            Ok(Some(RPCRequest::BlocksByRange(
                                 BlocksByRangeRequest::from_ssz_bytes(&packet)?,
-                    ))),
+                            )))
+                        } else {
+                            Err(RPCError::InvalidData)
+                        }
+                    }
                 },
                 Protocol::BlocksByRoot => match self.protocol.version {
-                    Version::V1 => Ok(Some(RPCRequest::BlocksByRoot(BlocksByRootRequest {
+                    Version::V1 => {
+                        if packet.len() >= *BLOCKS_BY_ROOT_REQUEST_MIN
+                            && packet.len() <= *BLOCKS_BY_ROOT_REQUEST_MAX
+                        {
+                            Ok(Some(RPCRequest::BlocksByRoot(BlocksByRootRequest {
                                 block_roots: VariableList::from_ssz_bytes(&packet)?,
-                    }))),
+                            })))
+                        } else {
+                            Err(RPCError::InvalidData)
+                        }
+                    }
                 },
                 Protocol::Ping => match self.protocol.version {
-                    Version::V1 => Ok(Some(RPCRequest::Ping(Ping {
+                    Version::V1 => {
+                        if packet.len() == <Ping as Encode>::ssz_fixed_len() {
+                            Ok(Some(RPCRequest::Ping(Ping {
                                 data: u64::from_ssz_bytes(&packet)?,
-                    }))),
+                            })))
+                        } else {
+                            Err(RPCError::InvalidData)
+                        }
+                    }
                 },
                 Protocol::MetaData => match self.protocol.version {
                     Version::V1 => {
@@ -208,30 +243,64 @@ impl<TSpec: EthSpec> Decoder for SSZOutboundCodec<TSpec> {
 
                     match self.protocol.message_name {
                         Protocol::Status => match self.protocol.version {
-                            Version::V1 => Ok(Some(RPCResponse::Status(
+                            Version::V1 => {
-                                StatusMessage::from_ssz_bytes(&raw_bytes)?,
+                                if raw_bytes.len() == <StatusMessage as Encode>::ssz_fixed_len() {
-                            ))),
+                                    Ok(Some(RPCResponse::Status(StatusMessage::from_ssz_bytes(
+                                        &raw_bytes,
+                                    )?)))
+                                } else {
+                                    Err(RPCError::InvalidData)
+                                }
+                            }
                         },
                         Protocol::Goodbye => Err(RPCError::InvalidData),
                         Protocol::BlocksByRange => match self.protocol.version {
-                            Version::V1 => Ok(Some(RPCResponse::BlocksByRange(Box::new(
+                            Version::V1 => {
+                                if raw_bytes.len() >= *SIGNED_BEACON_BLOCK_MIN
+                                    && raw_bytes.len() <= *SIGNED_BEACON_BLOCK_MAX
+                                {
+                                    Ok(Some(RPCResponse::BlocksByRange(Box::new(
                                         SignedBeaconBlock::from_ssz_bytes(&raw_bytes)?,
-                            )))),
+                                    ))))
+                                } else {
+                                    Err(RPCError::InvalidData)
+                                }
+                            }
                         },
                         Protocol::BlocksByRoot => match self.protocol.version {
-                            Version::V1 => Ok(Some(RPCResponse::BlocksByRoot(Box::new(
+                            Version::V1 => {
+                                if raw_bytes.len() >= *SIGNED_BEACON_BLOCK_MIN
+                                    && raw_bytes.len() <= *SIGNED_BEACON_BLOCK_MAX
+                                {
+                                    Ok(Some(RPCResponse::BlocksByRoot(Box::new(
                                         SignedBeaconBlock::from_ssz_bytes(&raw_bytes)?,
-                            )))),
+                                    ))))
+                                } else {
+                                    Err(RPCError::InvalidData)
+                                }
+                            }
                         },
                         Protocol::Ping => match self.protocol.version {
-                            Version::V1 => Ok(Some(RPCResponse::Pong(Ping {
+                            Version::V1 => {
+                                if raw_bytes.len() == <Ping as Encode>::ssz_fixed_len() {
+                                    Ok(Some(RPCResponse::Pong(Ping {
                                         data: u64::from_ssz_bytes(&raw_bytes)?,
-                            }))),
+                                    })))
+                                } else {
+                                    Err(RPCError::InvalidData)
+                                }
+                            }
                         },
                         Protocol::MetaData => match self.protocol.version {
-                            Version::V1 => Ok(Some(RPCResponse::MetaData(
+                            Version::V1 => {
-                                MetaData::from_ssz_bytes(&raw_bytes)?,
+                                if raw_bytes.len() == <MetaData<TSpec> as Encode>::ssz_fixed_len() {
-                            ))),
+                                    Ok(Some(RPCResponse::MetaData(MetaData::from_ssz_bytes(
+                                        &raw_bytes,
+                                    )?)))
+                                } else {
+                                    Err(RPCError::InvalidData)
+                                }
+                            }
                         },
                     }
                 }

beacon_node/eth2-libp2p/src/rpc/codec/ssz_snappy.rs

@@ -1,7 +1,10 @@
 use crate::rpc::methods::*;
 use crate::rpc::{
     codec::base::OutboundCodec,
-    protocol::{Encoding, Protocol, ProtocolId, RPCError, Version},
+    protocol::{
+        Encoding, Protocol, ProtocolId, RPCError, Version, BLOCKS_BY_ROOT_REQUEST_MAX,
+        BLOCKS_BY_ROOT_REQUEST_MIN, SIGNED_BEACON_BLOCK_MAX, SIGNED_BEACON_BLOCK_MIN,
+    },
 };
 use crate::rpc::{RPCCodedResponse, RPCRequest, RPCResponse};
 use libp2p::bytes::BytesMut;
@@ -123,29 +126,63 @@ impl<TSpec: EthSpec> Decoder for SSZSnappyInboundCodec<TSpec> {
                 let _read_bytes = src.split_to(n as usize);
                 match self.protocol.message_name {
                     Protocol::Status => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCRequest::Status(StatusMessage::from_ssz_bytes(
+                        Version::V1 => {
+                            if decoded_buffer.len() == <StatusMessage as Encode>::ssz_fixed_len() {
+                                Ok(Some(RPCRequest::Status(StatusMessage::from_ssz_bytes(
                                     &decoded_buffer,
-                        )?))),
+                                )?)))
+                            } else {
+                                Err(RPCError::InvalidData)
+                            }
+                        }
                     },
                     Protocol::Goodbye => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCRequest::Goodbye(
+                        Version::V1 => {
-                            GoodbyeReason::from_ssz_bytes(&decoded_buffer)?,
+                            if decoded_buffer.len() == <GoodbyeReason as Encode>::ssz_fixed_len() {
-                        ))),
+                                Ok(Some(RPCRequest::Goodbye(GoodbyeReason::from_ssz_bytes(
+                                    &decoded_buffer,
+                                )?)))
+                            } else {
+                                Err(RPCError::InvalidData)
+                            }
+                        }
                     },
                     Protocol::BlocksByRange => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCRequest::BlocksByRange(
+                        Version::V1 => {
+                            if decoded_buffer.len()
+                                == <BlocksByRangeRequest as Encode>::ssz_fixed_len()
+                            {
+                                Ok(Some(RPCRequest::BlocksByRange(
                                     BlocksByRangeRequest::from_ssz_bytes(&decoded_buffer)?,
-                        ))),
+                                )))
+                            } else {
+                                Err(RPCError::InvalidData)
+                            }
+                        }
                     },
                     Protocol::BlocksByRoot => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCRequest::BlocksByRoot(BlocksByRootRequest {
+                        Version::V1 => {
+                            if decoded_buffer.len() >= *BLOCKS_BY_ROOT_REQUEST_MIN
+                                && decoded_buffer.len() <= *BLOCKS_BY_ROOT_REQUEST_MAX
+                            {
+                                Ok(Some(RPCRequest::BlocksByRoot(BlocksByRootRequest {
                                     block_roots: VariableList::from_ssz_bytes(&decoded_buffer)?,
-                        }))),
+                                })))
+                            } else {
+                                Err(RPCError::InvalidData)
+                            }
+                        }
                     },
                     Protocol::Ping => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCRequest::Ping(Ping::from_ssz_bytes(
+                        Version::V1 => {
-                            &decoded_buffer,
+                            if decoded_buffer.len() == <Ping as Encode>::ssz_fixed_len() {
-                        )?))),
+                                Ok(Some(RPCRequest::Ping(Ping {
+                                    data: u64::from_ssz_bytes(&decoded_buffer)?,
+                                })))
+                            } else {
+                                Err(RPCError::InvalidData)
+                            }
+                        }
                     },
                     Protocol::MetaData => match self.protocol.version {
                         Version::V1 => {
@@ -268,33 +305,65 @@ impl<TSpec: EthSpec> Decoder for SSZSnappyOutboundCodec<TSpec> {
                 let _read_byts = src.split_to(n as usize);
                 match self.protocol.message_name {
                     Protocol::Status => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCResponse::Status(
+                        Version::V1 => {
-                            StatusMessage::from_ssz_bytes(&decoded_buffer)?,
+                            if decoded_buffer.len() == <StatusMessage as Encode>::ssz_fixed_len() {
-                        ))),
+                                Ok(Some(RPCResponse::Status(StatusMessage::from_ssz_bytes(
-                    },
+                                    &decoded_buffer,
-                    Protocol::Goodbye => {
+                                )?)))
-                        // Goodbye does not have a response
+                            } else {
                                 Err(RPCError::InvalidData)
                             }
+                        }
+                    },
+                    Protocol::Goodbye => Err(RPCError::InvalidData),
                     Protocol::BlocksByRange => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCResponse::BlocksByRange(Box::new(
+                        Version::V1 => {
+                            if decoded_buffer.len() >= *SIGNED_BEACON_BLOCK_MIN
+                                && decoded_buffer.len() <= *SIGNED_BEACON_BLOCK_MAX
+                            {
+                                Ok(Some(RPCResponse::BlocksByRange(Box::new(
                                     SignedBeaconBlock::from_ssz_bytes(&decoded_buffer)?,
-                        )))),
+                                ))))
+                            } else {
+                                Err(RPCError::InvalidData)
+                            }
+                        }
                     },
                     Protocol::BlocksByRoot => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCResponse::BlocksByRoot(Box::new(
+                        Version::V1 => {
+                            if decoded_buffer.len() >= *SIGNED_BEACON_BLOCK_MIN
+                                && decoded_buffer.len() <= *SIGNED_BEACON_BLOCK_MAX
+                            {
+                                Ok(Some(RPCResponse::BlocksByRoot(Box::new(
                                     SignedBeaconBlock::from_ssz_bytes(&decoded_buffer)?,
-                        )))),
+                                ))))
+                            } else {
+                                Err(RPCError::InvalidData)
+                            }
+                        }
                     },
                     Protocol::Ping => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCResponse::Pong(Ping {
+                        Version::V1 => {
+                            if decoded_buffer.len() == <Ping as Encode>::ssz_fixed_len() {
+                                Ok(Some(RPCResponse::Pong(Ping {
                                     data: u64::from_ssz_bytes(&decoded_buffer)?,
-                        }))),
+                                })))
+                            } else {
+                                Err(RPCError::InvalidData)
+                            }
+                        }
                     },
                     Protocol::MetaData => match self.protocol.version {
-                        Version::V1 => Ok(Some(RPCResponse::MetaData(MetaData::from_ssz_bytes(
+                        Version::V1 => {
+                            if decoded_buffer.len() == <MetaData<TSpec> as Encode>::ssz_fixed_len()
+                            {
+                                Ok(Some(RPCResponse::MetaData(MetaData::from_ssz_bytes(
                                     &decoded_buffer,
-                        )?))),
+                                )?)))
+                            } else {
+                                Err(RPCError::InvalidData)
+                            }
+                        }
                     },
                 }
             }

beacon_node/eth2-libp2p/src/rpc/methods.rs

@@ -11,7 +11,7 @@ use std::ops::Deref;
 use types::{Epoch, EthSpec, Hash256, SignedBeaconBlock, Slot};
 
 /// Maximum number of blocks in a single request.
-type MaxRequestBlocks = U1024;
+pub type MaxRequestBlocks = U1024;
 pub const MAX_REQUEST_BLOCKS: u64 = 1024;
 
 /// Maximum length of error message.
@@ -188,7 +188,7 @@ pub struct BlocksByRangeRequest {
 }
 
 /// Request a number of beacon block bodies from a peer.
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Encode, Decode, Clone, Debug, PartialEq)]
 pub struct BlocksByRootRequest {
     /// The list of beacon block bodies being requested.
     pub block_roots: VariableList<Hash256, MaxRequestBlocks>,

beacon_node/eth2-libp2p/src/rpc/mod.rs

@@ -23,8 +23,8 @@ pub(crate) use protocol::{RPCProtocol, RPCRequest};
 
 pub use handler::SubstreamId;
 pub use methods::{
-    BlocksByRangeRequest, BlocksByRootRequest, GoodbyeReason, RPCResponseErrorCode, RequestId,
+    BlocksByRangeRequest, BlocksByRootRequest, GoodbyeReason, MaxRequestBlocks,
-    ResponseTermination, StatusMessage, MAX_REQUEST_BLOCKS,
+    RPCResponseErrorCode, RequestId, ResponseTermination, StatusMessage, MAX_REQUEST_BLOCKS,
 };
 pub use protocol::{Protocol, RPCError};
 

beacon_node/eth2-libp2p/src/rpc/protocol.rs

@@ -9,11 +9,14 @@ use crate::rpc::{
         InboundCodec, OutboundCodec,
     },
     methods::ResponseTermination,
+    MaxRequestBlocks, MAX_REQUEST_BLOCKS,
 };
 use futures::future::Ready;
 use futures::prelude::*;
 use futures::prelude::{AsyncRead, AsyncWrite};
 use libp2p::core::{InboundUpgrade, OutboundUpgrade, ProtocolName, UpgradeInfo};
+use ssz::Encode;
+use ssz_types::VariableList;
 use std::io;
 use std::marker::PhantomData;
 use std::pin::Pin;
@@ -23,7 +26,38 @@ use tokio_util::{
     codec::Framed,
     compat::{Compat, FuturesAsyncReadCompatExt},
 };
-use types::EthSpec;
+use types::{BeaconBlock, EthSpec, Hash256, MainnetEthSpec, Signature, SignedBeaconBlock};
+
+lazy_static! {
+    // Note: Hardcoding the `EthSpec` type for `SignedBeaconBlock` as min/max values is
+    // same across different `EthSpec` implementations.
+    pub static ref SIGNED_BEACON_BLOCK_MIN: usize = SignedBeaconBlock::<MainnetEthSpec> {
+        message: BeaconBlock::empty(&MainnetEthSpec::default_spec()),
+        signature: Signature::empty_signature(),
+    }
+    .as_ssz_bytes()
+    .len();
+    pub static ref SIGNED_BEACON_BLOCK_MAX: usize = SignedBeaconBlock::<MainnetEthSpec> {
+        message: BeaconBlock::full(&MainnetEthSpec::default_spec()),
+        signature: Signature::empty_signature(),
+    }
+    .as_ssz_bytes()
+    .len();
+    pub static ref BLOCKS_BY_ROOT_REQUEST_MIN: usize = BlocksByRootRequest {
+        block_roots: VariableList::<Hash256, MaxRequestBlocks>::from(Vec::<Hash256>::new())
+    }
+    .as_ssz_bytes()
+    .len();
+    pub static ref BLOCKS_BY_ROOT_REQUEST_MAX: usize = BlocksByRootRequest {
+        block_roots: VariableList::<Hash256, MaxRequestBlocks>::from(vec![
+            Hash256::zero();
+            MAX_REQUEST_BLOCKS
+                as usize
+        ])
+    }
+    .as_ssz_bytes()
+    .len();
+}
 
 /// The maximum bytes that can be sent across the RPC.
 const MAX_RPC_SIZE: usize = 1_048_576; // 1M