Repository: radicle-dev/heartwood (rust)

Issue:
Check all lengths when decoding
To prevent OOM errors, we should check all lengths in the `wire` protocol. Currently, lengths are capped to 16bits, but for certain types of data, that could still be a lot, eg. when sending over public keys or hashes.

Pull request description:
node: Limit number of Refs in RefAnnouncements
Use the BoundedVec to limit the supported number of refs in RefAnnouncement messages.  The size (236) is the maximum supported by message wire size.

To reduce the complexity of the changes, BoundedVec is used again instead of using a new bounded Refs or BTreeMap.  This requires defining new Decode/Encode logic for Ref's element types, and workarounds for arbitrary instances.

Signed-off-by: Slack Coder <slackcoder@server.ky>

closes #16

Patch:
diff --git a/radicle-node/src/bounded.rs b/radicle-node/src/bounded.rs
index 01ac79c8..35b32207 100644
--- a/radicle-node/src/bounded.rs
+++ b/radicle-node/src/bounded.rs
@@ -18,6 +18,25 @@ impl<T, const N: usize> BoundedVec<T, N> {
         BoundedVec { v: Vec::new() }
     }
 
+    /// Build a `BoundedVec` by consuming from the given iterator up to its limit.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use radicle_node::bounded;
+    ///
+    /// let mut iter = (0..4).into_iter();
+    /// let bounded: bounded::BoundedVec<i32,3> = bounded::BoundedVec::collect_from(&mut iter);
+    ///
+    /// assert_eq!(bounded.len(), 3);
+    /// assert_eq!(iter.count(), 1);
+    /// ```
+    pub fn collect_from<I: Iterator<Item = T>>(iter: &mut I) -> Self {
+        BoundedVec {
+            v: iter.into_iter().take(N).collect(),
+        }
+    }
+
     /// Create a new `BoundedVec<T,N>` which takes upto the first N values of its argument, taking
     /// ownership.
     ///
diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs
index f86fd33e..9b3b6aa1 100644
--- a/radicle-node/src/service.rs
+++ b/radicle-node/src/service.rs
@@ -73,6 +73,8 @@ pub const MAX_CONNECTION_ATTEMPTS: usize = 3;
 pub use message::ADDRESS_LIMIT;
 /// Maximum inventory limit imposed by message size limits.
 pub use message::INVENTORY_LIMIT;
+/// Maximum number of project git references imposed by message size limits.
+pub use message::REF_LIMIT;
 
 /// A service event.
 #[derive(Debug, Clone)]
