From 60b7de14cb1834791716beb86fede398a477c06b Mon Sep 17 00:00:00 2001 From: jonaswre Date: Sun, 2 Aug 2026 15:29:34 +0000 Subject: [PATCH] fix: clamp remote sync ranges to the session namespace All namespaces share one records table keyed `(namespace, author, key)`, so the query bounds are the only thing keeping documents apart. But `get_range` builds those bounds from the range endpoints in an incoming `RangeItem`: Ordering::Equal => RecordsBounds::namespace(self.namespace) // correct Ordering::Less => RecordsBounds::new(start, end) // both remote Ordering::Greater => from_start(&self.namespace, end) // end remote to_end(&self.namespace, start) // start remote The endpoints are remote-controlled: `RecordIdentifier` is a raw `Bytes` with a derived `Deserialize`, and `Message::validate_limits` counts parts and entries without ever inspecting `range`. `RecordsBounds::new` is a passthrough and `RecordsRange::with_bounds` applies no post-filter, so nothing downstream catches it. `get_fingerprint` delegates to `get_range` and inherits the same flaw. The computed diff is echoed straight back to the peer, and the namespace pin and signature verification in `validate_entry` apply only to *incoming* entries, never to the outgoing diff. Combined with `accept_request`, which admits any authenticated peer for any namespace in the local sync set with no per-namespace allowlist, a peer holding a ticket for one document can read every document on the node: namespace ids, author keys, record keys, timestamps, content lengths, content hashes and signatures. One frame is enough -- `RangeItem { range: { x: 0x00 * 64, y: 0xFF * 64 }, values: vec![], have_local: false }`. `have_local: false` makes the node compute the diff over those bounds, the empty `values` filters nothing, and the result is returned. `MAX_ENTRIES_PER_SYNC_MESSAGE` caps a reply at 2048 entries, so a larger store is paged by splitting. That is worse than a metadata leak in two ways: `Capability::Read` *is* the namespace id, so leaked ids can be redeemed for full sync sessions through the front door, and leaked content hashes are fetchable from the blobs store. Writes are unaffected -- entry signatures bind both namespace and author, and all three ingress paths verify before `put`. Add `RecordsBounds::clamp_to_namespace` and apply it to every branch that consumes remote input, including both sub-ranges of `Greater`, where `from_start`/`to_end` pin only one side each. An empty intersection -- what a range naming only foreign namespaces clamps to -- normalizes to a range selecting nothing, since inverted bounds are not a valid query. Normal traffic never exercised the gap: sessions start at `Range::new(x, x)` (the `Equal` branch) and recursion only produces in-namespace split points, which is presumably why it went unnoticed. Reconciliation is unaffected: the full suite passes, including `sync_big`, `sync_full_basic`, `sync_gossip_bulk` and `sync_restart_node` (77 lib + 11 integration + doctests). The new test drives all three orderings with endpoints spanning the whole table and asserts nothing outside the session namespace comes back; it was confirmed to fail on unpatched main. Adjacent, not fixed here: `RecordIdentifier(Bytes)` deserializes with no length invariant while `to_byte_tuple`/`namespace()`/`author()` slice `0..32` and `32..64`, so a remote identifier shorter than 64 bytes panics the actor thread. Availability-only, but it shares the root cause and the same validation would close it. --- src/store/fs.rs | 66 ++++++++++++++++++++++++++++++++++++++++-- src/store/fs/bounds.rs | 39 +++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/src/store/fs.rs b/src/store/fs.rs index b9143f96..dc77b3a9 100644 --- a/src/store/fs.rs +++ b/src/store/fs.rs @@ -805,22 +805,31 @@ impl<'a> crate::ranger::Store for StoreInstance<'a> { // regular range: iter1 = x <= t < y, iter2 = none Ordering::Less => { // iterator for entries from range.x to range.y + // + // Both endpoints come from the remote peer, so the bounds have to be + // clamped to our namespace; otherwise a range naming another namespace + // would read that document's entries out of the shared records table. let start = Bound::Included(range.x().to_byte_tuple()); let end = Bound::Excluded(range.y().to_byte_tuple()); - let bounds = RecordsBounds::new(start, end); + let bounds = RecordsBounds::new(start, end).clamp_to_namespace(&self.namespace); let iter = RecordsRange::with_bounds(&tables.records, bounds)?; chain_none(iter) } // split range: iter1 = start <= t < y, iter2 = x <= t <= end Ordering::Greater => { // iterator for entries from start to range.y + // + // `from_start`/`to_end` pin only one side to our namespace, so the + // remote-supplied side still needs clamping. let end = Bound::Excluded(range.y().to_byte_tuple()); - let bounds = RecordsBounds::from_start(&self.namespace, end); + let bounds = RecordsBounds::from_start(&self.namespace, end) + .clamp_to_namespace(&self.namespace); let iter = RecordsRange::with_bounds(&tables.records, bounds)?; // iterator for entries from range.x to end let start = Bound::Included(range.x().to_byte_tuple()); - let bounds = RecordsBounds::to_end(&self.namespace, start); + let bounds = RecordsBounds::to_end(&self.namespace, start) + .clamp_to_namespace(&self.namespace); let iter2 = RecordsRange::with_bounds(&tables.records, bounds)?; iter.chain(Some(iter2).into_iter().flatten()) @@ -1050,6 +1059,57 @@ mod tests { Ok(()) } + /// `get_range` must never return entries outside the namespace it is pinned to. + /// + /// All namespaces share one records table, keyed `(namespace, author, key)`, so the + /// query bounds are the only thing keeping documents apart. The range in a + /// `RangeItem` is remote-supplied and unvalidated, and the resulting diff is echoed + /// straight back to the peer — so a range naming a foreign namespace must not widen + /// what the session can see, in any of the three orderings. + #[test] + fn get_range_stays_within_its_namespace() -> Result<()> { + let dbfile = tempfile::NamedTempFile::new()?; + let mut store = Store::persistent(dbfile.path())?; + let author = store.new_author(&mut rand::rng())?; + + let ours = NamespaceSecret::new(&mut rand::rng()); + let theirs = NamespaceSecret::new(&mut rand::rng()); + store.new_replica(ours.clone())?; + store.new_replica(theirs.clone())?; + for ns in [&ours, &theirs] { + let mut wrapper = StoreInstance::new(ns.id(), &mut store); + let id = RecordIdentifier::new(ns.id(), author.id(), b"key"); + let entry = Entry::new(id, Record::current_from_data(b"value")); + wrapper.entry_put(SignedEntry::from_entry(entry, ns, &author))?; + } + + // Endpoints spanning the whole table, i.e. naming neither namespace in + // particular. `min < max`, so swapping them walks each of the two branches that + // build bounds out of remote input. + let min = RecordIdentifier::new(NamespaceId::from(&[0u8; 32]), author.id(), b""); + let max = RecordIdentifier::new(NamespaceId::from(&[255u8; 32]), author.id(), b""); + + let mut wrapper = StoreInstance::new(ours.id(), &mut store); + for (label, range) in [ + ("less", Range::new(min.clone(), max.clone())), + ("greater", Range::new(max.clone(), min.clone())), + ("equal", Range::new(min.clone(), min.clone())), + ] { + let leaked = wrapper + .get_range(range)? + .map(|entry| entry.map(|e| e.namespace())) + .collect::>>()? + .into_iter() + .filter(|ns| *ns != ours.id()) + .collect::>(); + assert!( + leaked.is_empty(), + "{label} range leaked entries from another namespace: {leaked:?}" + ); + } + Ok(()) + } + #[test] fn test_basics() -> Result<()> { let dbfile = tempfile::NamedTempFile::new()?; diff --git a/src/store/fs/bounds.rs b/src/store/fs/bounds.rs index f8234512..a7737842 100644 --- a/src/store/fs/bounds.rs +++ b/src/store/fs/bounds.rs @@ -61,6 +61,45 @@ impl RecordsBounds { Self::new(start, Self::namespace_end(ns)) } + /// Intersect these bounds with `ns`. + /// + /// Every namespace shares the same records table, so the bounds are the only thing + /// keeping documents apart. Sync range endpoints arrive from the remote peer + /// unvalidated and may name any namespace, so they have to be intersected with the + /// namespace the session is pinned to: a range reaching into another document then + /// selects nothing instead of reading it. + pub fn clamp_to_namespace(self, ns: &NamespaceId) -> Self { + let Self(start, end) = self; + // Both `namespace_start` and every caller's start are `Included`; the tighter of + // two lower bounds is the greater one. Unexpected shapes fall back to the + // namespace bound, which is never wider than what was asked for. + let start = match (start, Self::namespace_start(ns)) { + (Bound::Included(remote), Bound::Included(ns_start)) => { + Bound::Included(remote.max(ns_start)) + } + (_, ns_start) => ns_start, + }; + // `namespace_end` is `Excluded`, or `Unbounded` for the last namespace, in which + // case the remote's own end is already within it. + let end = match (end, Self::namespace_end(ns)) { + (Bound::Excluded(remote), Bound::Excluded(ns_end)) => { + Bound::Excluded(remote.min(ns_end)) + } + (Bound::Excluded(remote), Bound::Unbounded) => Bound::Excluded(remote), + (_, ns_end) => ns_end, + }; + // The intersection is empty whenever the range covers only foreign namespaces. + // Normalize that to a range selecting nothing, as inverted bounds are not a + // valid query. + if let (Bound::Included(s), Bound::Excluded(e)) = (&start, &end) { + if s >= e { + let empty = (ns.to_bytes(), [0u8; 32], Bytes::new()); + return Self(Bound::Included(empty.clone()), Bound::Excluded(empty)); + } + } + Self(start, end) + } + pub fn as_ref(&self) -> (Bound>, Bound>) { fn map(id: &RecordsIdOwned) -> RecordsId<'_> { (&id.0, &id.1, &id.2[..])