From 034eb418600f01ffc27b84ad399372410d49cd13 Mon Sep 17 00:00:00 2001 From: cloudhead Date: Mon, 7 Oct 2024 18:11:59 +0200 Subject: [PATCH] node: Ensure private RIDs don't leak in gossip Though refs of private repositories are not announced publicly, they can sometimes be relayed to nodes that are not in the allow list. We fix this by always checking the visibility of a repository before sending a refs announcement of it to a peer. Discovered-by: Adrian Duke --- radicle-node/src/service.rs | 27 ++++++++++-- radicle-node/src/tests.rs | 84 ++++++++++++++++++++++++++++++++++--- 2 files changed, 102 insertions(+), 9 deletions(-) diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index 4727fc21..0b2bf7ff 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -2312,9 +2312,14 @@ where self.last_timestamp } - fn relay(&mut self, id: gossip::AnnouncementId, msg: Announcement) { - let announcer = msg.node; + fn relay(&mut self, id: gossip::AnnouncementId, ann: Announcement) { + let announcer = ann.node; let relayed_by = self.relayed_by.get(&id); + let rid = if let AnnouncementMessage::Refs(RefsAnnouncement { rid, .. }) = ann.message { + Some(rid) + } else { + None + }; // Choose peers we should relay this message to. // 1. Don't relay to a peer who sent us this message. // 2. Don't relay to the peer who signed this announcement. @@ -2327,9 +2332,25 @@ where .unwrap_or(true) // If there are no relayers we let it through. }) .filter(|(id, _)| **id != announcer) + .filter(|(id, _)| { + if let Some(rid) = rid { + // Only relay this message if the peer is allowed to know about the + // repository. If we don't have the repository, return `false` because + // we can't determine if it's private or public. + self.storage + .get(rid) + .ok() + .flatten() + .map(|doc| doc.is_visible_to(id)) + .unwrap_or(false) + } else { + // Announcement doesn't concern a specific repository, let it through. + true + } + }) .map(|(_, p)| p); - self.outbox.relay(msg, relay_to); + self.outbox.relay(ann, relay_to); } //////////////////////////////////////////////////////////////////////////// diff --git a/radicle-node/src/tests.rs b/radicle-node/src/tests.rs index da243612..d6d77acb 100644 --- a/radicle-node/src/tests.rs +++ b/radicle-node/src/tests.rs @@ -671,13 +671,9 @@ fn test_announcement_relay() { } #[test] -fn test_refs_announcement_relay() { +fn test_refs_announcement_relay_public() { let tmp = tempfile::tempdir().unwrap(); - let mut alice = Peer::with_storage( - "alice", - [7, 7, 7, 7], - Storage::open(tmp.path().join("alice"), fixtures::user()).unwrap(), - ); + let mut alice = Peer::with_storage("alice", [7, 7, 7, 7], MockStorage::empty()); let eve = Peer::with_storage( "eve", [8, 8, 8, 8], @@ -713,6 +709,12 @@ fn test_refs_announcement_relay() { .receive(bob.id(), bob.refs_announcement(bob_inv[0])) .elapse(service::GOSSIP_INTERVAL); + // Pretend Alice cloned Bob's repos. + let repos = gen::<[MockRepository; 3]>(1); + for (i, mut repo) in repos.into_iter().enumerate() { + repo.doc.doc.visibility = Visibility::Public; // Public repos are always gossiped. + alice.storage_mut().repos.insert(bob_inv[i], repo); + } assert_matches!( alice.messages(eve.id()).next(), Some(Message::Announcement(_)), @@ -746,6 +748,76 @@ fn test_refs_announcement_relay() { ); } +#[test] +fn test_refs_announcement_relay_private() { + let tmp = tempfile::tempdir().unwrap(); + let mut alice = Peer::with_storage("alice", [7, 7, 7, 7], MockStorage::empty()); + let eve = Peer::with_storage( + "eve", + [8, 8, 8, 8], + Storage::open(tmp.path().join("eve"), fixtures::user()).unwrap(), + ); + + let bob = { + let mut rng = fastrand::Rng::new(); + let signer = MockSigner::new(&mut rng); + let storage = fixtures::storage(tmp.path().join("bob"), &signer).unwrap(); + + Peer::config( + "bob", + [9, 9, 9, 9], + storage, + peer::Config { + signer, + rng, + ..peer::Config::default() + }, + ) + .initialized() + }; + let bob_inv = bob.inventory().into_iter().collect::>(); + + alice.seed(&bob_inv[0], policy::Scope::All).unwrap(); + alice.seed(&bob_inv[1], policy::Scope::All).unwrap(); + alice.connect_to(&bob); + alice.connect_to(&eve); + alice.receive(eve.id(), Message::Subscribe(Subscribe::all())); + + // The first repo is not visible to Eve. + let mut repo1 = gen::(1); + repo1.doc.doc.visibility = Visibility::Private { allow: [].into() }; + alice.storage_mut().repos.insert(bob_inv[0], repo1); + + // The second repo is visible to Eve. + let mut repo2 = gen::(1); + repo2.doc.doc.visibility = Visibility::Private { + allow: [eve.id.into()].into(), + }; + alice.storage_mut().repos.insert(bob_inv[1], repo2); + alice.elapse(service::GOSSIP_INTERVAL); + alice.messages(eve.id()).for_each(drop); + alice + .receive(bob.id(), bob.refs_announcement(bob_inv[0])) + .elapse(service::GOSSIP_INTERVAL); + assert_matches!( + alice.messages(eve.id()).next(), + None, + "The first ref announcement is not relayed to Eve" + ); + + alice + .receive(bob.id(), bob.refs_announcement(bob_inv[1])) + .elapse(service::GOSSIP_INTERVAL); + assert_matches!( + alice.messages(eve.id()).next(), + Some(Message::Announcement(Announcement { + message: AnnouncementMessage::Refs(_), + .. + })), + "The second ref announcement is relayed to Eve" + ); +} + /// Even if Alice is not tracking Bob, Alice will fetch Bob's refs for a repo she doesn't have. #[test] fn test_refs_announcement_fetch_trusted_no_inventory() {