diff --git a/Cargo.lock b/Cargo.lock index d4a65cf..bb0260e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -787,7 +787,7 @@ dependencies = [ [[package]] name = "chia-protocol" version = "0.26.0" -source = "git+https://github.com/DIG-Network/dig-gossip?rev=3e352d91fbda830e517c914cf4f6afa6a9a7e121#3e352d91fbda830e517c914cf4f6afa6a9a7e121" +source = "git+https://github.com/DIG-Network/dig-gossip?rev=dd29ce8df275d60b3568079f3739bbe55f7bbf17#dd29ce8df275d60b3568079f3739bbe55f7bbf17" dependencies = [ "chia-bls 0.26.0", "chia-serde", @@ -880,7 +880,7 @@ dependencies = [ [[package]] name = "chia-sdk-client" version = "0.28.0" -source = "git+https://github.com/DIG-Network/dig-gossip?rev=3e352d91fbda830e517c914cf4f6afa6a9a7e121#3e352d91fbda830e517c914cf4f6afa6a9a7e121" +source = "git+https://github.com/DIG-Network/dig-gossip?rev=dd29ce8df275d60b3568079f3739bbe55f7bbf17#dd29ce8df275d60b3568079f3739bbe55f7bbf17" dependencies = [ "aws-lc-rs", "chia-protocol", @@ -2031,8 +2031,8 @@ dependencies = [ [[package]] name = "dig-gossip" -version = "0.17.12" -source = "git+https://github.com/DIG-Network/dig-gossip?rev=3e352d91fbda830e517c914cf4f6afa6a9a7e121#3e352d91fbda830e517c914cf4f6afa6a9a7e121" +version = "0.17.21" +source = "git+https://github.com/DIG-Network/dig-gossip?rev=dd29ce8df275d60b3568079f3739bbe55f7bbf17#dd29ce8df275d60b3568079f3739bbe55f7bbf17" dependencies = [ "bincode", "chia-protocol", @@ -2211,7 +2211,7 @@ dependencies = [ [[package]] name = "dig-node-core" -version = "0.25.0" +version = "0.25.1" dependencies = [ "async-trait", "axum", @@ -2266,7 +2266,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.68.0" +version = "0.68.1" dependencies = [ "async-trait", "axum", @@ -2774,7 +2774,7 @@ dependencies = [ "libc", "option-ext", "redox_users 0.5.2", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2922,7 +2922,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -3697,7 +3697,7 @@ checksum = "3640c1c38b8e4e43584d8df18be5fc6b0aa314ce6ebf51b53313d4306cca8e46" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -4025,7 +4025,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -4621,7 +4621,7 @@ dependencies = [ "once_cell", "socket2 0.6.5", "tracing", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -4990,7 +4990,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.12.1", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -5639,7 +5639,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix 1.1.4", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -6697,7 +6697,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.48.0", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index b74e565..daed0a5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,7 +31,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.68.0" +version = "0.68.1" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over @@ -54,8 +54,8 @@ overflow-checks = true # standalone. `native-tls` is NOT patched: dig-gossip is pulled with # `default-features = false, features = ["rustls", "relay"]`, so the OpenSSL/native-tls path is off. [patch.crates-io] -chia-protocol = { git = "https://github.com/DIG-Network/dig-gossip", rev = "3e352d91fbda830e517c914cf4f6afa6a9a7e121" } -chia-sdk-client = { git = "https://github.com/DIG-Network/dig-gossip", rev = "3e352d91fbda830e517c914cf4f6afa6a9a7e121" } +chia-protocol = { git = "https://github.com/DIG-Network/dig-gossip", rev = "dd29ce8df275d60b3568079f3739bbe55f7bbf17" } +chia-sdk-client = { git = "https://github.com/DIG-Network/dig-gossip", rev = "dd29ce8df275d60b3568079f3739bbe55f7bbf17" } # The dig-nat unification patch is RETIRED (#1280 crates.io cascade): dig-nat 0.7 is on crates.io and # the ENTIRE peer stack (dig-gossip 0.7.1, dig-dht 0.2.2, dig-download 0.2.1, dig-peer-selector 0.2.1, diff --git a/SPEC.md b/SPEC.md index a016ee3..02e67c5 100644 --- a/SPEC.md +++ b/SPEC.md @@ -3247,6 +3247,14 @@ connects to, with a restart re-entering the same state. Re-stating the whole inv single chokepoint. dig-dht is crypto-free by design, and `ingest_verified_provider` is the sole sanctioned bypass of its mTLS self-announce check, so these are the whole of the authentication: +0. **DECODE refuses a frame whose declared counts exceed the protocol maxima, BEFORE reserving for them** + (#1723). The wire states its batch size and each `Add`'s address count as `u16`, so both are the + sender's to choose; decode MUST check them against `HOLDINGS_MAX_CHANGES` (256) and + `MAX_ADDRS_PER_CHANGE` (32) before any allocation is sized from them. This gate is stated separately + from gate 1 because it necessarily runs BEFORE it: decode precedes the signature check, so an + allocation sized here is one an UNAUTHENTICATED peer commissioned. A ~200-byte frame declaring + 65,535 addresses would otherwise reserve ~2 MiB, and no later gate can refund it. The general rule + this instantiates: **never size an allocation from a number a peer supplied.** 1. `verify_holdings_announce` passes (batch cap, `SHA-256(provider_spki) == provider_peer_id`, P-256 SPKI, valid signature over the `dig:holdings:v1` domain-separated message). 2. `provider_peer_id` is CANONICALIZED (decoded to 32 bytes, re-encoded lowercase) and every subsequent diff --git a/crates/dig-node-core/Cargo.toml b/crates/dig-node-core/Cargo.toml index 2ab52e4..ac5259f 100644 --- a/crates/dig-node-core/Cargo.toml +++ b/crates/dig-node-core/Cargo.toml @@ -6,7 +6,7 @@ name = "dig-node-core" # compatibility-relevant change even though the crate is consumed in-workspace and is not published to # crates.io. Holding it at 0.23.0 would have overstated stability to anyone reading the manifest for # what the exported types are (dig_ecosystem#1785c). -version = "0.25.0" +version = "0.25.1" edition = "2021" license = "GPL-2.0-only" description = "The canonical DIG node ENGINE library (crate `dig_node_core`): the JSON-RPC dispatch (`handle_rpc`, the same contract as rpc.dig.net), local-first content serve/fetch/redirect from LOCAL .dig store modules (via digstore_host::serve_blind), chain-anchored-root resolution, chain-watch + subscriptions + generation gap-fill, the LRU cache, and the full P2P stack. Shared UNCHANGED by both host shells: the `dig-node` OS-service binary (dig-node-service) and the DIG Browser's in-process cdylib (dig-runtime). Native Rust so the compiled-module serve path works." @@ -177,7 +177,7 @@ dig-nat = "0.14" # graph still unifies to ONE of each. The vendored chia-protocol/chia-sdk-client forks it path-patches # are re-declared at this SAME rev in the workspace `[patch.crates-io]` (a git dep's own patches do not # apply transitively — the #1529 3-rev lockstep: this dep rev + the two patch revs move together). -dig-gossip = { git = "https://github.com/DIG-Network/dig-gossip", rev = "3e352d91fbda830e517c914cf4f6afa6a9a7e121", default-features = false, features = ["rustls", "relay"] } +dig-gossip = { git = "https://github.com/DIG-Network/dig-gossip", rev = "dd29ce8df275d60b3568079f3739bbe55f7bbf17", default-features = false, features = ["rustls", "relay"] } # The canonical DIG network id (genesis challenge) for the gossip handshake / introducer scope. From # crates.io: dig-nat 0.7 requires `>=0.4, <0.6` and dig-gossip 0.7.1 requires `>=0.2, <0.5`, so `0.4` # sits in both windows and the whole graph resolves to ONE crates.io dig-constants 0.4.x. diff --git a/crates/dig-node-core/src/download.rs b/crates/dig-node-core/src/download.rs index 539e14b..e1235da 100644 --- a/crates/dig-node-core/src/download.rs +++ b/crates/dig-node-core/src/download.rs @@ -338,11 +338,22 @@ pub(crate) fn pool_event_to_selector(peer_id: [u8; 32], event: PoolEventKind) -> } } -/// The 1:1 field map of `dig_gossip::PoolRemovalReason` → the selector's local [`PoolRemovalReason`] -/// (identical variants; `Banned` makes the peer ineligible until re-added, SPEC §9.4). +/// Maps `dig_gossip::PoolRemovalReason` → the selector's local [`PoolRemovalReason`] +/// (`Banned` makes the peer ineligible until re-added, SPEC §9.4). +/// +/// `Reaped` has no selector counterpart, so it must fold into one of the three the selector knows. +/// It folds to `Disconnected` rather than `Dead` because a reaped peer's transport was *provably +/// closed* — a departure, which is what `Disconnected` names — whereas `Dead` names a keepalive +/// finding a peer unresponsive. What makes the choice safe rather than merely tidy: the selector +/// distinguishes only `Banned` behaviourally (`engine.rs` matches on it alone to mark a peer +/// ineligible) and treats `Disconnected`/`Dead` identically, so this fold is observability-only and +/// cannot change eligibility. Folding it to `Banned` would be the real error — it would make an +/// honestly-departed peer ineligible and bias the node toward unremembered peers, which is a sybil. pub(crate) fn pool_removal_reason(reason: GossipRemovalReason) -> PoolRemovalReason { match reason { - GossipRemovalReason::Disconnected => PoolRemovalReason::Disconnected, + GossipRemovalReason::Disconnected | GossipRemovalReason::Reaped => { + PoolRemovalReason::Disconnected + } GossipRemovalReason::Dead => PoolRemovalReason::Dead, GossipRemovalReason::Banned => PoolRemovalReason::Banned, } @@ -415,6 +426,9 @@ pub(crate) enum GossipRemovalReason { Dead, /// Banned for misbehaviour. Banned, + /// Swept up by dig-gossip's departed-peer reaper: the transport was provably closed, but the + /// slot is keepalive-less so nothing else observed the departure. + Reaped, } // -- Selector-driven DIAL ordering (#384) ------------------------------------------------------------ @@ -2649,10 +2663,12 @@ pub(crate) mod tests { // -- the peer selector (#178): the discovery → select → download → record_outcome loop ---------- - /// The gossip → selector `PoolEvent` map is a byte-identical 1:1 (SPEC §5.4): the peer id is the - /// same 32 bytes and the removal reasons map variant-for-variant. + /// The gossip → selector `PoolEvent` map preserves identity and removal semantics (SPEC §5.4): + /// the peer id is the same 32 bytes, the three reasons the selector shares map + /// variant-for-variant, and `Reaped` — which the selector has no variant for — folds to the + /// non-punitive `Disconnected`. #[test] - fn pool_event_map_is_1_to_1() { + fn pool_event_map_preserves_identity_and_removal_semantics() { let addr: std::net::SocketAddr = "203.0.113.7:9444".parse().unwrap(); let added = pool_event_to_selector([9u8; 32], PoolEventKind::Added { addr }); assert_eq!( @@ -2669,6 +2685,10 @@ pub(crate) mod tests { ), (GossipRemovalReason::Dead, PoolRemovalReason::Dead), (GossipRemovalReason::Banned, PoolRemovalReason::Banned), + // `Reaped` has no selector counterpart and folds to `Disconnected`. Pinned here so the + // fold is a decision the suite states, not an accident a later edit can quietly change + // into `Banned` — which would make an honestly-departed peer ineligible. + (GossipRemovalReason::Reaped, PoolRemovalReason::Disconnected), ] { assert_eq!(pool_removal_reason(g), s); assert_eq!( diff --git a/crates/dig-node-core/src/peer.rs b/crates/dig-node-core/src/peer.rs index 8b5f387..db24edc 100644 --- a/crates/dig-node-core/src/peer.rs +++ b/crates/dig-node-core/src/peer.rs @@ -1929,6 +1929,9 @@ pub(crate) fn map_gossip_pool_event(ev: &dig_gossip::PoolEvent) -> dig_peer_sele dig_gossip::PoolRemovalReason::Banned => { crate::download::GossipRemovalReason::Banned } + dig_gossip::PoolRemovalReason::Reaped => { + crate::download::GossipRemovalReason::Reaped + } }; crate::download::pool_event_to_selector( bytes, diff --git a/crates/dig-node-core/tests/holdings_decode_alloc.rs b/crates/dig-node-core/tests/holdings_decode_alloc.rs new file mode 100644 index 0000000..e4b153a --- /dev/null +++ b/crates/dig-node-core/tests/holdings_decode_alloc.rs @@ -0,0 +1,177 @@ +//! The opcode-222 decoder must never size an allocation from a peer-declared count (#1723). +//! +//! A `holdings-announce` frame declares its per-change address count as a `u16`. Reserving on that +//! declaration lets a ~200-byte frame from an **unauthenticated** peer commit ~2 MiB, because decode +//! runs *before* `verify_holdings_announce` checks the signature. The bound is not the problem — +//! `u16` bounds it — the problem is that the number is the peer's to choose and nothing verifies it +//! against the protocol's own per-change maximum until much later. +//! +//! # Why this test measures ALLOCATION rather than the return value +//! +//! The fix is a **placement**, not an outcome: a decoder that reserves first and then fails on the +//! truncated address bytes returns exactly the same `None` as one that rejects the count up front. +//! An `assert!(decoded.is_none())` therefore passes identically against the defect and against the +//! fix — it would pin a coincidence, and a later refactor moving the guard back below the reservation +//! would keep it green. The only observable that distinguishes the two placements is the reservation +//! itself, so this test instruments the allocator and asserts the peak single request made *during* +//! the decode call. +//! +//! The frame is deliberately truncated after the declared count. That is what makes the test +//! discriminating: it means the *only* reason a large allocation could appear is the declaration +//! being trusted, since no address bytes exist to justify one. + +use std::alloc::{GlobalAlloc, Layout, System}; +use std::cell::Cell; + +use chia_protocol::{Bytes, Message, ProtocolMessageTypes}; +use dig_gossip::holdings_announce_payload; + +// ============================================================================ +// Allocation instrumentation +// ============================================================================ + +thread_local! { + /// Whether allocations on THIS thread are currently being measured. Thread-local so the + /// measurement is unaffected by tests running concurrently in the same binary. + static ARMED: Cell = const { Cell::new(false) }; + /// The largest single allocation request seen on this thread while armed. + static PEAK_REQUEST: Cell = const { Cell::new(0) }; +} + +/// Records `size` as a candidate peak. Uses `try_with` and allocation-free `Cell`s because a global +/// allocator hook that itself allocates — or that panics during thread-local teardown — deadlocks. +fn record(size: usize) { + let _ = ARMED.try_with(|armed| { + if armed.get() { + let _ = PEAK_REQUEST.try_with(|peak| { + if size > peak.get() { + peak.set(size); + } + }); + } + }); +} + +/// A pass-through allocator that notes the size of every request while [`ARMED`]. +struct PeakRecordingAllocator; + +// SAFETY: every method delegates to `System` with the caller's original arguments and contracts; +// the only added behaviour is recording a size into allocation-free thread-local `Cell`s. +unsafe impl GlobalAlloc for PeakRecordingAllocator { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + record(layout.size()); + unsafe { System.alloc(layout) } + } + + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + unsafe { System.dealloc(ptr, layout) } + } + + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + record(new_size); + unsafe { System.realloc(ptr, layout, new_size) } + } + + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + record(layout.size()); + unsafe { System.alloc_zeroed(layout) } + } +} + +#[global_allocator] +static ALLOCATOR: PeakRecordingAllocator = PeakRecordingAllocator; + +/// Runs `body` with allocation measurement armed, returning its value and the peak request seen. +fn measuring_allocations(body: impl FnOnce() -> T) -> (T, usize) { + PEAK_REQUEST.with(|peak| peak.set(0)); + ARMED.with(|armed| armed.set(true)); + let value = body(); + ARMED.with(|armed| armed.set(false)); + (value, PEAK_REQUEST.with(Cell::get)) +} + +// ============================================================================ +// The crafted frame +// ============================================================================ + +/// `HoldingsDelta::Add`'s wire kind tag. +const KIND_ADD: u8 = 0x01; + +/// The largest count the wire's `u16` can express — the whole point of the ticket is that a peer +/// may choose it freely. +const DECLARED_ADDRESS_COUNT: u16 = u16::MAX; + +/// The ceiling the decode path must stay under for this frame. +/// +/// Sized from the protocol, not from taste: the per-change maximum is 32 addresses, so an HONEST +/// worst-case reservation here is 32 × `size_of::()` — well under a kibibyte. 64 KiB +/// leaves three orders of magnitude of headroom for the small `String`/`Vec` allocations the header +/// legitimately makes, while sitting ~32× below the ~2 MiB a trusted `u16` would commit. Any +/// allocation between those two figures is not something this decode path has an honest reason to +/// make. +const ALLOCATION_CEILING: usize = 64 * 1024; + +/// Appends a `u16`-length-prefixed byte string, matching the wire's `put_bytes`. +fn put_bytes(buf: &mut Vec, bytes: &[u8]) { + buf.extend_from_slice(&(bytes.len() as u16).to_be_bytes()); + buf.extend_from_slice(bytes); +} + +/// A well-formed opcode-222 header followed by one `Add` delta that declares +/// [`DECLARED_ADDRESS_COUNT`] addresses and then **stops**. +/// +/// Every field up to the count is valid, so the decoder reaches the count by the ordinary path +/// rather than bailing early on a malformed header — the frame must get far enough to be able to +/// make the allocation under test. +fn frame_declaring_an_impossible_address_count() -> Vec { + let mut buf = Vec::new(); + put_bytes(&mut buf, &[b'a'; 64]); // provider_peer_id (hex-shaped, never parsed at decode) + put_bytes(&mut buf, &[0x30; 91]); // provider_spki (opaque to decode) + buf.extend_from_slice(&7u64.to_be_bytes()); // seq + buf.extend_from_slice(&1_800_000_000u64.to_be_bytes()); // announced_at + buf.extend_from_slice(&1u16.to_be_bytes()); // change_count — one delta, within MAX_CHANGES + buf.push(KIND_ADD); + buf.extend_from_slice(&[0xAB; 32]); // content_key + buf.extend_from_slice(&DECLARED_ADDRESS_COUNT.to_be_bytes()); + // Deliberately truncated: not one address byte follows the declaration. + buf +} + +/// Wraps `payload` in the opcode-222 message the inbound peer path actually receives. +fn holdings_message(payload: Vec) -> Message { + Message { + msg_type: ProtocolMessageTypes::HoldingsAnnounce, + id: None, + data: Bytes::new(payload), + } +} + +// ============================================================================ +// The regression +// ============================================================================ + +/// Decoding a frame that DECLARES 65,535 addresses and carries none must not reserve for 65,535. +/// +/// Asserted at `holdings_announce_payload` — the function `HoldingsIngress`'s inbound handler calls +/// on every opcode-222 frame — so this exercises the real entry point rather than a helper. +#[test] +fn decoding_a_declared_address_count_reserves_nothing_proportional_to_the_declaration() { + let message = holdings_message(frame_declaring_an_impossible_address_count()); + + // A first, unmeasured pass so that any one-time lazily-initialised state inside the decode path + // is not charged to the measurement below. + let _ = holdings_announce_payload(&message); + + let (decoded, peak_request) = measuring_allocations(|| holdings_announce_payload(&message)); + + assert!( + decoded.is_none(), + "a frame truncated after its address-count declaration must not decode" + ); + assert!( + peak_request <= ALLOCATION_CEILING, + "decoding a frame that declares {DECLARED_ADDRESS_COUNT} addresses and carries none \ + requested {peak_request} bytes in a single allocation (ceiling {ALLOCATION_CEILING}); \ + the declared count is being trusted to size a reservation before the signature is checked" + ); +}