diff --git a/src/host/observer/adapter.rs b/src/host/observer/adapter.rs index 501e5b6..85a41c7 100644 --- a/src/host/observer/adapter.rs +++ b/src/host/observer/adapter.rs @@ -779,6 +779,118 @@ mod tests { } } + /// The same property, asserted through the **production wiring** rather + /// than the helper. + /// + /// ⚠️ **This is the gate; the one above is a unit test of a private + /// function** (round 10 review, finding 4). Mutating + /// [`node_observation_from_props`] back to `truthy(props.get(…))` left the + /// helper test green, because it calls [`peerspeak_owned`] directly and + /// the only live case it shares with production — exact `"1"` — passes + /// under both implementations. That is precisely the "a gate satisfiable + /// by two sources gates neither" failure that bit the `main.rs` wiring + /// guard and phase 3r row 1. + /// + /// So: build a real `pw_properties` dictionary, push it through the same + /// function the registry callback calls, and assert the resulting + /// [`NodeProps::peerspeak_owned`] for every spelling. + #[test] + fn the_production_wiring_reads_the_ownership_carrier_exactly() { + pw::init(); + + // (property value, must be read as peerspeak-owned) + let spellings = [ + (Some(PEERSPEAK_OWNED_VALUE), true), + (None, false), + (Some(""), false), + (Some("false"), false), + (Some("0"), false), + (Some("false "), false), + (Some("true"), false), + (Some("yes"), false), + (Some("1 "), false), + (Some(" 1"), false), + (Some("01"), false), + (Some("2"), false), + ]; + + for (value, expected) in spellings { + let mut props = pw::properties::PropertiesBox::new(); + // A realistic node, so the rest of the parse runs too: this is the + // shape peerspeak's own tagged playback arrives in. + props.insert("media.class", "Stream/Output/Audio"); + props.insert("node.name", "probe"); + props.insert("client.id", "42"); + if let Some(value) = value { + props.insert(PEERSPEAK_OWNED_PROP, value); + } + + let observation = node_observation_from_props(props.dict()); + assert_eq!( + observation.props.peerspeak_owned, expected, + "{PEERSPEAK_OWNED_PROP}={value:?} through the real adapter" + ); + // The surrounding parse must still work, or a green result above + // could just mean the whole dictionary was dropped. + assert_eq!(observation.role, MediaRole::StreamOutput); + assert_eq!(observation.name.as_deref(), Some("probe")); + assert_eq!(observation.props.client_id, Some(GlobalId(42))); + } + } + + /// The cross-repo fixture's `prop_value` is the only spelling this + /// consumer treats as owned — asserted through the production wiring. + /// + /// The taint module's `ownership_carriers_match_the_cross_repo_fixture` + /// proves the two repos agree on the *literal*. That is not the same as + /// proving the shipping observer reads it, which is the half the round-10 + /// review's finding 6 was about: a future producer following the fixture + /// needs the file to describe what the code does, and only a test that + /// runs the code can keep those two honest. + #[test] + fn the_fixture_value_is_the_only_owned_spelling() { + const FIXTURE: &str = include_str!("../../../tests/fixtures/ownership-tag-contract.txt"); + pw::init(); + + let pinned = FIXTURE + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .map(|line| line.split_once('=').expect("fixture line is key=value")); + let mut prop_key = None; + let mut prop_value = None; + for (key, value) in pinned { + match key { + "prop_key" => prop_key = Some(value), + "prop_value" => prop_value = Some(value), + _ => {} + } + } + let prop_key = prop_key.expect("fixture defines prop_key"); + let prop_value = prop_value.expect("fixture defines prop_value"); + + let observe = |value: &str| { + let mut props = pw::properties::PropertiesBox::new(); + props.insert("media.class", "Stream/Output/Audio"); + props.insert(prop_key, value); + node_observation_from_props(props.dict()) + .props + .peerspeak_owned + }; + + assert!( + observe(prop_value), + "the fixture's own {prop_key}={prop_value} must read as owned" + ); + // The spellings the fixture explicitly says are NOT owned. + for denied in ["true", "yes", ""] { + assert!( + !observe(denied), + "{prop_key}={denied:?} must not read as owned; the fixture says so" + ); + } + } + /// The other three boolean properties keep the lenient spelling, and that /// is deliberate rather than an oversight: each is PipeWire's own, each /// varies by producer, and each causes *exclusion* when true — so reading diff --git a/src/host/taint/owner.rs b/src/host/taint/owner.rs index 579d27f..4727dd5 100644 --- a/src/host/taint/owner.rs +++ b/src/host/taint/owner.rs @@ -183,26 +183,48 @@ fn keys_of(node: &NodeSnapshot, ctx: &OwnerCtx) -> Vec<(OwnerKey, KeyValue)> { if let Some(client) = node.props.client_id { out.push((OwnerKey::ClientId, KeyValue::Num(u64::from(client.0)))); } - // Key 4, from the node if it has one, otherwise from its Client - // (round 10, R10-3). The node's own property wins when both exist: it is - // the direct statement, and the Client's is a one-hop inference. + // Key 4, from the node **and** from its Client (round 10, R10-3; made a + // union rather than a fallback by the round-10 review, finding 1). // - // ⚠️ **Exception 1 must apply to the fallback too, and this is the whole - // risk of the fallback.** Measured on this host: 15 unrelated Clients - // share `sec_pid` 2528, which is pipewire-pulse's own — every - // Pulse-emulated app has one. Adding key 4 unguarded would fuse all - // fifteen into a single owner and bridge taint between completely - // unrelated applications, which is enormous over-exclusion. Guarded, the - // fallback strictly *adds* correct bounding: it fires only for native - // clients, which are exactly the ones that carry a real per-app pid here. - if let Some(pid) = node.props.process_id.or_else(|| ctx.client_pid(node)) { - // Note the fail-closed asymmetry when the daemon PID is unknown - // (`None`): the exception does *not* fire, key 4 applies to - // everything, and Pulse modules fuse into one owner. That is broad - // over-exclusion — annoying and safe — which is the direction v3.4 - // §6.1.2's failure-mode paragraph asks for. - if Some(pid) != ctx.pipewire_pulse_pid { - out.push((OwnerKey::ProcessId, KeyValue::Num(u64::from(pid)))); + // ⚠️ **A union, not `node.or_else(client)`, and the difference is a leak.** + // The node's `application.process.id` is client-controlled and optional; + // the Client's `pipewire.sec.pid` is `pipewire.*`, protected, and the only + // one that can carry a soundness argument (the same reason + // `propagate_unresolved_owner` sweeps everything for an unbounded reader). + // Letting the node's value *replace* the Client's meant one process using + // two Clients could escape the bridge entirely: its tainted reader reports + // a bogus node pid, its output leg omits the node pid and falls back to + // the Client's real one, the two legs are bounded by different values, so + // they neither bridge nor trip the unbounded sweep — and the output stays + // eligible while re-emitting the call. Carrying both values costs nothing + // and closes it: a leg that presents *either* value bridges. + // + // ⚠️ **Exception 1 applies to each value independently, and that is the + // whole risk here.** Measured on this host: 15 unrelated Clients share + // `sec_pid` 2528, which is pipewire-pulse's own — every Pulse-emulated app + // has one. Suppressing it per value is what keeps the union from fusing + // all fifteen into a single owner while still keeping each app's real + // per-app pid. For the common Pulse shape (node pid = the app's, Client + // `sec_pid` = the daemon's) the union therefore reduces to exactly the + // node's pid, as before. + // + // Note the fail-closed asymmetry when the daemon PID is unknown (`None`): + // the exception does *not* fire, key 4 applies to everything, and Pulse + // modules fuse into one owner. That is broad over-exclusion — annoying and + // safe — which is the direction v3.4 §6.1.2's failure-mode paragraph asks + // for. + for pid in [node.props.process_id, ctx.client_pid(node)] + .into_iter() + .flatten() + { + if Some(pid) == ctx.pipewire_pulse_pid { + continue; + } + let key = (OwnerKey::ProcessId, KeyValue::Num(u64::from(pid))); + // The two agree far more often than not; a duplicate entry would be + // harmless but would make the audit's key list read oddly. + if !out.contains(&key) { + out.push(key); } } out diff --git a/src/host/taint/snapshot.rs b/src/host/taint/snapshot.rs index 1f669f7..61d75ad 100644 --- a/src/host/taint/snapshot.rs +++ b/src/host/taint/snapshot.rs @@ -111,8 +111,11 @@ impl MediaRole { /// on this feature means "not tainted". #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct NodeProps { - /// `peerspeak.owned` is present and truthy (v3.4 §5.1). A correctness - /// mechanism, explicitly *not* a security boundary. + /// `peerspeak.owned` is present and **exactly** + /// [`super::PEERSPEAK_OWNED_VALUE`] (v3.4 §5.1, tightened by round 10's + /// R10-4 — it is not "present and truthy", and the round-10 review found + /// this doc still saying so). A correctness mechanism, explicitly *not* a + /// security boundary. /// /// ⚠️ **Ownership carrier 1 of 2, so this being `false` does not mean /// "not peerspeak's".** Carrier 2 is the [`NodeSnapshot::name`] prefix diff --git a/src/host/taint/tests.rs b/src/host/taint/tests.rs index ef883de..ac0a958 100644 --- a/src/host/taint/tests.rs +++ b/src/host/taint/tests.rs @@ -362,17 +362,18 @@ fn ownership_carriers_match_the_cross_repo_fixture() { assert_eq!(super::PEERSPEAK_OWNED_PROP, get("prop_key")); assert_eq!(super::PEERSPEAK_OWNED_NODE_PREFIX, get("node_name_prefix")); + // ⚠️ **Equality, and that is now the whole rule**: carrier 1 is matched + // exactly, not as "anything but false/0" (round 10, R10-4). This assert + // used to be followed by a weaker `value != "false" && value != "0"` + // check, which described a leniency that no longer exists — the round-10 + // review's finding 6, and a real trap: a future producer reading the old + // fixture prose could emit "true" and silently lose this carrier. + // + // That this consumer actually *listens* to the fixture's value, through + // the production observer wiring rather than a helper, is asserted by + // `observer::adapter::tests::the_fixture_value_is_the_only_owned_spelling`. assert_eq!(super::PEERSPEAK_OWNED_VALUE, get("prop_value")); - // …and that value must be one this consumer reads as truthy. `truthy` - // lives at the observer boundary; assert the property of it that matters - // here rather than reaching across modules for the function. - let value = get("prop_value"); - assert!( - value != "false" && value != "0", - "pinned prop value {value:?} would read as untruthy" - ); - // And the fixture's own worked example must be one this engine excludes, // through carrier 2, exactly as written in the shared file. let mut graph = Graph::new(); @@ -991,10 +992,81 @@ fn a_pidless_first_client_still_makes_its_id_ambiguous() { ); } -/// The node's own `application.process.id` wins when both are available. It is -/// a direct statement about the node; the Client's is a one-hop inference, and -/// they can legitimately differ (a Pulse-emulated node's pid is the app's while -/// its Client's `sec_pid` is the daemon's — the single most common shape here). +/// A process using **two** Clients cannot escape the bridge by presenting a +/// bogus pid on one leg and none on the other. +/// +/// ⚠️ **This is the round-10 review's finding 1, and it was a real leak while +/// key 4 was `node.or_else(client)`.** The node's `application.process.id` is +/// client-controlled; the Client's `pipewire.sec.pid` is protected. Letting +/// the node's value *replace* the Client's meant the reader was bounded by +/// `12_345` and the output leg by `50_000`, so they shared no key, did not +/// bridge, and — both being bounded — neither tripped the unbounded sweep. +/// The output stayed eligible while re-emitting the call. +/// +/// Carrying both values fixes it: the two legs share the Client pid. +/// +/// Reachability, stated honestly: `evaluate()` today is reached only by the +/// dry-run audit, which creates no links, so this could not echo on this +/// branch. It becomes live the moment phase 6 consumes these decisions. +#[test] +fn one_process_with_two_clients_cannot_split_its_pid_to_escape_the_bridge() { + let mut graph = Graph::new(); + let hw = graph.device_node("hw-sink", MediaRole::Sink); + let call = graph.peerspeak_node("peerspeak", 7); + graph.link(call, hw); + + // One native process, two Clients, one protected pid. + let reader_client = graph.client(Some(50_000)); + let output_client = graph.client(Some(50_000)); + + // Its reading leg claims a pid that is not its own. + let reader = graph.node( + "two-client-reader", + MediaRole::StreamInput, + NodeProps { + client_id: Some(reader_client), + process_id: Some(12_345), + ..NodeProps::default() + }, + ); + graph.link(hw, reader); + + // Its re-emitting leg claims no pid at all. + let output = graph.node( + "two-client-output", + MediaRole::StreamOutput, + NodeProps { + client_id: Some(output_client), + process_id: None, + ..NodeProps::default() + }, + ); + graph.link(output, hw); + + // A genuinely unrelated app must survive, or "exclude everything" would + // pass this test — the §5.1 eligible-half rule. + let bystander = graph.app_node("mpv", MediaRole::StreamOutput, 9_001); + graph.link(bystander, hw); + + assert_partition( + &run(&graph, &ctx()), + &[("mpv", bystander)], + &[ + ("call", call, "peerspeak-owned"), + ("two-client-output", output, "tainted-owner-bridge"), + ], + ); +} + +/// The node's own `application.process.id` is used even when its Client's +/// `sec_pid` is the daemon's — the single most common shape here, since a +/// Pulse-emulated node's pid is the app's while its Client's is +/// pipewire-pulse's. +/// +/// ⚠️ Both values are now carried (round-10 review, finding 1), so this is no +/// longer "the node's wins" but "exception 1 is applied per value": the +/// daemon's `sec_pid` is dropped and the node's real pid is kept, leaving the +/// same single key as before. #[test] fn the_nodes_own_process_id_wins_over_its_clients() { let mut graph = Graph::new(); diff --git a/tests/fixtures/ownership-tag-contract.txt b/tests/fixtures/ownership-tag-contract.txt index 471c413..fa1b3ef 100644 --- a/tests/fixtures/ownership-tag-contract.txt +++ b/tests/fixtures/ownership-tag-contract.txt @@ -20,9 +20,17 @@ # PipeWire registry `global` event and readable only via a node bind, so the # primary taint root must not rest on one observation mechanism alone. -# Carrier 1 — a node property. The consumer treats any value other than -# "false"/"0" as truthy, which is the fail-closed direction; the producer -# always emits exactly this value. +# Carrier 1 — a node property, matched EXACTLY: `prop_value` below is the +# ONLY spelling the consumer reads as owned. A producer emitting "true", "yes" +# or "" is NOT owned on this carrier, and only carrier 2 would still catch it. +# +# ⚠️ This wording is load-bearing and it CHANGED in round 10. The consumer +# used to accept any value other than "false"/"0", on the theory that leniency +# over-excludes and is therefore safe. It is not: leniency buys false-positive +# exclusion, and it let any process suppress a rival application's audio from +# the share with a property it did not even have to spell right. Fail-closed +# on this feature is about ANCESTRY — an unresolvable graph is not eligible — +# not about parsing. prop_key=peerspeak.owned prop_value=1