From 323ec3867cb84b1d36a241526ef34cbbaa125252 Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Wed, 23 Sep 2026 04:42:41 +0200 Subject: [PATCH] R-AF/R-AG: Rust resolves references by the wire's actual rule; checked cursor arithmetic R-AF. Probed the real wire bytes a stored CultRecordRef produces (GameCult.Caching.MessagePack.CultRecordRefFormatter.Serialize writes only value.Key.Value - a probe against a real document confirmed the serialized "Design" field is the bare msgpack string "a-eq", nothing else). The wire carries no schema for a reference; C#'s rule was right and Rust's exact-(schema,key) lookup assumed a schema id the edge never actually carries. Row::references() now returns (role, target_record_key, payload) - a bare key, matching C#'s ReferencesOf. matches_citation/build_incoming_index resolve through the new resolve_reference_target, which mirrors C#'s TryResolveReferenceTarget: search every row sharing the key (by_key is now keyed by record_key alone, grouping rows across schemas) for the one whose schema falls inside the declared leaf set, refusing deterministically (by schema id, code-point order) when none do. Folds D9's leaf-set check into resolution itself rather than a second ensure_within_declared_target pass - deleted, since it also dropped the old bypass-on-empty-leaves behaviour C# never had (empty leaves always refuses when a candidate exists). Also moved the citation's own role filter before resolution in matches_citation (mirroring C#'s ReferenceMembers(descriptor, citation.Role), which narrows to that role's members before R-W's per-edge check ever runs) - found while landing R-AF's vectors: Rust was checking every declared reference a row carries regardless of the requested role, a stronger and different guarantee than the C# reference actually gives. R-AG. read_length_prefixed_fields used unchecked usize addition (pos + len) to bound a length prefix parsed from the wire; a hostile prefix near usize::MAX overflowed and panicked. Uses checked_add now, returning None (which Cursor::parse already turns into a typed CursorInvalid) the same way every other malformed shape here already does. Co-Authored-By: Claude Sonnet 5 --- packages/cultnet-rs/src/selection.rs | 199 +++++++++++++++++----- packages/cultnet-rs/src/snapshot_query.rs | 4 +- 2 files changed, 156 insertions(+), 47 deletions(-) diff --git a/packages/cultnet-rs/src/selection.rs b/packages/cultnet-rs/src/selection.rs index 668f5453..c3c27b1c 100644 --- a/packages/cultnet-rs/src/selection.rs +++ b/packages/cultnet-rs/src/selection.rs @@ -1016,10 +1016,17 @@ pub trait Row { /// This row's declared numeric value at `index`, as its exact canonical decimal (Q-J) - never /// `f64`. `None` when the row has no value, or the value is NaN/infinite. fn number(&self, index: &str) -> Option; - /// Every declared reference edge this row carries: `(role, target, payload)`. A one-reference - /// yields one entry, a many-reference one per element, a dictionary reference one per key with - /// its value as the payload (D11). - fn references(&self) -> Vec<(String, RecordRef, Option>)>; + /// Every declared reference edge this row carries: `(role, target_record_key, payload)`. A + /// one-reference yields one entry, a many-reference one per element, a dictionary reference one + /// per key with its value as the payload (D11). + /// + /// R-AF: the target is a bare record key, not a `RecordRef` - a stored reference carries no + /// schema id on the wire (`GameCult.Caching.MessagePack.CultRecordRefFormatter` serializes a + /// `CultRecordRef` as only its key string; probed against real serialized bytes, + /// docs/cultnet-selection-cut.md, "R-AF"). `resolve_reference_target` turns this bare key into a + /// resolved row, by record key plus the declared leaf set (D9) - never by an exact (schema, key) + /// pair, which would assume a schema the edge never actually carries. + fn references(&self) -> Vec<(String, String, Option>)>; } impl Row for &T { @@ -1041,7 +1048,7 @@ impl Row for &T { fn number(&self, index: &str) -> Option { (**self).number(index) } - fn references(&self) -> Vec<(String, RecordRef, Option>)> { + fn references(&self) -> Vec<(String, String, Option>)> { (**self).references() } } @@ -1468,12 +1475,16 @@ pub fn select( ) -> Result, SelectionRefusal> { validate(selection, row_set)?; - // R-V: a row's identity is `(schemaId, recordKey)`, never the key alone - CultCache keys are - // unique per schema, so two schemas can share one record key and must not collide here. - let by_key: HashMap<(&str, &str), &R> = all_rows - .iter() - .map(|row| ((row.schema_id(), row.record_key()), row)) - .collect(); + // R-AF: keyed by record key alone, not `(schemaId, recordKey)` - a stored reference edge + // carries no schema id on the wire (see `Row::references`'s doc comment), so resolving "the + // row this edge names" must search every row sharing a key, not look one up by a pair the edge + // never actually supplies. Two schemas legitimately sharing one record key both live under + // that key's entry; `resolve_reference_target` is what picks the one inside the declared + // target. + let mut by_key: HashMap<&str, Vec<&R>> = HashMap::new(); + for row in all_rows { + by_key.entry(row.record_key()).or_default().push(row); + } let mut candidates: Vec<&R> = all_rows .iter() @@ -1730,53 +1741,83 @@ fn compare_position(row: &R, cursor: &Cursor) -> Ordering { .then_with(|| row.record_key().cmp(cursor.record_key.as_str())) } -// D9: a reference's target set is every schema `RowSet::target_leaves` names for that role. A -// stored edge naming a row whose schema is outside that set refuses the selection (S18). -fn ensure_within_declared_target( +// D9/R-AF: a reference's target set is every schema `RowSet::target_leaves` names for that role. +// A stored edge names only a bare record key (see `Row::references`'s doc comment) - resolving +// "the row this edge names" means searching every row sharing that key for the one whose schema +// falls inside the declared set, not an exact (schema, key) lookup that assumes a schema the edge +// never carries. Two schemas legitimately sharing one record key resolve to two different rows +// depending on which reference declared which target. Folds D9's leaf-set check into resolution +// itself (mirroring C#'s `TryResolveReferenceTarget`) rather than a second pass after the fact - +// there is exactly one row-owner decision here, not two. +// +// R-T: every resolvable edge is checked against its declared target, not only the ones a +// citation's own role/key filter happens to ask about - a corrupt edge must not hide behind an +// unrelated filter (F15: the pre-fix evaluator skipped this check for every edge that did not +// point at the requested key). `matches_citation`/`build_incoming_index` call this +// unconditionally, before any role/key/schema filtering, for exactly that reason. +fn resolve_reference_target<'a, R: Row>( row_set: &impl RowSet, + by_key: &HashMap<&str, Vec<&'a R>>, role: &str, from: &R, - to: &R, -) -> Result<(), SelectionRefusal> { + record_key: &str, +) -> Result, SelectionRefusal> { + let Some(candidates) = by_key.get(record_key) else { + return Ok(None); // a dangling reference - no row at all exists at this key (not an error). + }; let leaves = row_set.target_leaves(role); - if leaves.is_empty() || leaves.iter().any(|leaf| leaf == to.schema_id()) { - return Ok(()); + for &candidate in candidates { + if leaves.iter().any(|leaf| leaf == candidate.schema_id()) { + return Ok(Some(candidate)); + } } + // Every row sharing this record key falls outside the reference's declared target: refuse + // deterministically (by schema id, code-point order - Rust's byte-wise `str` ordering already + // matches `CultNetCodePointComparer` for well-formed UTF-8, R-C) rather than let `by_key`'s + // insertion order decide which of them the refusal names (both row orders give the same + // answer). + let outside = candidates + .iter() + .min_by(|a, b| a.schema_id().cmp(b.schema_id())) + .expect("by_key never stores an empty Vec - entry()/push() always leaves at least one"); Err(SelectionRefusal::ReferenceOutsideTarget { from_schema_id: from.schema_id().to_string(), from_key: from.record_key().to_string(), role: role.to_string(), - to_schema_id: to.schema_id().to_string(), - to_key: to.record_key().to_string(), + to_schema_id: outside.schema_id().to_string(), + to_key: outside.record_key().to_string(), }) } fn matches_citation( row_set: &impl RowSet, - by_key: &HashMap<(&str, &str), &R>, + by_key: &HashMap<&str, Vec<&R>>, citer: &R, citation: &Citation, edge_sink: &mut Vec>, ) -> Result { let mut found = false; - for (role, target, payload) in citer.references() { - // R-V: the edge's own declared target is resolved by (schemaId, recordKey), not the key - // alone, so a row of the wrong schema sharing this record key can never stand in for it. - let Some(resolved) = by_key.get(&(target.schema_id.as_str(), target.record_key.as_str())) - else { - continue; - }; - // R-T: every resolvable edge is checked against its declared target, not only the ones - // the citation's own role/key filters happen to ask about - a corrupt edge must not hide - // behind an unrelated filter (F15: the pre-fix evaluator skipped this check for every - // edge that did not point at the requested key). - ensure_within_declared_target(row_set, &role, citer, *resolved)?; + for (role, target_key, payload) in citer.references() { + // R-T/R-W parity fix (found landing R-AF): the citation's own role filter is checked + // *before* resolving the edge, mirroring C#'s ReferenceMembers(descriptor, citation.Role) - + // which narrows to that role's declared members before any of R-W's per-edge target check + // ever runs (CultNetSelectionEvaluator.cs:451-458, MatchesCitation's own foreach). This + // module used to resolve every edge unconditionally regardless of the requested role, which + // is a stronger and different guarantee than the reference ever gives: C#'s own R-T(F15) + // test (Evaluator_CitesRefusesAnOutOfTargetEdgeEvenWhenTheQueriedKeyMatchesNeitherEdge) + // queries the *same* role/member that carries the corrupt edge, never an unrelated one - R-W + // still checks every edge of the *requested* role unconditionally (untouched below), just + // not every edge the row happens to carry under other roles when a role was asked for. if let Some(wanted_role) = &citation.role && *wanted_role != role { continue; } - if target.record_key != citation.target.record_key { + let Some(resolved) = resolve_reference_target(row_set, by_key, &role, citer, &target_key)? + else { + continue; + }; + if target_key != citation.target.record_key { continue; } // R-E: the target's schema id is matched through the one alias rule, not exact equality. @@ -1789,7 +1830,7 @@ fn matches_citation( edge_sink.push(EdgeMatch { from: citer.clone(), role, - to: (*resolved).clone(), + to: resolved.clone(), payload, anchor: EdgeAnchor::Citer, }); @@ -1804,28 +1845,25 @@ fn matches_citation( fn build_incoming_index( row_set: &impl RowSet, all_rows: &[R], - by_key: &HashMap<(&str, &str), &R>, + by_key: &HashMap<&str, Vec<&R>>, role: &str, edge_sink: &mut Vec>, ) -> Result, SelectionRefusal> { let mut incoming = HashSet::new(); for citer in all_rows { - for (edge_role, target, payload) in citer.references() { + for (edge_role, target_key, payload) in citer.references() { if edge_role != role { continue; } - // R-V: resolved by (schemaId, recordKey) - see the matching comment in - // `matches_citation`. - let Some(resolved) = by_key.get(&(target.schema_id.as_str(), target.record_key.as_str())) + let Some(resolved) = resolve_reference_target(row_set, by_key, &edge_role, citer, &target_key)? else { continue; }; - ensure_within_declared_target(row_set, &edge_role, citer, *resolved)?; incoming.insert((resolved.schema_id().to_string(), resolved.record_key().to_string())); edge_sink.push(EdgeMatch { from: citer.clone(), role: edge_role, - to: (*resolved).clone(), + to: resolved.clone(), payload, anchor: EdgeAnchor::Citee, }); @@ -2043,11 +2081,17 @@ fn read_length_prefixed_fields(bytes: &[u8], count: usize) -> Option let colon = bytes[pos..].iter().position(|&b| b == b':')?; let len: usize = std::str::from_utf8(&bytes[pos..pos + colon]).ok()?.parse().ok()?; pos += colon + 1; - if pos + len > bytes.len() { + // R-AG: checked arithmetic - an attacker-supplied prefix near usize::MAX makes `pos + len` + // overflow. In a release build that wraps silently instead of panicking on the add itself, + // but the wrapped (small) sum then passes this guard and `&bytes[pos..pos + len]` below + // panics on the out-of-bounds slice anyway; `checked_add` catches both paths the same way + // every other malformed shape here already returns `None` instead of unwinding. + let end = pos.checked_add(len)?; + if end > bytes.len() { return None; } - fields.push(std::str::from_utf8(&bytes[pos..pos + len]).ok()?.to_string()); - pos += len; + fields.push(std::str::from_utf8(&bytes[pos..end]).ok()?.to_string()); + pos = end; } if pos != bytes.len() { return None; @@ -2110,4 +2154,69 @@ mod cursor_tests { let parsed = Cursor::parse(&genuine).expect("well-formed body"); assert!(parsed.verify_digest(&selection, &key)); } + + // R-AG: a length-prefixed field with a hostile prefix, first 4 fields well-formed so + // `read_length_prefixed_fields` reaches the hostile one mid-loop rather than failing earlier + // for an unrelated reason. + fn cursor_with_hostile_last_field(hostile_field: &str) -> String { + let mut body = String::new(); + write_length_prefixed(&mut body, "1"); + write_length_prefixed(&mut body, "5"); + write_length_prefixed(&mut body, "schema-a"); + write_length_prefixed(&mut body, "k1"); + body.push_str(hostile_field); + URL_SAFE_NO_PAD.encode(body.as_bytes()) + } + + // R-AG: Rust's own proven overflow (`selection.rs`, before this cut) - a length prefix of + // `usize::MAX` makes `pos + len` overflow; `checked_add` must refuse typed, never panic. + #[test] + fn hostile_length_prefix_near_usize_max_refuses_typed_not_panic() { + let cursor = cursor_with_hostile_last_field("18446744073709551615:x"); + let result = std::panic::catch_unwind(|| Cursor::parse(&cursor)); + match result { + Ok(Err(SelectionRefusal::CursorInvalid { .. })) => {} + other => panic!("expected a typed CursorInvalid refusal, not a panic or acceptance: {other:?}"), + } + } + + // R-AG: the mirror-image proven overflow, C#'s (`CultNetSelectionEvaluator.cs`, + // `ReadLengthPrefixedFields`, before this cut) - a length prefix of `int.MaxValue` + // (2147483647) overflowed C#'s `int` guard. Rust's `usize` is wider so this magnitude was + // never Rust's own hole, but "each runtime holding exactly the hole the other closed" means + // neither side's tests are evidence about the other - fed here too. + #[test] + fn hostile_length_prefix_near_i32_max_the_csharp_overflow_refuses_typed_here_too() { + let cursor = cursor_with_hostile_last_field("2147483647:x"); + let result = std::panic::catch_unwind(|| Cursor::parse(&cursor)); + match result { + Ok(Err(SelectionRefusal::CursorInvalid { .. })) => {} + other => panic!("expected a typed CursorInvalid refusal, not a panic or acceptance: {other:?}"), + } + } + + // R-AG: a length prefix with more digits than any integer type holds - `.parse::()` + // itself fails, a different failure path than the checked-add overflow above, and must also + // refuse typed rather than panic. + #[test] + fn hostile_length_prefix_wider_than_any_integer_type_refuses_typed() { + let cursor = cursor_with_hostile_last_field("999999999999999999999999999999:x"); + let result = std::panic::catch_unwind(|| Cursor::parse(&cursor)); + match result { + Ok(Err(SelectionRefusal::CursorInvalid { .. })) => {} + other => panic!("expected a typed CursorInvalid refusal, not a panic or acceptance: {other:?}"), + } + } + + // R-AG: a negative-looking length prefix - `usize::parse` refuses the leading `-` outright, + // a third failure path (neither overflow nor digit-count) that must also refuse typed. + #[test] + fn hostile_negative_length_prefix_refuses_typed() { + let cursor = cursor_with_hostile_last_field("-1:x"); + let result = std::panic::catch_unwind(|| Cursor::parse(&cursor)); + match result { + Ok(Err(SelectionRefusal::CursorInvalid { .. })) => {} + other => panic!("expected a typed CursorInvalid refusal, not a panic or acceptance: {other:?}"), + } + } } diff --git a/packages/cultnet-rs/src/snapshot_query.rs b/packages/cultnet-rs/src/snapshot_query.rs index a39532b1..92e9c12a 100644 --- a/packages/cultnet-rs/src/snapshot_query.rs +++ b/packages/cultnet-rs/src/snapshot_query.rs @@ -1,7 +1,7 @@ use anyhow::{Result, anyhow}; use std::collections::{BTreeMap, BTreeSet}; -use crate::{CultNetDocumentRegistry, CultNetMessage, CultNetRawDocumentRecord, RecordRef, Row, Selection}; +use crate::{CultNetDocumentRegistry, CultNetMessage, CultNetRawDocumentRecord, Row, Selection}; /// Read-only backing surface for a CultNet snapshot server. /// @@ -175,7 +175,7 @@ impl Row for RawSnapshotRow<'_> { fn number(&self, _index: &str) -> Option { unreachable!("v0's lowered selection carries no fields; number is not read") } - fn references(&self) -> Vec<(String, RecordRef, Option>)> { + fn references(&self) -> Vec<(String, String, Option>)> { unreachable!("v0's lowered selection carries no cites/cited; references is not read") } }