@@ -959,12 +961,22 @@ where
 
     /// Announce local refs for given id.
     fn announce_refs(&mut self, id: Id) -> Result<(), storage::Error> {
+        type Refs = BoundedVec<Id, REF_LIMIT>;
+
         let node = self.node_id();
         let repo = self.storage.repository(id)?;
         let remote = repo.remote(&node)?;
         let peers = self.sessions.negotiated().map(|(_, _, p)| p);
-        let refs = remote.refs.into();
         let timestamp = self.clock.as_secs();
+
+        if remote.refs.len() > Refs::max() {
+            log::error!(
+                "refs announcement limit ({}) exceeded, other nodes will see only some of your project references",
+                Refs::max(),
+            );
+        }
+        let refs = BoundedVec::collect_from(&mut remote.refs.iter().map(|(a, b)| (a.clone(), *b)));
+
         let msg = AnnouncementMessage::from(RefsAnnouncement {
             id,
             refs,
diff --git a/radicle-node/src/service/message.rs b/radicle-node/src/service/message.rs
index 7794fb81..d7d6255e 100644
--- a/radicle-node/src/service/message.rs
+++ b/radicle-node/src/service/message.rs
@@ -4,16 +4,18 @@ use std::{fmt, io, mem, net};
 use thiserror::Error;
 
 use crate::crypto;
+use crate::git;
 use crate::identity::Id;
 use crate::node;
 use crate::prelude::BoundedVec;
 use crate::service::filter::Filter;
 use crate::service::{NodeId, Timestamp, PROTOCOL_VERSION};
-use crate::storage::refs::Refs;
 use crate::wire;
 
 /// Maximum number of addresses which can be announced to other nodes.
 pub const ADDRESS_LIMIT: usize = 16;
+/// Maximum number of project git references.
+pub const REF_LIMIT: usize = 235;
 /// Maximum number of inventory which can be announced to other nodes.
 pub const INVENTORY_LIMIT: usize = 2973;
 
@@ -225,7 +227,7 @@ pub struct RefsAnnouncement {
     /// Repository identifier.
     pub id: Id,
     /// Updated refs.
-    pub refs: Refs,
+    pub refs: BoundedVec<(git::RefString, git::Oid), REF_LIMIT>,
     /// Time of announcement.
     pub timestamp: Timestamp,
 }
@@ -508,8 +510,43 @@ mod tests {
 
     use crate::crypto::test::signer::MockSigner;
     use crate::test::arbitrary;
+    use fastrand;
     use qcheck_macros::quickcheck;
 
+    #[test]
+    fn test_ref_limit() {
+        let mut refs = Refs::default();
+        while refs.len() < REF_LIMIT {
+            refs.insert(arbitrary::refstring(u8::MAX as usize), arbitrary::oid());
+        }
+
+        let bounded_refs = BoundedVec::collect_from(&mut refs.iter().map(|(a, b)| (a.clone(), *b)));
+        let msg: Message = AnnouncementMessage::from(RefsAnnouncement {
+            id: arbitrary::gen(1),
+            refs: bounded_refs,
+            timestamp: LocalTime::now().as_secs(),
+        })
+        .signed(&MockSigner::default())
+        .into();
+
+        let mut buf: Vec<u8> = Vec::new();
+        assert!(
+            msg.encode(&mut buf).is_ok(),
+            "REF_LIMIT is too big to support message encoding",
+        );
+
+        let decoded = wire::deserialize(buf.as_slice());
+        assert!(
+            decoded.is_ok(),
+            "REF_LIMIT is too big to support message decoding"
+        );
+        assert_eq!(
+            msg,
+            decoded.unwrap(),
+            "encoding and decoding should be safe for message at REF_LIMIT",
+        );
+    }
+
     #[test]
     fn test_inventory_limit() {
         let msg = Message::inventory(
@@ -543,9 +580,10 @@ mod tests {
     fn prop_refs_announcement_signing(id: Id, refs: Refs) {
         let signer = MockSigner::new(&mut fastrand::Rng::new());
         let timestamp = 0;
+
         let message = AnnouncementMessage::Refs(RefsAnnouncement {
             id,
-            refs,
+            refs: BoundedVec::collect_from(&mut refs.iter().map(|(k, v)| (k.clone(), *v))),
             timestamp,
         });
         let ann = message.signed(&signer);
diff --git a/radicle-node/src/wire.rs b/radicle-node/src/wire.rs
index a85f4a07..ea22b3f6 100644
--- a/radicle-node/src/wire.rs
+++ b/radicle-node/src/wire.rs
@@ -226,6 +226,24 @@ impl Encode for Refs {
     }
 }
 
+impl<A, B> Encode for (A, B)
+where
+    A: Encode,
+    B: Encode,
+{
+    fn encode<W: io::Write + ?Sized>(&self, writer: &mut W) -> Result<usize, io::Error> {
+        let mut n = self.0.encode(writer)?;
+        n += self.1.encode(writer)?;
+        Ok(n)
+    }
+}
+
+impl Encode for git::RefString {
+    fn encode<W: io::Write + ?Sized>(&self, writer: &mut W) -> Result<usize, io::Error> {
+        self.as_str().encode(writer)
+    }
+}
+
 impl Encode for Signature {
     fn encode<W: io::Write + ?Sized>(&self, writer: &mut W) -> Result<usize, io::Error> {
         self.deref().encode(writer)
@@ -266,6 +284,25 @@ impl Decode for Refs {
     }
 }
 
+impl Decode for git::RefString {
+    fn decode<R: io::Read + ?Sized>(reader: &mut R) -> Result<Self, Error> {
+        let ref_str = String::decode(reader)?;
+        git::RefString::try_from(ref_str).map_err(Error::from)
+    }
+}
+
+impl<A, B> Decode for (A, B)
+where
+    A: Decode,
+    B: Decode,
+{
+    fn decode<R: io::Read + ?Sized>(reader: &mut R) -> Result<Self, Error> {
+        let a = A::decode(reader)?;
+        let b = B::decode(reader)?;
+        Ok((a, b))
+    }
+}
+
 impl Decode for git::Oid {
     fn decode<R: io::Read + ?Sized>(reader: &mut R) -> Result<Self, Error> {
         let len = Size::decode(reader)? as usize;
@@ -628,6 +665,7 @@ impl<R, S, W, G, H: Handshake> Iterator for Wire<R, S, W, G, H> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use qcheck;
     use qcheck_macros::quickcheck;
 
     use crate::crypto::Unverified;
@@ -700,6 +738,14 @@ mod tests {
         assert_eq!(deserialize::<Refs>(&serialize(&input)).unwrap(), input);
     }
 
+    #[quickcheck]
+    fn prop_tuple(input: (String, String)) {
+        assert_eq!(
+            deserialize::<(String, String)>(&serialize(&input)).unwrap(),
+            input
+        );
+    }
+
     #[quickcheck]
     fn prop_signature(input: [u8; 64]) {
         let signature = Signature::from(input);
diff --git a/radicle-node/src/wire/message.rs b/radicle-node/src/wire/message.rs
index e4086846..56dae578 100644
--- a/radicle-node/src/wire/message.rs
+++ b/radicle-node/src/wire/message.rs
@@ -130,7 +130,7 @@ impl wire::Encode for RefsAnnouncement {
 impl wire::Decode for RefsAnnouncement {
     fn decode<R: std::io::Read + ?Sized>(reader: &mut R) -> Result<Self, wire::Error> {
         let id = Id::decode(reader)?;
-        let refs = Refs::decode(reader)?;
+        let refs = BoundedVec::decode(reader)?;
         let timestamp = Timestamp::decode(reader)?;
 
         Ok(Self {


Test patch:
diff --git a/radicle-node/src/test/arbitrary.rs b/radicle-node/src/test/arbitrary.rs
index 0b9e85d7..141cae08 100644
--- a/radicle-node/src/test/arbitrary.rs
+++ b/radicle-node/src/test/arbitrary.rs
@@ -56,7 +56,9 @@ impl Arbitrary for Message {
                 node: NodeId::arbitrary(g),
                 message: RefsAnnouncement {
                     id: Id::arbitrary(g),
-                    refs: Refs::arbitrary(g),
+                    refs: BoundedVec::collect_from(
+                        &mut Refs::arbitrary(g).iter().map(|(k, v)| (k.clone(), *v)),
+                    ),
                     timestamp: Timestamp::arbitrary(g),
                 }
                 .into(),
diff --git a/radicle-node/src/test/peer.rs b/radicle-node/src/test/peer.rs
index 1f47aa79..b2d47077 100644
--- a/radicle-node/src/test/peer.rs
+++ b/radicle-node/src/test/peer.rs
@@ -1,5 +1,4 @@
 #![allow(dead_code)]
-use std::collections::BTreeMap;
 use std::iter;
 use std::net;
 use std::ops::{Deref, DerefMut};
@@ -13,7 +12,7 @@ use crate::crypto::test::signer::MockSigner;
 use crate::crypto::Signer;
 use crate::identity::Id;
 use crate::node;
-use crate::prelude::NodeId;
+use crate::prelude::*;
 use crate::service;
 use crate::service::message::*;
 use crate::service::reactor::Io;
@@ -214,7 +213,7 @@ where
     }
 
     pub fn refs_announcement(&self, id: Id) -> Message {
-        let refs = BTreeMap::new().into();
+        let refs = BoundedVec::new();
         let ann = AnnouncementMessage::from(RefsAnnouncement {
             id,
             refs,
diff --git a/radicle/src/test/arbitrary.rs b/radicle/src/test/arbitrary.rs
index 80f2fce5..6104e088 100644
--- a/radicle/src/test/arbitrary.rs
+++ b/radicle/src/test/arbitrary.rs
@@ -15,6 +15,23 @@ use crate::storage;
 use crate::storage::refs::{Refs, SignedRefs};
 use crate::test::storage::MockStorage;
 
+pub fn oid() -> storage::Oid {
+    let oid_bytes: [u8; 20] = gen(1);
+    storage::Oid::try_from(oid_bytes.as_slice()).unwrap()
+}
+
+pub fn refstring(len: usize) -> git::RefString {
+    let mut buf = Vec::<u8>::new();
+    for _ in 0..len {
+        buf.push(fastrand::u8(0x61..0x7a));
+    }
+    std::str::from_utf8(&buf)
+        .unwrap()
+        .to_string()
+        .try_into()
+        .unwrap()
+}
+
 pub fn set<T: Eq + Hash + Arbitrary>(range: impl RangeBounds<usize>) -> HashSet<T> {
     let size = fastrand::usize(range);
     let mut set = HashSet::with_capacity(size);