From 1b01847c66cbbc0e3bda2c78339d884ca5cb5b1c Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 19:19:39 -0400 Subject: [PATCH 01/11] host/taint: match peerspeak's second ownership carrier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The consumer half of phase 1 (plan §5.1, impl plan §3). The engine's tag root becomes a union: `peerspeak.owned` truthy OR `node.name` starting with `peerspeak_owned_`. Round 8 added the second carrier because a node property is invisible to the registry `global` event and recoverable only by binding the node — which is exactly how the phase-5 gate failed — while `node.name` is announced directly. The union lives in `local_root_reason`, not in the adapter. Folding both into the one `peerspeak_owned` bool at the observation boundary would make each carrier untestable alone, which is the phase-3r lesson: a gate asserting a value two sources can satisfy gates neither. The existing `peerspeak_tagged_nodes_…` fixture now carries both carriers, so it would keep passing if either were deleted; two new tests pin them individually, and a third pins that the prefix matches only at the start of a name. Both literals are now named constants — they are a cross-repo wire contract with peerspeak, not local naming — and asserted against tests/fixtures/ownership-tag-contract.txt, committed byte-identical in both repos. That test also runs the fixture's own worked example name through the engine, so the shared file cannot document a value this side does not actually exclude. Five mutations verified: drop either carrier, loosen `starts_with` to `contains`, or rename either constant, and exactly the intended test fails. Co-Authored-By: Claude Opus 5 --- src/host/audit/tests.rs | 20 ++++- src/host/observer/adapter.rs | 7 +- src/host/taint/fixture.rs | 22 +++++ src/host/taint/mod.rs | 35 +++++++- src/host/taint/snapshot.rs | 6 ++ src/host/taint/tests.rs | 98 +++++++++++++++++++++++ tests/fixtures/ownership-tag-contract.txt | 34 ++++++++ 7 files changed, 216 insertions(+), 6 deletions(-) create mode 100644 tests/fixtures/ownership-tag-contract.txt diff --git a/src/host/audit/tests.rs b/src/host/audit/tests.rs index ce254df..473a6b3 100644 --- a/src/host/audit/tests.rs +++ b/src/host/audit/tests.rs @@ -22,9 +22,17 @@ use super::metrics::{BUCKET_LABELS, Metrics, QUEUE_THRESHOLD_US, Sample}; use super::*; use crate::host::aec::AecConfig; use crate::host::observer::{EventKind, Readiness}; +use crate::host::taint::PEERSPEAK_OWNED_NODE_PREFIX; use crate::host::taint::fixture::{self, Graph, NodeRef}; use crate::host::taint::snapshot::{GraphSnapshot, MediaRole}; +/// The `node.name` a [`Graph::peerspeak_node`] fixture produces. Built from +/// the same constant the engine matches on, so these audit rows report the +/// name shape a live peerspeak node actually has (v3.5 §5.1, carrier 2). +fn owned_name(role: &str, pid: u32) -> String { + format!("{PEERSPEAK_OWNED_NODE_PREFIX}{role}_{pid}") +} + const AEC_MODULE: u64 = 7; const TIMEOUT: Millis = 5_000; @@ -284,13 +292,14 @@ fn a_shut_gate_preserves_the_engines_own_reasons() { .body; let (_, excluded) = partition(&body); + let playback = owned_name("peerspeak-playback", 200); assert!(!body.fan_out_permitted); assert_eq!( excluded, vec![ ("music", "aec-validating"), // Tagged, so it keeps the reason that actually applies to it. - ("peerspeak-playback", "peerspeak-owned"), + (playback.as_str(), "peerspeak-owned"), ] ); } @@ -457,11 +466,12 @@ fn row_1_owner_bridge_forwarder_with_an_untainted_control() { .body; let (eligible, excluded) = partition(&body); + let call_name = owned_name("peerspeak-call", 200); assert_eq!(eligible, vec!["clean-loopback-playback"]); assert_eq!( excluded, vec![ - ("peerspeak-call", "peerspeak-owned"), + (call_name.as_str(), "peerspeak-owned"), ("tainted-loopback-playback", "tainted-owner-bridge"), ] ); @@ -494,11 +504,12 @@ fn row_3_one_tainted_module_does_not_taint_the_other() { .body; let (eligible, excluded) = partition(&body); + let call_name = owned_name("peerspeak-call", 200); assert_eq!(eligible, vec!["module-b-playback"]); assert_eq!( excluded, vec![ - ("peerspeak-call", "peerspeak-owned"), + (call_name.as_str(), "peerspeak-owned"), ("module-a-playback", "tainted-owner-bridge"), ] ); @@ -701,7 +712,8 @@ fn the_taint_view_covers_non_candidate_roles() { tainted.contains(&("null-sink", "tainted-upstream")), "the sink is not a candidate but its taint is what explains the row: {tainted:?}" ); - assert!(tainted.contains(&("peerspeak-call", "peerspeak-owned"))); + let call_name = owned_name("peerspeak-call", 200); + assert!(tainted.contains(&(call_name.as_str(), "peerspeak-owned"))); } /// A record must serialise to a single line. Newlines inside a JSON Lines diff --git a/src/host/observer/adapter.rs b/src/host/observer/adapter.rs index f0e31ee..ef6302e 100644 --- a/src/host/observer/adapter.rs +++ b/src/host/observer/adapter.rs @@ -9,6 +9,7 @@ use super::{ EventKind, LinkEndpoints, NodeObservation, Outcome, Projection, RegEvent, RegistryModel, }; use crate::host::audio::parse_object_serial; +use crate::host::taint::PEERSPEAK_OWNED_PROP; use crate::host::taint::snapshot::{ ClientSnapshot, GlobalId, MediaRole, NodeProps, PortDirection, PortSnapshot, Serial, }; @@ -657,7 +658,11 @@ fn node_observation_from_props(props: &pw::spa::utils::dict::DictRef) -> NodeObs name: props.get("node.name").map(str::to_string), role: MediaRole::parse(props.get("media.class")), props: NodeProps { - peerspeak_owned: truthy(props.get("peerspeak.owned")), + // Carrier 1 only. Carrier 2 (the `node.name` prefix) is matched + // in the engine off `NodeObservation::name` above, so each + // carrier stays independently testable — see + // [`crate::host::taint::PEERSPEAK_OWNED_NODE_PREFIX`]. + peerspeak_owned: truthy(props.get(PEERSPEAK_OWNED_PROP)), pulse_module_id: props .get("pulse.module.id") .and_then(|value| value.parse::().ok()), diff --git a/src/host/taint/fixture.rs b/src/host/taint/fixture.rs index ec3f2bd..4ae652f 100644 --- a/src/host/taint/fixture.rs +++ b/src/host/taint/fixture.rs @@ -152,11 +152,33 @@ impl Graph { self.node(name, role, app(client, pid)) } + /// A peerspeak-owned node carrying **both** ownership carriers, as a + /// live one does. `name` gets the real `node.name` prefix so the fixture + /// cannot pass on the property alone. pub fn peerspeak_node(&mut self, name: &str, pid: u32) -> NodeRef { + let client = self.client_of_app(pid); + let name = format!("{}{name}_{pid}", super::PEERSPEAK_OWNED_NODE_PREFIX); + self.node(&name, MediaRole::StreamOutput, peerspeak_owned(client, pid)) + } + + /// Carrier 1 alone: the `peerspeak.owned` property present, the + /// `node.name` prefix absent. What the engine sees for a node it had to + /// bind to observe (v3.5 §6.7). + pub fn peerspeak_node_prop_only(&mut self, name: &str, pid: u32) -> NodeRef { let client = self.client_of_app(pid); self.node(name, MediaRole::StreamOutput, peerspeak_owned(client, pid)) } + /// Carrier 2 alone: the `node.name` prefix present, the property absent + /// — indistinguishable from an ordinary app in every other respect. + /// This is the case that survives the F1 observation defect, and the + /// reason round 8 added a second carrier at all. + pub fn peerspeak_node_name_only(&mut self, role: &str, pid: u32) -> NodeRef { + let client = self.client_of_app(pid); + let name = format!("{}{role}_{pid}", super::PEERSPEAK_OWNED_NODE_PREFIX); + self.node(&name, MediaRole::StreamOutput, app(client, pid)) + } + pub fn node(&mut self, name: &str, role: MediaRole, props: NodeProps) -> NodeRef { let id = self.id(); self.node_with_id(name, role, id, props) diff --git a/src/host/taint/mod.rs b/src/host/taint/mod.rs index 9ee8b9c..4c4e9b4 100644 --- a/src/host/taint/mod.rs +++ b/src/host/taint/mod.rs @@ -123,6 +123,29 @@ pub const CAPTURE_SINK_PREFIX: &str = "pixelpass_capture_"; /// what `pulse.module.id` is for (v3.4 §5.2 correction 4). pub const ECHO_CANCEL_GROUP_PREFIX: &str = "echo-cancel-"; +/// Ownership carrier 1: the node property peerspeak sets on everything it +/// plays (v3.5 §5.1). Read at the observer boundary, which is the only place +/// that touches raw property names — see [`super::observer`]. +/// +/// ⚠️ **Cross-repo wire contract.** peerspeak emits this; it does not depend +/// on this crate, nor this crate on it. The values are pinned in +/// `tests/fixtures/ownership-tag-contract.txt`, committed byte-identical in +/// both repos, and asserted by [`tests::ownership_carriers_match_the_cross_repo_fixture`]. +/// The producer's matching constants live in peerspeak +/// `src/audio/ownership.rs`. Changing either is a both-repos-same-session +/// change that invalidates the phase 5 matrix. +pub const PEERSPEAK_OWNED_PROP: &str = "peerspeak.owned"; + +/// Ownership carrier 2: a `node.name` prefix (v3.5 §5.1, round 8). +/// +/// Matched as a **union** with [`PEERSPEAK_OWNED_PROP`] — either one makes a +/// node peerspeak-owned. Two carriers because a property is invisible to the +/// registry `global` event and recoverable only by binding the node (v3.5 +/// §6.7), which is precisely how the phase-5 gate failed; this one is +/// announced directly. A union is also the fail-closed direction: a missed +/// tag leaks call audio into the share, a spurious one only over-excludes. +pub const PEERSPEAK_OWNED_NODE_PREFIX: &str = "peerspeak_owned_"; + /// Why a node is tainted or excluded. Stable machine-readable codes: this /// value is the phase 5 audit output, the phase 6 status event, and the /// eventual answer to "why isn't this app being shared?". @@ -535,7 +558,17 @@ fn ambiguous_id_nodes(snapshot: &GraphSnapshot) -> BTreeSet { } fn local_root_reason(node: &NodeSnapshot, ctx: &ExclusionCtx) -> Option { - if node.props.peerspeak_owned { + // The two ownership carriers, as a union (v3.5 §5.1). Kept here rather + // than folded together at the observer boundary so that the union is a + // pure, directly-testable rule: an adapter that collapsed both into the + // one `peerspeak_owned` bool would make each carrier untestable alone, + // which is exactly how phase 3r's row 1 nearly gated nothing. + if node.props.peerspeak_owned + || node + .name + .as_deref() + .is_some_and(|name| name.starts_with(PEERSPEAK_OWNED_NODE_PREFIX)) + { return Some(Reason::PeerspeakOwned); } if let (Some(module), Some(aec)) = (node.props.pulse_module_id, ctx.aec_module_id) diff --git a/src/host/taint/snapshot.rs b/src/host/taint/snapshot.rs index 88c3417..29b02f5 100644 --- a/src/host/taint/snapshot.rs +++ b/src/host/taint/snapshot.rs @@ -99,6 +99,12 @@ impl MediaRole { pub struct NodeProps { /// `peerspeak.owned` is present and truthy (v3.4 §5.1). 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 + /// [`super::PEERSPEAK_OWNED_NODE_PREFIX`], matched as a union in + /// `local_root_reason`. Read that function, not this field, to answer + /// "is this node owned?". pub peerspeak_owned: bool, /// `pulse.module.id`, parsed as `u64` — never `u32`, per v3.4 §5.2's /// parse-defensively note and the phase 0a truncation bug. diff --git a/src/host/taint/tests.rs b/src/host/taint/tests.rs index d6bde50..0c4faa2 100644 --- a/src/host/taint/tests.rs +++ b/src/host/taint/tests.rs @@ -176,6 +176,104 @@ fn peerspeak_tagged_nodes_are_excluded_and_plain_apps_are_not() { assert_tainted(&decisions, sink, "tainted-upstream"); } +/// Each ownership carrier must work **alone** (v3.5 §5.1). +/// +/// ⚠️ The phase-3r lesson, applied deliberately: a gate that asserts a value +/// two sources can satisfy gates neither. `peerspeak_tagged_nodes_…` above +/// uses nodes carrying both carriers, so it would keep passing if either +/// were deleted. These are the rows that actually pin them. +#[test] +fn either_ownership_carrier_alone_taints_the_node() { + let mut graph = Graph::new(); + let sink = graph.device_node("hw-sink", MediaRole::Sink); + // Carrier 1: the property, on a node whose name says nothing. + let prop_only = graph.peerspeak_node_prop_only("some-playback-stream", 7); + // Carrier 2: the name prefix, property absent — the F1 case. + let name_only = graph.peerspeak_node_name_only("mpv", 31_284); + let firefox = graph.app_node("firefox", MediaRole::StreamOutput, 11_114); + for node in [prop_only, name_only, firefox] { + graph.link(node, sink); + } + + assert_partition( + &run(&graph, &ctx()), + &[("firefox", firefox)], + &[ + ("prop_only", prop_only, "peerspeak-owned"), + ("name_only", name_only, "peerspeak-owned"), + ], + ); +} + +/// The prefix is a **prefix**, not a substring: an unrelated app must not be +/// excluded because the literal appears somewhere in its name. Over-exclusion +/// is the safe direction, but it is still wrong, and the phase-5 gate now +/// asserts exact partitions in both halves. +#[test] +fn the_owned_prefix_matches_only_at_the_start_of_node_name() { + let mut graph = Graph::new(); + let sink = graph.device_node("hw-sink", MediaRole::Sink); + let impostor = graph.app_node( + &format!("recorder-of-{}stuff", super::PEERSPEAK_OWNED_NODE_PREFIX), + MediaRole::StreamOutput, + 11_114, + ); + graph.link(impostor, sink); + + assert_partition(&run(&graph, &ctx()), &[("impostor", impostor)], &[]); +} + +/// The consumer half of the cross-repo contract test (impl plan §3 +/// requirement 2). peerspeak runs the mirror of this against a byte-identical +/// copy of the same file, and asserts the environment a real child `Command` +/// would carry produces exactly these literals. +/// +/// This proves the two repos agree on the *literals*. That pixelpass actually +/// *listens* is proven by the two carrier tests above, and against the live +/// graph by the phase 5 dry-run. +#[test] +fn ownership_carriers_match_the_cross_repo_fixture() { + const FIXTURE: &str = include_str!("../../../tests/fixtures/ownership-tag-contract.txt"); + + let pinned: Vec<(&str, &str)> = FIXTURE + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .map(|line| line.split_once('=').expect("fixture line is key=value")) + .collect(); + let get = |key: &str| -> &str { + pinned + .iter() + .find(|(k, _)| *k == key) + .unwrap_or_else(|| panic!("fixture has no key {key:?}")) + .1 + }; + + assert_eq!(super::PEERSPEAK_OWNED_PROP, get("prop_key")); + assert_eq!(super::PEERSPEAK_OWNED_NODE_PREFIX, get("node_name_prefix")); + + // The value the producer pins 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(); + let sink = graph.device_node("hw-sink", MediaRole::Sink); + let example = graph.app_node(get("node_name_example"), MediaRole::StreamOutput, 31_284); + graph.link(example, sink); + assert_partition( + &run(&graph, &ctx()), + &[], + &[("example", example, "peerspeak-owned")], + ); +} + #[test] fn aec_identity_is_exact_equality_and_other_modules_stay_eligible() { let mut graph = Graph::new(); diff --git a/tests/fixtures/ownership-tag-contract.txt b/tests/fixtures/ownership-tag-contract.txt new file mode 100644 index 0000000..471c413 --- /dev/null +++ b/tests/fixtures/ownership-tag-contract.txt @@ -0,0 +1,34 @@ +# Screenshare audio exclusion — ownership tagging wire contract. +# +# peerspeak PRODUCES these carriers on every audio node it owns; pixelpass +# CONSUMES them as the primary taint root of the exclusion engine. Neither +# repo depends on the other, so this file is the contract: it is committed +# byte-identical in both, and each repo has a test that asserts its own named +# constants (and, on the producer side, the environment a real child Command +# would carry) match these values exactly. +# +# peerspeak/tests/fixtures/ownership-tag-contract.txt +# pixelpass/tests/fixtures/ownership-tag-contract.txt +# +# Pinned by peerspeak docs/screenshare-audio-exclusion-impl-plan.md §3 and +# docs/screenshare-audio-exclusion-plan.md §5.1 (v3.5). Changing a value here +# is a cross-repo breaking change: both repos must land in the same session, +# and the phase 5 matrix must be re-run. +# +# Two carriers, matched as a UNION — a node is peerspeak-owned if EITHER +# matches. Round 8 added the second because a property is invisible to the +# 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. +prop_key=peerspeak.owned +prop_value=1 + +# Carrier 2 — a `node.name` prefix, announced by the registry without a bind. +# `node.description` is deliberately NOT touched, so mixers still show "mpv". +# Only the prefix is matched; the rest of the name is for diagnostics. +node_name_prefix=peerspeak_owned_ +node_name_format=peerspeak_owned__ +node_name_example=peerspeak_owned_mpv_31284 From 8b41f64e1285157974efcd797031ddd0ebbd6f5f Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 20:06:06 -0400 Subject: [PATCH 02/11] host/observer: tag the live prop-recovery row with the real wire value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification-round follow-up to 1b01847. The phase-3r live row proves carrier 1 survives the bind, which is the property F1 destroyed — but it tagged its fixture sink with `peerspeak.owned=true`, not the `1` the contract pins and peerspeak actually emits. It would have passed even if the real literal did not. Adds PEERSPEAK_OWNED_VALUE so the fixture can name the producer's value, and asserts it against the shared contract file alongside the other two literals. The sink's name still deliberately avoids the `peerspeak_owned_` prefix, so carrier 2 cannot stand in for carrier 1 in that row. Co-Authored-By: Claude Opus 5 --- src/host/observer/adapter.rs | 10 +++++++++- src/host/taint/mod.rs | 9 +++++++++ src/host/taint/tests.rs | 7 ++++--- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/host/observer/adapter.rs b/src/host/observer/adapter.rs index ef6302e..387bb26 100644 --- a/src/host/observer/adapter.rs +++ b/src/host/observer/adapter.rs @@ -898,7 +898,15 @@ mod tests { "module-null-sink", &[ format!("sink_name={unique}"), - "sink_properties=peerspeak.owned=true node.passthrough=true".to_string(), + // The value peerspeak actually emits, not merely a truthy one: + // this row is the live proof that carrier 1 survives the bind, + // and the sink's name deliberately does *not* carry the + // `peerspeak_owned_` prefix, so carrier 2 cannot stand in for + // it here. + format!( + "sink_properties={PEERSPEAK_OWNED_PROP}={} node.passthrough=true", + crate::host::taint::PEERSPEAK_OWNED_VALUE + ), ], ); let null_sink_id = null_sink.id.expect("null-sink module must have an id"); diff --git a/src/host/taint/mod.rs b/src/host/taint/mod.rs index 4c4e9b4..2479be6 100644 --- a/src/host/taint/mod.rs +++ b/src/host/taint/mod.rs @@ -136,6 +136,15 @@ pub const ECHO_CANCEL_GROUP_PREFIX: &str = "echo-cancel-"; /// change that invalidates the phase 5 matrix. pub const PEERSPEAK_OWNED_PROP: &str = "peerspeak.owned"; +/// The value peerspeak emits for [`PEERSPEAK_OWNED_PROP`]. +/// +/// The consumer deliberately accepts **any** truthy value, not just this one +/// (see the observer's `truthy`) — treating an unexpected value as "owned" is +/// the fail-closed direction. This constant exists so live tests can tag a +/// node with exactly what the producer sends, rather than something merely +/// truthy that would pass even if the real literal did not. +pub const PEERSPEAK_OWNED_VALUE: &str = "1"; + /// Ownership carrier 2: a `node.name` prefix (v3.5 §5.1, round 8). /// /// Matched as a **union** with [`PEERSPEAK_OWNED_PROP`] — either one makes a diff --git a/src/host/taint/tests.rs b/src/host/taint/tests.rs index 0c4faa2..10df0c4 100644 --- a/src/host/taint/tests.rs +++ b/src/host/taint/tests.rs @@ -251,10 +251,11 @@ 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")); + assert_eq!(super::PEERSPEAK_OWNED_VALUE, get("prop_value")); - // The value the producer pins 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. + // …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", From 45ca5057f86984d56b439f9af370a039d7502ba7 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 20:25:22 -0400 Subject: [PATCH 03/11] host/taint: refuse an ambiguous contract fixture instead of resolving it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex phase-1 review, finding 3 (P2), concrete half. This side searched a list and took the first match for a key; peerspeak's side collected into a map and took the last. A byte-identical fixture containing a duplicated key would therefore leave both suites green while the two repos had selected *different* contracts — the precise drift the shared file exists to prevent. Both sides now assert the key is not already defined. Verified by appending a duplicate `prop_value` to both fixtures: both suites fail. The rest of finding 3 — one CI gate that feeds peerspeak's real tag_child output through this repo's actual adapter and classifier, rather than two per-repo literal tests — is a larger piece of work and is not attempted here. Co-Authored-By: Claude Opus 5 --- src/host/taint/tests.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/host/taint/tests.rs b/src/host/taint/tests.rs index 10df0c4..eff5f24 100644 --- a/src/host/taint/tests.rs +++ b/src/host/taint/tests.rs @@ -241,6 +241,17 @@ fn ownership_carriers_match_the_cross_repo_fixture() { .filter(|line| !line.is_empty() && !line.starts_with('#')) .map(|line| line.split_once('=').expect("fixture line is key=value")) .collect(); + + // ⚠️ Refuse a duplicated key rather than resolving it (Codex phase-1 + // review, finding 3). This side takes the first match and peerspeak's + // took the last, so a duplicate in a byte-identical file could leave both + // repos green having selected *different* contracts. + for (index, (key, _)) in pinned.iter().enumerate() { + assert!( + !pinned[..index].iter().any(|(seen, _)| seen == key), + "fixture defines {key:?} twice; the two repos would disagree on which wins" + ); + } let get = |key: &str| -> &str { pinned .iter() From bf5f2508b877ba9c43ad6f0dd256da08d88c550c Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 20:46:40 -0400 Subject: [PATCH 04/11] host/taint: honour the ownership carriers on producers only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither ownership carrier is a security boundary — both are strings any unprivileged process can put on its own node — so an unrestricted taint root is a denial of the whole feature. An unlinked Stream/Input/Audio named `peerspeak_owned_rogue` is a tainted *reader* (receivers includes nodes by role, no link required) and an unbounded one, so propagate_unresolved_owner fails every candidate on the machine closed. Measured before this change: BASELINE eligible=1 excluded=[] became WITH IMPOSTOR eligible=0 excluded=[firefox -> unresolved-owner]. Restricting the root to Stream/Output/Audio costs nothing real — peerspeak only ever tags playback streams — and the AEC's virtual sink/source is untouched, since it roots on module id, not on this tag. A tag that is ignored is not silent: misplaced_ownership_tags feeds a new `ignored_ownership_tags` audit field (omitted when empty), because the fix *removes* an exclusion, and the two causes of a dropped tag — a peerspeak tagging bug, or an impersonation attempt — both want seeing. Codex phase-1 review F2, reproduced live. Round 10, R10-1. 5 new rows, mutation-verified: dropping the role restriction kills both engine rows, and stubbing the diagnostic kills the third. Co-Authored-By: Claude Opus 5 --- src/host/audit/mod.rs | 32 ++++++++++++ src/host/audit/tests.rs | 49 ++++++++++++++++++ src/host/taint/fixture.rs | 10 ++++ src/host/taint/mod.rs | 53 +++++++++++++++++--- src/host/taint/snapshot.rs | 14 ++++++ src/host/taint/tests.rs | 100 +++++++++++++++++++++++++++++++++++++ 6 files changed, 252 insertions(+), 6 deletions(-) diff --git a/src/host/audit/mod.rs b/src/host/audit/mod.rs index 996805a..b76dfc9 100644 --- a/src/host/audit/mod.rs +++ b/src/host/audit/mod.rs @@ -198,6 +198,17 @@ pub struct TaintRow { pub sticky: bool, } +/// A node carrying a peerspeak ownership carrier on a role the engine does not +/// honour it on (round 10, R10-1). `role` is the point of the row: it says +/// which non-producer role the tag turned up on, which is what distinguishes a +/// producer-side bug from an impersonation attempt. +#[derive(Clone, Debug, PartialEq, Eq, Serialize)] +pub struct IgnoredTagRow { + pub serial: u64, + pub name: Option, + pub role: &'static str, +} + /// The decision content of one recompute — everything except which recompute it /// was. Split out from [`AuditRecord`] so "did anything actually change?" is a /// derived `==` rather than a hand-maintained field comparison that a later @@ -227,6 +238,16 @@ pub struct AuditBody { pub excluded_count: usize, /// Taint across all node roles, ascending by serial. pub taint: Vec, + /// Nodes carrying a peerspeak ownership carrier that the engine + /// **ignored** because they are not `Stream/Output/Audio` (round 10, + /// R10-1). Normally empty; a non-empty list means either peerspeak is + /// tagging something it should not, or a process is impersonating the + /// tag. Neither is an exclusion, and neither should be silent. + /// + /// Omitted from the JSONL when empty, so it costs nothing on the common + /// path and is impossible to miss when it is not. + #[serde(skip_serializing_if = "Vec::is_empty")] + pub ignored_ownership_tags: Vec, } impl AuditBody { @@ -433,6 +454,16 @@ fn build_body( }) .collect(); + let ignored_ownership_tags: Vec = + crate::host::taint::misplaced_ownership_tags(&projection.snapshot) + .into_iter() + .map(|node| IgnoredTagRow { + serial: node.serial.0, + name: node.name.clone(), + role: node.role.code(), + }) + .collect(); + AuditBody { graph_ready: projection.graph_ready, epoch: readiness_code(projection.readiness), @@ -444,6 +475,7 @@ fn build_body( eligible_count, candidates, taint, + ignored_ownership_tags, } } diff --git a/src/host/audit/tests.rs b/src/host/audit/tests.rs index 473a6b3..f62a0de 100644 --- a/src/host/audit/tests.rs +++ b/src/host/audit/tests.rs @@ -125,6 +125,55 @@ fn the_record_carries_the_complete_candidate_universe() { assert_eq!(outcome.record.body.excluded_count, 0); } +/// **R10-1's diagnostic reaches the record.** The engine deliberately ignores +/// an ownership carrier on a non-producer, which means the fix removes an +/// exclusion — so the only way an operator learns a tag was seen and dropped is +/// this field. A matrix row that silently grew an impostor would otherwise read +/// as a clean pass. +#[test] +fn an_ignored_ownership_tag_is_reported_without_excluding_anything() { + let mut graph = Graph::new(); + graph.app_node("music", MediaRole::StreamOutput, 100); + let impostor = graph.peerspeak_tagged_node("rogue", MediaRole::StreamInput, 4_242); + let projection = ready(graph.build()); + + let body = observe(&mut auditor_off(), &projection, 0).record.body; + + // The bystander is untouched — the point of the fix. + let (eligible, excluded) = partition(&body); + assert_eq!(eligible, vec!["music"]); + assert!(excluded.is_empty(), "unexpected exclusions: {excluded:?}"); + assert!(body.taint.is_empty(), "unexpected taint: {:?}", body.taint); + + // ...but the tag is not silent, and the row names the role it appeared on. + assert_eq!(body.ignored_ownership_tags.len(), 1); + let row = &body.ignored_ownership_tags[0]; + assert_eq!(row.serial, impostor.serial.0); + assert_eq!(row.role, "stream-input"); + assert_eq!(row.name.as_deref(), Some(owned_name("rogue", 4_242).as_str())); +} + +/// The common path stays quiet: a correctly tagged peerspeak producer is +/// honoured as a taint root and is *not* reported as a misplaced tag. Without +/// this, a diagnostic that fired on every normal run would be worthless. +#[test] +fn a_correctly_tagged_producer_is_not_reported_as_misplaced() { + let mut graph = Graph::new(); + let sink = graph.device_node("speakers", MediaRole::Sink); + let call = graph.peerspeak_node("call", 200); + graph.link(call, sink); + let projection = ready(graph.build()); + + let body = observe(&mut auditor_off(), &projection, 0).record.body; + + assert_eq!(body.excluded_count, 1); + assert!( + body.ignored_ownership_tags.is_empty(), + "honoured tag reported as misplaced: {:?}", + body.ignored_ownership_tags + ); +} + /// The fail-closed default asserted at the boundary (impl plan §4, phase 2's /// "one addition"): nothing in, nothing eligible — and, just as importantly, no /// panic and no invented row. diff --git a/src/host/taint/fixture.rs b/src/host/taint/fixture.rs index 4ae652f..7d146aa 100644 --- a/src/host/taint/fixture.rs +++ b/src/host/taint/fixture.rs @@ -161,6 +161,16 @@ impl Graph { self.node(&name, MediaRole::StreamOutput, peerspeak_owned(client, pid)) } + /// Both ownership carriers on a node of **any** role — an impostor, or a + /// producer-side tagging bug. Only [`MediaRole::StreamOutput`] makes it a + /// taint root (round 10, R10-1); every other role must be ignored, and + /// these are the fixtures that prove it. + pub fn peerspeak_tagged_node(&mut self, name: &str, role: MediaRole, pid: u32) -> NodeRef { + let client = self.client_of_app(pid); + let name = format!("{}{name}_{pid}", super::PEERSPEAK_OWNED_NODE_PREFIX); + self.node(&name, role, peerspeak_owned(client, pid)) + } + /// Carrier 1 alone: the `peerspeak.owned` property present, the /// `node.name` prefix absent. What the engine sees for a node it had to /// bind to observe (v3.5 §6.7). diff --git a/src/host/taint/mod.rs b/src/host/taint/mod.rs index 2479be6..488d267 100644 --- a/src/host/taint/mod.rs +++ b/src/host/taint/mod.rs @@ -566,18 +566,59 @@ fn ambiguous_id_nodes(snapshot: &GraphSnapshot) -> BTreeSet { .collect() } +/// Does this node carry either ownership carrier? **Tag presence only** — it +/// deliberately says nothing about whether the tag is honoured, which is +/// `local_root_reason`'s business (round 10 restricts that to producers). +/// Split out so the "is it tagged?" and "does the tag count?" questions can +/// be tested, and reported, independently. +pub fn is_peerspeak_tagged(node: &NodeSnapshot) -> bool { + node.props.peerspeak_owned + || node + .name + .as_deref() + .is_some_and(|name| name.starts_with(PEERSPEAK_OWNED_NODE_PREFIX)) +} + +/// Nodes carrying an ownership carrier that `local_root_reason` **ignored** +/// because the node is not a producer (round 10, R10-1). Ascending by serial. +/// +/// Purely diagnostic — nothing in the engine consumes it. It exists because +/// R10-1 turns a formerly load-bearing tag into a no-op, and a silently +/// ignored tag has exactly two causes, both of which someone wants to know +/// about: peerspeak tagging a node it should not (a producer-side bug this +/// would otherwise hide), or another process impersonating the tag (the F2 +/// attack, now defanged but still worth seeing). +pub fn misplaced_ownership_tags(snapshot: &GraphSnapshot) -> Vec<&NodeSnapshot> { + let mut tagged: Vec<&NodeSnapshot> = snapshot + .nodes() + .filter(|node| node.role != MediaRole::StreamOutput && is_peerspeak_tagged(node)) + .collect(); + tagged.sort_by_key(|node| node.serial); + tagged +} + fn local_root_reason(node: &NodeSnapshot, ctx: &ExclusionCtx) -> Option { // The two ownership carriers, as a union (v3.5 §5.1). Kept here rather // than folded together at the observer boundary so that the union is a // pure, directly-testable rule: an adapter that collapsed both into the // one `peerspeak_owned` bool would make each carrier untestable alone, // which is exactly how phase 3r's row 1 nearly gated nothing. - if node.props.peerspeak_owned - || node - .name - .as_deref() - .is_some_and(|name| name.starts_with(PEERSPEAK_OWNED_NODE_PREFIX)) - { + // + // ⚠️ **Producer roles only** (round 10, R10-1). Neither carrier is a + // security boundary — both are strings any unprivileged process can put + // on its own node — so an unrestricted root is a denial of the whole + // feature: an unlinked `Stream/Input/Audio` named `peerspeak_owned_x` + // is a tainted *reader* with no owner bound to it, which fails every + // candidate closed machine-wide (Codex phase-1 F2, reproduced live). + // Restricting the root to `Stream/Output/Audio` costs nothing real — + // peerspeak only ever tags playback streams — and the attack needs the + // impostor to be a plausible playback node instead, which taints only + // its own descendants. The AEC's virtual sink/source is unaffected: it + // roots on [`Reason::AecIdentity`] below, by module id, not by this tag. + // A tag on a non-producer falls through: ignored for taint, but not + // nothing — it is either a peerspeak bug or an impostor, and + // [`misplaced_ownership_tags`] surfaces it so neither is silent. + if is_peerspeak_tagged(node) && node.role == MediaRole::StreamOutput { return Some(Reason::PeerspeakOwned); } if let (Some(module), Some(aec)) = (node.props.pulse_module_id, ctx.aec_module_id) diff --git a/src/host/taint/snapshot.rs b/src/host/taint/snapshot.rs index 29b02f5..1f669f7 100644 --- a/src/host/taint/snapshot.rs +++ b/src/host/taint/snapshot.rs @@ -87,6 +87,20 @@ impl MediaRole { pub fn is_candidate(self) -> bool { matches!(self, Self::StreamOutput) } + + /// Stable machine-readable code for the audit output. Not the raw + /// `media.class`: `Other` has no single one, and the audit's codes are a + /// contract with the matrix, not with PipeWire. + pub fn code(self) -> &'static str { + match self { + Self::StreamOutput => "stream-output", + Self::StreamInput => "stream-input", + Self::Sink => "sink", + Self::Source => "source", + Self::Duplex => "duplex", + Self::Other => "other", + } + } } /// The subset of node properties the engine actually reasons about. diff --git a/src/host/taint/tests.rs b/src/host/taint/tests.rs index eff5f24..ee9c285 100644 --- a/src/host/taint/tests.rs +++ b/src/host/taint/tests.rs @@ -205,6 +205,106 @@ fn either_ownership_carrier_alone_taints_the_node() { ); } +/// **R10-1, the F2 fix.** Neither carrier is a security boundary — both are +/// strings any unprivileged process can set on its own node — so the tag is +/// honoured only on `Stream/Output/Audio`, the one role peerspeak ever tags. +/// +/// Without the restriction, a tagged `Stream/Input/Audio` **with no links at +/// all** is a tainted *reader* (`receivers` includes nodes by role, no link +/// required), and an unbounded one, so `propagate_unresolved_owner` fails +/// every candidate on the machine closed. That is a whole-feature denial from +/// an unprivileged process, reproduced live during the phase-1 review. +#[test] +fn an_ownership_tag_on_a_non_producer_is_not_a_taint_root() { + for role in [ + MediaRole::StreamInput, + MediaRole::Sink, + MediaRole::Source, + MediaRole::Duplex, + MediaRole::Other, + ] { + let mut graph = Graph::new(); + let sink = graph.device_node("hw-sink", MediaRole::Sink); + let firefox = graph.app_node("firefox", MediaRole::StreamOutput, 11_114); + graph.link(firefox, sink); + // Deliberately unlinked: the F2 shape needs no edges whatsoever. + let impostor = graph.peerspeak_tagged_node("rogue", role, 4_242); + + let decisions = run(&graph, &ctx()); + assert_untainted(&decisions, impostor); + assert!( + decisions.taint.is_empty(), + "{role:?} impostor tainted something: {:?}", + decisions.taint.keys().collect::>() + ); + // The whole point: the eligible half stays non-empty. + assert_partition(&decisions, &[("firefox", firefox)], &[]); + } +} + +/// **The live F2 reproduction, verbatim.** The measured impostor was an +/// *unbounded* reader — `client.id` present, `application.process.id` absent +/// — which is what turns "one bogus tainted node" into "nothing on this +/// machine is shareable": `propagate_unresolved_owner` cannot prove any +/// candidate independent of a reader it cannot attribute to an owner. +/// +/// Measured before the fix: `BASELINE eligible=1 excluded=[]` → +/// `WITH IMPOSTOR eligible=0 excluded=[firefox → unresolved-owner]`. +/// +/// Distinct from the row above, which uses a *bounded* impostor and so would +/// still pass if only the cheap half of the fix were present. +#[test] +fn an_unbounded_tagged_impostor_cannot_exclude_a_bystander_app() { + let mut graph = Graph::new(); + let sink = graph.device_node("hw-sink", MediaRole::Sink); + let firefox = graph.app_node("firefox", MediaRole::StreamOutput, 11_114); + let mpv = graph.app_node("mpv", MediaRole::StreamOutput, 31_284); + for node in [firefox, mpv] { + graph.link(node, sink); + } + + let baseline = run(&graph, &ctx()); + assert_partition(&baseline, &[("firefox", firefox), ("mpv", mpv)], &[]); + + // Both carriers, no pid, no links — everything an unprivileged process + // can arrange for itself in one `pw-cli` invocation. + let rogue_client = graph.client(Some(PULSE_PID)); + let impostor = graph.node( + &format!("{}rogue_4242", super::PEERSPEAK_OWNED_NODE_PREFIX), + MediaRole::StreamInput, + NodeProps { + peerspeak_owned: true, + client_id: Some(rogue_client), + ..NodeProps::default() + }, + ); + + let decisions = run(&graph, &ctx()); + assert_untainted(&decisions, impostor); + assert_partition(&decisions, &[("firefox", firefox), ("mpv", mpv)], &[]); +} + +/// A tag that R10-1 ignores is still reported, so that neither a peerspeak +/// tagging bug nor an impersonation attempt is silent. +#[test] +fn ignored_ownership_tags_are_surfaced_for_diagnostics() { + let mut graph = Graph::new(); + let sink = graph.device_node("hw-sink", MediaRole::Sink); + let call = graph.peerspeak_node("call", 7); + graph.link(call, sink); + let impostor = graph.peerspeak_tagged_node("rogue", MediaRole::StreamInput, 4_242); + + let snapshot = graph.build(); + let misplaced: Vec = super::misplaced_ownership_tags(&snapshot) + .iter() + .map(|node| node.serial) + .collect(); + + // Exactly the ignored one: the honoured producer is not "misplaced". + assert_eq!(misplaced, vec![impostor.serial]); + assert_ne!(impostor.serial, call.serial); +} + /// The prefix is a **prefix**, not a substring: an unrelated app must not be /// excluded because the literal appears somewhere in its name. Over-exclusion /// is the safe direction, but it is still wrong, and the phase-5 gate now From 295a575b15b2c9ead4fcdf55b919b2b52ee9f638 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 20:57:47 -0400 Subject: [PATCH 05/11] host/taint: owner key 4 falls back to the Client's pipewire.sec.pid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Native PipeWire clients put no application.process.id on their nodes — only client.id. keys_of read node properties alone, so those nodes had no key 4, were therefore unbounded, and propagate_unresolved_owner excluded them the moment any tainted reader existed anywhere on the machine. Measured: an untagged mpv was eligible alone, and became unresolved-owner the instant peerspeak played audio. Since peerspeak playing audio is the only situation in which this feature runs, that amounted to "native PipeWire apps are never shareable". The tainted reader that armed it was sunshine, which is itself bounded — so this is the bounded-reader arm, not the keyless-reader case §6.1.1 narrates. The pid is one hop away, on the node's Client, already in the snapshot. RISK, and the guard on it: every Pulse-emulated Client carries pipewire-pulse's own PID as sec_pid — measured, 15 unrelated Clients sharing 2528 on this host. An unguarded fallback would fuse all of them into one owner. Exception 1 therefore applies to the fallback exactly as it does to the node's own property, so the fallback strictly *adds* correct bounding rather than trading it. Ambiguous client ids yield no fallback pid: inventing an owner key is the one direction that can reduce taint, so a coin toss is the wrong guess. The client index is threaded through a new OwnerCtx rather than a sixth positional Option, and evaluate() builds one and shares it, so the components and the key index cannot disagree about who is bounded. Round 10, R10-3. 6 new rows; 3 mutations verified — removing the fallback, dropping the pulse-pid exception (11 rows die), and resolving an ambiguous client id instead of dropping it. Co-Authored-By: Claude Opus 5 --- src/host/audit/tests.rs | 5 +- src/host/taint/fixture.rs | 33 ++++++ src/host/taint/mod.rs | 9 +- src/host/taint/owner.rs | 97 +++++++++++++++--- src/host/taint/tests.rs | 210 +++++++++++++++++++++++++++++++++++++- 5 files changed, 333 insertions(+), 21 deletions(-) diff --git a/src/host/audit/tests.rs b/src/host/audit/tests.rs index f62a0de..80fc890 100644 --- a/src/host/audit/tests.rs +++ b/src/host/audit/tests.rs @@ -150,7 +150,10 @@ fn an_ignored_ownership_tag_is_reported_without_excluding_anything() { let row = &body.ignored_ownership_tags[0]; assert_eq!(row.serial, impostor.serial.0); assert_eq!(row.role, "stream-input"); - assert_eq!(row.name.as_deref(), Some(owned_name("rogue", 4_242).as_str())); + assert_eq!( + row.name.as_deref(), + Some(owned_name("rogue", 4_242).as_str()) + ); } /// The common path stays quiet: a correctly tagged peerspeak producer is diff --git a/src/host/taint/fixture.rs b/src/host/taint/fixture.rs index 7d146aa..d0c2647 100644 --- a/src/host/taint/fixture.rs +++ b/src/host/taint/fixture.rs @@ -36,6 +36,10 @@ pub struct Graph { /// (GStreamer opens one per stream) pass clients explicitly instead. client_by_app: BTreeMap, client_by_module: BTreeMap, + /// Native (non-Pulse-emulated) clients, whose `pipewire.sec.pid` is the + /// app's **own** pid rather than pipewire-pulse's. See + /// [`Graph::native_client_node`]. + native_client_by_app: BTreeMap, session_client: Option, } @@ -84,6 +88,35 @@ impl Graph { id } + /// A **native PipeWire** client's stream: `client.id` on the node, **no + /// `application.process.id`**, and the app's real pid only on the Client + /// as `pipewire.sec.pid`. + /// + /// ⚠️ This is what an ordinary app actually looks like when it does not go + /// through pipewire-pulse — measured for mpv on its default ao and for + /// peerspeak's own playback stream. [`Graph::app_node`] models the + /// Pulse-emulated shape, where the pid is on the node and the Client's + /// `sec_pid` is the *daemon's*; both shapes are live on this host, and + /// only this one exercises key 4's Client fallback (round 10, R10-3). + pub fn native_client_node(&mut self, name: &str, role: MediaRole, pid: u32) -> NodeRef { + let client = match self.native_client_by_app.get(&pid) { + Some(id) => *id, + None => { + let id = self.client(Some(pid)); + self.native_client_by_app.insert(pid, id); + id + } + }; + self.node( + name, + role, + NodeProps { + client_id: Some(client), + ..NodeProps::default() + }, + ) + } + /// An ordinary application stream: its own client, its own PID. pub fn app_node(&mut self, name: &str, role: MediaRole, pid: u32) -> NodeRef { let client = self.client_of_app(pid); diff --git a/src/host/taint/mod.rs b/src/host/taint/mod.rs index 488d267..76a8b45 100644 --- a/src/host/taint/mod.rs +++ b/src/host/taint/mod.rs @@ -410,8 +410,13 @@ pub fn evaluate( ctx: &ExclusionCtx, prior: &StickyState, ) -> (Decisions, StickyState) { - let components = OwnerComponents::build(snapshot, ctx.pipewire_pulse_pid); - let keys = owner::OwnerKeyIndex::build(snapshot, ctx.pipewire_pulse_pid); + // Built once and shared: it carries the Client → `pipewire.sec.pid` index + // that key 4 falls back to (round 10, R10-3), so the components and the + // key index must be derived from the *same* one or they would disagree + // about which nodes are bounded. + let owner_ctx = owner::OwnerCtx::new(snapshot, ctx.pipewire_pulse_pid); + let components = OwnerComponents::build(snapshot, &owner_ctx); + let keys = owner::OwnerKeyIndex::build(snapshot, &owner_ctx); // Pass 1 — the fail-closed view. Every decision is made from this one, so // "we could not see" counts as taint. diff --git a/src/host/taint/owner.rs b/src/host/taint/owner.rs index 94de20e..d0cbd7b 100644 --- a/src/host/taint/owner.rs +++ b/src/host/taint/owner.rs @@ -67,10 +67,65 @@ //! Grouping is **transitive** (union-find). That is the fail-closed //! direction: bigger owner components mean more taint, never less. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use super::snapshot::{GlobalId, GraphSnapshot, NodeSnapshot, Serial}; +/// Everything owner-key derivation needs from outside a single node. +/// +/// Introduced by round 10 (R10-3). Before it, `keys_of` read only node +/// properties, and key 4 was therefore available **only** to nodes carrying +/// `application.process.id` — which native PipeWire clients do not. mpv on its +/// default ao, and peerspeak's own playback stream, expose nothing but +/// `client.id`, so both were *unbounded*, and the moment any tainted reader +/// existed anywhere, `propagate_unresolved_owner` excluded every one of them. +/// Measured: an untagged mpv went from eligible (alone) to `unresolved-owner` +/// the instant peerspeak played audio. That is "native-PipeWire apps are never +/// shareable", which is not a feature. +/// +/// The missing pid is not missing at all — it is one hop away, on the node's +/// **Client**, as `pipewire.sec.pid`, and already in the snapshot. +pub struct OwnerCtx { + pub pipewire_pulse_pid: Option, + /// `client.id` → that Client's `pipewire.sec.pid`. + /// + /// Clients whose global id is **ambiguous** (two live objects claiming it, + /// i.e. the observer missed a removal) are deliberately absent: resolving + /// an ambiguous id to a pid would attribute a node to whichever Client won + /// a coin toss, and inventing an owner key is the one direction that can + /// *reduce* taint. Absent ⇒ unbounded ⇒ fails closed, as before. + client_pids: BTreeMap, +} + +impl OwnerCtx { + pub fn new(snapshot: &GraphSnapshot, pipewire_pulse_pid: Option) -> Self { + let mut client_pids: BTreeMap = BTreeMap::new(); + let mut ambiguous: BTreeSet = BTreeSet::new(); + for client in snapshot.clients() { + if client_pids.contains_key(&client.id) || ambiguous.contains(&client.id) { + // Two Clients claiming one id: drop it entirely rather than + // pick. See the field docs. + client_pids.remove(&client.id); + ambiguous.insert(client.id); + continue; + } + if let Some(pid) = client.sec_pid { + client_pids.insert(client.id, pid); + } + } + Self { + pipewire_pulse_pid, + client_pids, + } + } + + /// The `pipewire.sec.pid` of this node's Client, if it has one and that + /// Client's id is unambiguous. + fn client_pid(&self, node: &NodeSnapshot) -> Option { + self.client_pids.get(&node.props.client_id?).copied() + } +} + /// Which key bridged two legs. Ordered strongest first; the `Ord` derive is /// load-bearing for "report the strongest shared key". #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)] @@ -106,7 +161,7 @@ enum KeyValue { /// A key that is present but unusable (the pipewire-pulse PID; a coarse key /// on a device node) is **absent** here — that is the whole mechanism of the /// two exceptions. -fn keys_of(node: &NodeSnapshot, pipewire_pulse_pid: Option) -> Vec<(OwnerKey, KeyValue)> { +fn keys_of(node: &NodeSnapshot, ctx: &OwnerCtx) -> Vec<(OwnerKey, KeyValue)> { let mut out = Vec::new(); if let Some(group) = &node.props.link_group { out.push((OwnerKey::LinkGroup, KeyValue::Text(group.clone()))); @@ -122,13 +177,25 @@ fn keys_of(node: &NodeSnapshot, pipewire_pulse_pid: Option) -> Vec<(OwnerKe if let Some(client) = node.props.client_id { out.push((OwnerKey::ClientId, KeyValue::Num(u64::from(client.0)))); } - if let Some(pid) = node.props.process_id { - // Exception 1. Note the fail-closed asymmetry when the daemon PID is - // unknown (`None`): the exception does *not* fire, key 4 applies to + // 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. + // + // ⚠️ **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) != pipewire_pulse_pid { + if Some(pid) != ctx.pipewire_pulse_pid { out.push((OwnerKey::ProcessId, KeyValue::Num(u64::from(pid)))); } } @@ -151,8 +218,8 @@ fn keys_of(node: &NodeSnapshot, pipewire_pulse_pid: Option) -> Vec<(OwnerKe /// nothing else relates them. Its sibling output leg cannot be found, so /// the engine must fail closed rather than declare it clean /// (v3.4 §6.1.1, final paragraph). -pub fn owner_is_bounded(node: &NodeSnapshot, pipewire_pulse_pid: Option) -> bool { - keys_of(node, pipewire_pulse_pid) +pub fn owner_is_bounded(node: &NodeSnapshot, ctx: &OwnerCtx) -> bool { + keys_of(node, ctx) .iter() .any(|(key, _)| *key != OwnerKey::ClientId) } @@ -168,11 +235,11 @@ pub struct OwnerKeyIndex { } impl OwnerKeyIndex { - pub fn build(snapshot: &GraphSnapshot, pipewire_pulse_pid: Option) -> Self { + pub fn build(snapshot: &GraphSnapshot, ctx: &OwnerCtx) -> Self { Self { keys: snapshot .nodes() - .map(|node| (node.serial, keys_of(node, pipewire_pulse_pid))) + .map(|node| (node.serial, keys_of(node, ctx))) .collect(), } } @@ -252,10 +319,10 @@ impl OwnerKeyIndex { pub fn strongest_shared_key( a: &NodeSnapshot, b: &NodeSnapshot, - pipewire_pulse_pid: Option, + ctx: &OwnerCtx, ) -> Option { - let a_keys = keys_of(a, pipewire_pulse_pid); - let b_keys = keys_of(b, pipewire_pulse_pid); + let a_keys = keys_of(a, ctx); + let b_keys = keys_of(b, ctx); // `keys_of` yields strongest-first, so the first match is the strongest. a_keys.iter().find_map(|(key, value)| { b_keys @@ -279,7 +346,7 @@ pub struct OwnerComponents { } impl OwnerComponents { - pub fn build(snapshot: &GraphSnapshot, pipewire_pulse_pid: Option) -> Self { + pub fn build(snapshot: &GraphSnapshot, ctx: &OwnerCtx) -> Self { let serials: Vec = snapshot.nodes().map(|n| n.serial).collect(); let index: BTreeMap = serials.iter().enumerate().map(|(i, s)| (*s, i)).collect(); @@ -290,7 +357,7 @@ impl OwnerComponents { let mut buckets: BTreeMap<(OwnerKey, KeyValue), Vec> = BTreeMap::new(); for node in snapshot.nodes() { let slot = index[&node.serial]; - for (key, value) in keys_of(node, pipewire_pulse_pid) { + for (key, value) in keys_of(node, ctx) { buckets.entry((key, value)).or_default().push(slot); } } diff --git a/src/host/taint/tests.rs b/src/host/taint/tests.rs index ee9c285..c8e57f5 100644 --- a/src/host/taint/tests.rs +++ b/src/host/taint/tests.rs @@ -15,7 +15,7 @@ use std::collections::BTreeSet; use super::fixture::{Graph, NodeRef, PULSE_PID, app}; -use super::owner::{OwnerKey, strongest_shared_key}; +use super::owner::{OwnerCtx, OwnerKey, strongest_shared_key}; use super::snapshot::{MediaRole, NodeProps, PortDirection, Serial}; use super::{Decisions, Eligibility, ExclusionCtx, ObjectRef, Reason, StickyState, evaluate}; @@ -717,8 +717,9 @@ fn owner_key_union_falls_through_a_present_but_unequal_key() { snapshot.node(b.serial).unwrap(), ); assert_ne!(a.props.client_id, b.props.client_id); + let owner_ctx = OwnerCtx::new(&snapshot, Some(PULSE_PID)); assert_eq!( - strongest_shared_key(a, b, Some(PULSE_PID)), + strongest_shared_key(a, b, &owner_ctx), Some(OwnerKey::ProcessId) ); } @@ -729,11 +730,12 @@ fn the_strongest_shared_key_wins_when_several_match() { let a = graph.group_node("a", MediaRole::StreamInput, "g", 500); let b = graph.group_node("b", MediaRole::StreamOutput, "g", 500); let snapshot = graph.build(); + let owner_ctx = OwnerCtx::new(&snapshot, Some(PULSE_PID)); assert_eq!( strongest_shared_key( snapshot.node(a.serial).unwrap(), snapshot.node(b.serial).unwrap(), - Some(PULSE_PID) + &owner_ctx ), Some(OwnerKey::LinkGroup) ); @@ -770,6 +772,208 @@ fn the_pipewire_pulse_pid_does_not_fuse_unrelated_modules() { assert_untainted(&decisions, b_in); } +/// **R10-3, the fix.** A native PipeWire client puts no +/// `application.process.id` on its node — only `client.id` — so before the +/// Client fallback it had no key 4, was therefore *unbounded*, and +/// `propagate_unresolved_owner` excluded it the moment any tainted reader +/// existed anywhere on the machine. +/// +/// Measured live: an untagged mpv was eligible alone, and became +/// `unresolved-owner` the instant peerspeak played audio. Since peerspeak +/// playing audio is the only situation in which this feature runs at all, that +/// amounted to "native-PipeWire apps are never shareable". +#[test] +fn a_native_client_is_bounded_by_its_clients_sec_pid() { + 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); + + // The tainted reader that arms the unresolved-owner arm. Bounded itself + // (a real pid), exactly as the live `sunshine` was — so this is the + // bounded-reader arm, not the keyless-reader one. + let sunshine = graph.app_node("sunshine", MediaRole::StreamInput, 3_838); + graph.link(hw, sunshine); + + // mpv on its default ao: client.id only, pid on the Client. + let mpv = graph.native_client_node("mpv", MediaRole::StreamOutput, 31_284); + graph.link(mpv, hw); + + assert_partition( + &run(&graph, &ctx()), + &[("mpv", mpv)], + &[("call", call, "peerspeak-owned")], + ); +} + +/// The fallback must bridge a native app's *own* legs, or it has bought +/// boundedness without buying correctness: an app that reads the call and +/// re-emits it on a second native node would be declared clean. +#[test] +fn the_sec_pid_fallback_still_bridges_a_native_apps_own_legs() { + 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 nodes, no link between them — the forwarder + // shape, in the native flavour. + let leg_in = graph.native_client_node("forwarder-in", MediaRole::StreamInput, 50_000); + let leg_out = graph.native_client_node("forwarder-out", MediaRole::StreamOutput, 50_000); + graph.link(hw, leg_in); + + let decisions = run(&graph, &ctx()); + assert_tainted(&decisions, leg_out, "tainted-owner-bridge"); + assert_partition( + &decisions, + &[], + &[ + ("call", call, "peerspeak-owned"), + ("forwarder-out", leg_out, "tainted-owner-bridge"), + ], + ); +} + +/// **The risk the fallback creates, and the guard on it.** Every +/// Pulse-emulated Client carries pipewire-pulse's own PID as `sec_pid` — +/// measured, 15 unrelated Clients sharing 2528 on this host. An unguarded +/// fallback would give all of them key 4 with the *same* value and fuse them +/// into one owner, so a single tainted Pulse app would exclude every other +/// Pulse app on the machine. +/// +/// Exception 1 therefore applies to the fallback exactly as it does to the +/// node's own property. Without that, this row goes red. +#[test] +fn the_sec_pid_fallback_does_not_fuse_every_pulse_client() { + 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); + + // Three unrelated Pulse-emulated apps, each on its own Client, none + // exposing a node-level pid — so each can only reach key 4 through its + // Client, whose sec_pid is the daemon's. + let pulse_app = |graph: &mut Graph, name: &str, role| { + let client = graph.client(Some(PULSE_PID)); + graph.node( + name, + role, + NodeProps { + client_id: Some(client), + ..NodeProps::default() + }, + ) + }; + // One of them reads the tainted sink; the other two must not care. + let reader = pulse_app(&mut graph, "recorder", MediaRole::StreamInput); + graph.link(hw, reader); + let other_a = pulse_app(&mut graph, "player-a", MediaRole::StreamOutput); + let other_b = pulse_app(&mut graph, "player-b", MediaRole::StreamOutput); + + let decisions = run(&graph, &ctx()); + // They are unbounded (`client.id` alone never bounds an owner), so the + // fail-closed arm still excludes them — but as `unresolved-owner`, NOT as + // `tainted-owner-bridge`. That distinction is the whole assertion: a + // bridge reason here would mean the daemon pid had fused three unrelated + // applications into one owner, and unlike fail-closed exclusion, fusion + // does not go away when the apps are given real pids + // (`distinct_sec_pids_bound_each_native_app_separately` is that half). + assert_tainted(&decisions, other_a, "unresolved-owner"); + assert_tainted(&decisions, other_b, "unresolved-owner"); + for node in [other_a, other_b] { + assert_ne!( + decisions.taint.get(&node.serial).map(|e| e.reason.code()), + Some("tainted-owner-bridge"), + "the daemon pid must not bridge unrelated Pulse clients" + ); + } +} + +/// The same three apps, given **real per-app** `sec_pid`s: now the fallback +/// fires, all three are bounded, and only the one actually reading the call is +/// affected. This is the row that proves the guard above suppresses the daemon +/// pid *specifically* rather than disabling the fallback outright. +#[test] +fn distinct_sec_pids_bound_each_native_app_separately() { + 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); + + let reader = graph.native_client_node("recorder", MediaRole::StreamInput, 6_001); + graph.link(hw, reader); + let other_a = graph.native_client_node("player-a", MediaRole::StreamOutput, 6_002); + let other_b = graph.native_client_node("player-b", MediaRole::StreamOutput, 6_003); + + assert_partition( + &run(&graph, &ctx()), + &[("player-a", other_a), ("player-b", other_b)], + &[("call", call, "peerspeak-owned")], + ); +} + +/// An **ambiguous** `client.id` — two live Clients claiming it, meaning the +/// observer missed a removal — must not yield a fallback pid. Inventing an +/// owner key is the one direction that can *reduce* taint, so resolving the +/// ambiguity by coin toss is the wrong kind of guess. +#[test] +fn an_ambiguous_client_id_yields_no_fallback_pid() { + 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); + let sunshine = graph.app_node("sunshine", MediaRole::StreamInput, 3_838); + graph.link(hw, sunshine); + + // Two Clients, one id, distinct real pids. + let shared_id = graph.client(Some(6_010)); + graph.client_with_id(shared_id, Some(6_011)); + let app = graph.node( + "native-app", + MediaRole::StreamOutput, + NodeProps { + client_id: Some(shared_id), + ..NodeProps::default() + }, + ); + graph.link(app, hw); + + // Unbounded ⇒ fails closed, exactly as before R10-3. + assert_partition( + &run(&graph, &ctx()), + &[], + &[ + ("call", call, "peerspeak-owned"), + ("native-app", app, "unresolved-owner"), + ], + ); +} + +/// 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). +#[test] +fn the_nodes_own_process_id_wins_over_its_clients() { + let mut graph = Graph::new(); + // `app_node` is exactly that shape: node pid 11_114, Client sec_pid + // PULSE_PID. If the Client's won, exception 1 would suppress key 4 and + // this node would be unbounded. + let hw = graph.device_node("hw-sink", MediaRole::Sink); + let call = graph.peerspeak_node("peerspeak", 7); + graph.link(call, hw); + let sunshine = graph.app_node("sunshine", MediaRole::StreamInput, 3_838); + graph.link(hw, sunshine); + let firefox = graph.app_node("firefox", MediaRole::StreamOutput, 11_114); + graph.link(firefox, hw); + + assert_partition( + &run(&graph, &ctx()), + &[("firefox", firefox)], + &[("call", call, "peerspeak-owned")], + ); +} + #[test] fn an_unknown_pipewire_pulse_pid_over_excludes_rather_than_leaks() { // v3.4 §6.1.2's failure-mode paragraph: if pixelpass cannot identify From 67f4ff931e275144bfa38d41d7152d025ec3d91f Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 20:59:17 -0400 Subject: [PATCH 06/11] host/observer: match the ownership carrier exactly, not leniently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lenient `truthy` spelling was wrong for this one property. Under it, `peerspeak.owned=""` and `peerspeak.owned="false "` both read as owned, so any process could suppress a rival application's audio from the share with a property it did not have to spell correctly. The justification for leniency was that treating an unexpected value as "owned" over-excludes and is therefore safe. That does not hold: leniency here buys false-positive exclusion, not safety. Fail-closed on this feature is about ancestry — an unresolvable graph is not eligible — not about parsing. The producer emits exactly PEERSPEAK_OWNED_VALUE at all three of its sites and is pinned to it by the shared cross-repo fixture, and a garbled property still leaves carrier 2's node.name prefix, which is a union with this one. `truthy` stays as it is for port.exclusive, port.monitor and node.passthrough: those are PipeWire's own, their spelling varies by producer, and each causes exclusion when true, so leniency really is the safe direction there. Both halves now have a row saying so. Codex phase-1 review F6. Round 10, R10-4. Mutation-verified. Co-Authored-By: Claude Opus 5 --- src/host/observer/adapter.rs | 75 +++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/src/host/observer/adapter.rs b/src/host/observer/adapter.rs index 387bb26..501e5b6 100644 --- a/src/host/observer/adapter.rs +++ b/src/host/observer/adapter.rs @@ -9,10 +9,10 @@ use super::{ EventKind, LinkEndpoints, NodeObservation, Outcome, Projection, RegEvent, RegistryModel, }; use crate::host::audio::parse_object_serial; -use crate::host::taint::PEERSPEAK_OWNED_PROP; use crate::host::taint::snapshot::{ ClientSnapshot, GlobalId, MediaRole, NodeProps, PortDirection, PortSnapshot, Serial, }; +use crate::host::taint::{PEERSPEAK_OWNED_PROP, PEERSPEAK_OWNED_VALUE}; use anyhow::{Context, Result}; use pipewire::{self as pw, types::ObjectType}; use std::cell::{Cell, RefCell}; @@ -649,10 +649,34 @@ fn parse_serial(id: u32, kind: &str, raw: Option<&str>) -> Option { } } +/// Lenient boolean for PipeWire's own `bool`-ish properties +/// (`port.exclusive`, `port.monitor`, `node.passthrough`), whose spelling +/// varies by producer. Leniency is the fail-closed direction *for these*: +/// each one, when true, causes exclusion. fn truthy(value: Option<&str>) -> bool { value.is_some_and(|value| value != "false" && value != "0") } +/// The ownership carrier is matched **exactly**, not leniently (round 10, +/// R10-4). +/// +/// It is tempting to reuse [`truthy`] here on the grounds that treating an +/// unexpected value as "owned" over-excludes and is therefore safe. That +/// argument does not hold: leniency buys false-positive *exclusion*, not +/// safety. Under `truthy`, `peerspeak.owned=""` and `peerspeak.owned=false ` +/// (trailing space) both mean owned, so any process can suppress a rival's +/// audio from the share with a property it does not even have to spell right. +/// +/// Fail-closed on this feature is about **ancestry** — an unresolvable graph +/// is not eligible — not about parsing. The producer emits exactly +/// [`PEERSPEAK_OWNED_VALUE`] at all three of its sites and is pinned to it by +/// the shared cross-repo fixture, so there is no real value to be lenient +/// about. And a missed tag is not silent: carrier 2 is a union with this one, +/// so a garbled property still leaves the `node.name` prefix. +fn peerspeak_owned(value: Option<&str>) -> bool { + value == Some(PEERSPEAK_OWNED_VALUE) +} + fn node_observation_from_props(props: &pw::spa::utils::dict::DictRef) -> NodeObservation { NodeObservation { name: props.get("node.name").map(str::to_string), @@ -662,7 +686,7 @@ fn node_observation_from_props(props: &pw::spa::utils::dict::DictRef) -> NodeObs // in the engine off `NodeObservation::name` above, so each // carrier stays independently testable — see // [`crate::host::taint::PEERSPEAK_OWNED_NODE_PREFIX`]. - peerspeak_owned: truthy(props.get(PEERSPEAK_OWNED_PROP)), + peerspeak_owned: peerspeak_owned(props.get(PEERSPEAK_OWNED_PROP)), pulse_module_id: props .get("pulse.module.id") .and_then(|value| value.parse::().ok()), @@ -722,6 +746,53 @@ mod tests { use super::*; use std::process::Command; + /// **R10-4.** The ownership carrier is matched exactly; the lenient + /// [`truthy`] spelling is wrong for it. + /// + /// Under `truthy`, every value in `denied` below meant "peerspeak owns + /// this" — including the empty string and a `false` with a trailing space + /// — so any process could suppress a rival application's audio from the + /// share with a property it did not have to spell correctly. Leniency here + /// buys false-positive exclusion, not safety. + #[test] + fn the_ownership_carrier_is_matched_exactly_not_leniently() { + assert!(peerspeak_owned(Some(PEERSPEAK_OWNED_VALUE))); + + let denied = [ + None, + Some(""), + Some("false"), + Some("0"), + Some("false "), + Some("true"), + Some("yes"), + Some("1 "), + Some(" 1"), + Some("01"), + Some("2"), + ]; + for value in denied { + assert!( + !peerspeak_owned(value), + "{value:?} must not read as peerspeak-owned" + ); + } + } + + /// 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 + /// an unrecognised value as true is genuinely the safe direction for them. + #[test] + fn pipewires_own_boolean_props_stay_lenient() { + assert!(truthy(Some("true"))); + assert!(truthy(Some("1"))); + assert!(truthy(Some(""))); + assert!(!truthy(Some("false"))); + assert!(!truthy(Some("0"))); + assert!(!truthy(None)); + } + struct PactlModule { id: Option, } From abaf5d9c1056897c3a4d33e35a0e0bce4f6e1376 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 21:07:42 -0400 Subject: [PATCH 07/11] host/taint: a pid-less Client still makes its id ambiguous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification round on R10-3's own fix. The ambiguity guard detected a duplicate client id by looking it up in the pid map — which is only populated for Clients that carry a sec_pid at all. A pid-less first claimant therefore left no trace, so the next Client claiming the same id looked unique and its pid was used, resolving an ambiguous id: exactly the guess the guard exists to refuse. Reachable, not theoretical — pid-less Clients are ordinary here (the session manager's is one). Reproduced: the bystander app went eligible off a coin-toss owner attribution. Claimed ids are now tracked separately from resolved pids. Co-Authored-By: Claude Opus 5 --- src/host/taint/owner.rs | 12 +++++++++--- src/host/taint/tests.rs | 42 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/host/taint/owner.rs b/src/host/taint/owner.rs index d0cbd7b..579d27f 100644 --- a/src/host/taint/owner.rs +++ b/src/host/taint/owner.rs @@ -100,13 +100,19 @@ pub struct OwnerCtx { impl OwnerCtx { pub fn new(snapshot: &GraphSnapshot, pipewire_pulse_pid: Option) -> Self { let mut client_pids: BTreeMap = BTreeMap::new(); - let mut ambiguous: BTreeSet = BTreeSet::new(); + // ⚠️ Tracked separately from `client_pids`, and that is the point: a + // Client with no `sec_pid` still *claims* its id. Detecting duplicates + // by looking in the pid map would let a pid-less first claimant leave + // no trace, so the next Client claiming the same id would look unique + // and its pid would be used — resolving an ambiguous id, which is the + // one guess this guard exists to refuse. Pid-less Clients are ordinary + // (the session manager's is one). + let mut seen: BTreeSet = BTreeSet::new(); for client in snapshot.clients() { - if client_pids.contains_key(&client.id) || ambiguous.contains(&client.id) { + if !seen.insert(client.id) { // Two Clients claiming one id: drop it entirely rather than // pick. See the field docs. client_pids.remove(&client.id); - ambiguous.insert(client.id); continue; } if let Some(pid) = client.sec_pid { diff --git a/src/host/taint/tests.rs b/src/host/taint/tests.rs index c8e57f5..ef883de 100644 --- a/src/host/taint/tests.rs +++ b/src/host/taint/tests.rs @@ -949,6 +949,48 @@ fn an_ambiguous_client_id_yields_no_fallback_pid() { ); } +/// The ambiguity guard must not depend on the *first* Client claiming an id +/// having a `sec_pid`. +/// +/// Found by auditing R10-3 rather than by a failing case: the first cut +/// detected a duplicate id by looking it up in the pid map, which is only +/// populated for Clients that carry a pid at all. A pid-less Client therefore +/// left no trace, and the next Client claiming the same id was treated as +/// unique — resolving an ambiguous id, which is exactly the guess the guard +/// exists to refuse. Pid-less Clients are ordinary here (`device_node`'s +/// session client is one), so this is reachable, not theoretical. +#[test] +fn a_pidless_first_client_still_makes_its_id_ambiguous() { + 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); + let sunshine = graph.app_node("sunshine", MediaRole::StreamInput, 3_838); + graph.link(hw, sunshine); + + // First claimant has NO sec_pid; second has one. + let shared_id = graph.client(None); + graph.client_with_id(shared_id, Some(6_011)); + let app = graph.node( + "native-app", + MediaRole::StreamOutput, + NodeProps { + client_id: Some(shared_id), + ..NodeProps::default() + }, + ); + graph.link(app, hw); + + assert_partition( + &run(&graph, &ctx()), + &[], + &[ + ("call", call, "peerspeak-owned"), + ("native-app", app, "unresolved-owner"), + ], + ); +} + /// 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 From 993befdedd65ce416012c262dd510b2aaba5be29 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 23:57:09 -0400 Subject: [PATCH 08/11] host/taint: carry both pids as owner keys; gate the wiring and the contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 10 review, findings 1, 4 and 6. Finding 1 (P1, phase 6) — key 4 was `node.or_else(client)`, so a node's client-controlled application.process.id REPLACED its Client's protected pipewire.sec.pid. One process using two Clients could therefore split its identity: the tainted reader reports a bogus node pid, the output leg omits the node pid and falls back to the Client's real one, the legs are bounded by different values, and they neither bridge nor trip the unbounded sweep — the output stays eligible while re-emitting the call. Now a union of both values, deduplicated, with exception 1 applied to each independently so the pipewire-pulse pid still cannot fuse unrelated Clients. Mutation-verified: reverting to or_else fails ONLY the new split-Client test (so the union changes nothing else), dropping exception 1 fails 32 rows, and using the Client pid alone fails 16. Not reachable today — evaluate() is reached only by the dry-run audit, which creates no links. It becomes live when phase 6 consumes these decisions. Finding 4 — R10-4's test called peerspeak_owned() directly, so reverting node_observation_from_props to truthy() left it green; the only case it shared with production, exact "1", passes under both. A new test builds a real pw_properties dict and drives the production wiring, and the mutation now fails exactly that test while the helper test still passes. Finding 6 — the cross-repo fixture still documented carrier 1 as "any value other than false/0", which R10-4 made exact-"1". A producer following it could emit "true" and silently lose the carrier. Fixture updated in both repos (byte-identical, verified), along with the stale prose in taint/tests and snapshot.rs, and the contract is now also exercised through the production adapter rather than only against the constants. Co-Authored-By: Claude Opus 5 --- src/host/observer/adapter.rs | 112 ++++++++++++++++++++++ src/host/taint/owner.rs | 60 ++++++++---- src/host/taint/snapshot.rs | 7 +- src/host/taint/tests.rs | 98 ++++++++++++++++--- tests/fixtures/ownership-tag-contract.txt | 14 ++- 5 files changed, 254 insertions(+), 37 deletions(-) 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 From 4b2b192601569233419433f4a7e059ffacde9b9e Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 00:00:38 -0400 Subject: [PATCH 09/11] host/taint: correct the constant's own doc, and record the ProcessId ambiguity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification round on the round-10 review fixes. Finding 6 named the fixture, taint/tests.rs and snapshot.rs, but the same stale claim was also on PEERSPEAK_OWNED_VALUE itself — the definition site for the very literal the finding was about, still arguing that any truthy value counts and that this is the fail-closed direction. Corrected with the reason the argument fails. Also records a known imprecision the union widened: OwnerKey::ProcessId now covers both application.process.id and the Client's pipewire.sec.pid, so a bridge reported under the former may have resolved on the latter. Pre-existing since R10-3; not fixed here because these codes are a stable contract for the audit output and the phase 6 status event, so splitting one wants its own decision. Co-Authored-By: Claude Opus 5 --- src/host/taint/mod.rs | 18 ++++++++++++------ src/host/taint/owner.rs | 10 ++++++++++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/host/taint/mod.rs b/src/host/taint/mod.rs index 76a8b45..a9e1054 100644 --- a/src/host/taint/mod.rs +++ b/src/host/taint/mod.rs @@ -136,13 +136,19 @@ pub const ECHO_CANCEL_GROUP_PREFIX: &str = "echo-cancel-"; /// change that invalidates the phase 5 matrix. pub const PEERSPEAK_OWNED_PROP: &str = "peerspeak.owned"; -/// The value peerspeak emits for [`PEERSPEAK_OWNED_PROP`]. +/// The value peerspeak emits for [`PEERSPEAK_OWNED_PROP`], and the **only** +/// value this consumer reads as owned. /// -/// The consumer deliberately accepts **any** truthy value, not just this one -/// (see the observer's `truthy`) — treating an unexpected value as "owned" is -/// the fail-closed direction. This constant exists so live tests can tag a -/// node with exactly what the producer sends, rather than something merely -/// truthy that would pass even if the real literal did not. +/// ⚠️ This doc used to say the opposite — that any truthy value counted, on +/// the theory that treating an unexpected value as "owned" is the fail-closed +/// direction. R10-4 removed that leniency and the round-10 review caught the +/// prose surviving it here and in the shared fixture. The theory is wrong: +/// leniency buys false-positive *exclusion*, not safety, and it let any +/// process suppress a rival application's audio from the share with a +/// property it did not have to spell right. Fail-closed on this feature is +/// about **ancestry** — an unresolvable graph is not eligible — not about +/// parsing. The matching lives in the observer's `peerspeak_owned`, which is +/// deliberately *not* the lenient `truthy` used for PipeWire's own booleans. pub const PEERSPEAK_OWNED_VALUE: &str = "1"; /// Ownership carrier 2: a `node.name` prefix (v3.5 §5.1, round 8). diff --git a/src/host/taint/owner.rs b/src/host/taint/owner.rs index 4727dd5..7e71fd4 100644 --- a/src/host/taint/owner.rs +++ b/src/host/taint/owner.rs @@ -145,6 +145,16 @@ pub enum OwnerKey { impl OwnerKey { /// Stable, machine-readable — this ends up in the phase 5 audit output /// and the phase 6 status event. + /// + /// ⚠️ **Known imprecision, deliberately not fixed here.** `ProcessId` now + /// covers two sources — the node's `application.process.id` and its + /// Client's `pipewire.sec.pid` (see [`keys_of`]) — so a bridge reported as + /// `application.process.id` may in fact have resolved on the Client's + /// protected pid. Pre-existing since R10-3 made the Client a fallback, and + /// widened by the review's finding 1 making it a union. Splitting it would + /// add a code to a set that is explicitly a stable contract for the audit + /// output and the "why isn't this app being shared?" answer, so it wants + /// its own decision rather than a drive-by. pub fn code(self) -> &'static str { match self { Self::LinkGroup => "node.link-group", From 289016d071558ad0a3d16570e1e9744cef74b2ca Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 01:09:17 -0400 Subject: [PATCH 10/11] host/taint: record the open owner-union/boundedness interaction Round 11 review, finding 1. Verified correct: round 10's claim that the key union was "strictly additive" was too strong. The same key list feeds owner_is_bounded, and the unresolved-owner sweep is triggered by an UNbounded tainted reader -- so adding the Client's PID can move a reader from unbounded to bounded and switch the sweep off, letting a same-process output leg with an ambiguous Client and a bogus self-claimed PID stay eligible. Cannot leak today (evaluate() is audit-only); becomes live in phase 6. Not fixed in this round, and the doc says why: the blunt repair -- only protected keys bound an owner -- makes every Pulse-emulated app unbounded, which re-triggers the mass over-exclusion the design exists to avoid and would empty the eligible half of the 5.1 matrix. The targeted rule (a node whose Client cannot be resolved at all is not bounded by its own self-claimed PID) is written down along with what it needs structurally, to be implemented with matrix data in hand rather than argued from a whiteboard. Co-Authored-By: Claude Opus 5 --- src/host/taint/owner.rs | 44 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/host/taint/owner.rs b/src/host/taint/owner.rs index 7e71fd4..ac7d0eb 100644 --- a/src/host/taint/owner.rs +++ b/src/host/taint/owner.rs @@ -256,6 +256,50 @@ fn keys_of(node: &NodeSnapshot, ctx: &OwnerCtx) -> Vec<(OwnerKey, KeyValue)> { /// nothing else relates them. Its sibling output leg cannot be found, so /// the engine must fail closed rather than declare it clean /// (v3.4 §6.1.1, final paragraph). +/// +/// # 🔴 OPEN, phase-6 blocking — the key union can *reduce* taint here +/// +/// **Round 11 review, finding 1. Verified correct; deliberately not fixed in +/// that round.** Round 10 made key 4 a union of the node's +/// `application.process.id` and its Client's `pipewire.sec.pid`, and the claim +/// that this was "strictly additive" was too strong: the same key list also +/// feeds *this* predicate, so adding a value can move a node from unbounded to +/// bounded, and `propagate_unresolved_owner`'s global sweep is triggered by an +/// **un**bounded tainted reader. Concretely: +/// +/// 1. A tainted reader's node claims the pipewire-pulse PID while its Client +/// holds a real protected PID `A`. Under `or_else` the node's value won and +/// exception 1 suppressed it, leaving the reader unbounded; under the union +/// it is bounded by `A`. +/// 2. Its process's output leg uses a second Client whose id is **ambiguous** +/// (the observer missed a removal), so no protected PID is available — but +/// the leg claims a bogus `application.process.id` `B`, which bounds it. +/// 3. Neither the bridge nor the sweep fires, and the output stays eligible +/// while re-emitting the call. +/// +/// It cannot leak today: `evaluate()` is reached only by the dry-run audit, +/// which creates no links. It becomes live when phase 6 consumes eligibility. +/// +/// **Why it is not fixed yet.** The principled repair is provenance: a +/// self-claimed `application.process.id` is not a *sound* bound, only the +/// protected keys are. But applying that bluntly makes every Pulse-emulated +/// app unbounded — their Client's `sec_pid` is the daemon's and suppressed, so +/// the node's own claim is their only per-app identity — which re-triggers the +/// §6.1.1 mass over-exclusion the whole design is built to avoid, and would +/// make the eligible half of the §5.1 matrix empty. +/// +/// The targeted rule that closes the path above without that cost: **a node +/// whose Client cannot be resolved at all must not be bounded by its own +/// self-claimed PID.** An ambiguous Client already means "we do not know who +/// owns this", and a self-claim must not paper over it; a Pulse app's Client +/// *is* resolved (to the daemon's PID, then suppressed), so it keeps its +/// bound. Implementing it needs `OwnerCtx` to distinguish "resolved" from +/// "absent", and `OwnerKeyIndex` to carry boundedness separately from the key +/// set, since bridging must keep using the full union. +/// +/// ⚠️ Do this **with the §5.1 matrix data in hand**, not before: the whole +/// question is how much over-exclusion the rule actually causes on a real +/// graph, and that is measurable rather than arguable. pub fn owner_is_bounded(node: &NodeSnapshot, ctx: &OwnerCtx) -> bool { keys_of(node, ctx) .iter() From b8b8b78b0924a4db59119cccd0d31519a8b2191f Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 02:01:07 -0400 Subject: [PATCH 11/11] host/taint: pin what "resolved" must mean before F11-1 is implemented MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 12 re-examined the deferral and agreed it holds while evaluate() is audit-only, and that the recorded rule closes the path without unbounding Pulse-emulated apps — but only under one reading of "a node whose Client cannot be resolved at all". The trap is worth writing down before anyone implements it: reading "resolved" as "a unique Client object exists" passes for a unique Client with sec_pid = None, which supplies no protected identity and leaves exactly the self-claimed-PID hole the rule exists to close. It has to mean an unambiguous Client yielding Some(pipewire.sec.pid), taken before pipewire-pulse suppression. That also means the §5.1 matrix needs five Client cases rather than two: absent, ambiguous, unique-but-pid-less, resolved-native, and resolved-to-pipewire-pulse. The pid-less row is the one that distinguishes the two readings and the one a two-case matrix skips without saying so. Docs only. Still deferred, still to be decided with matrix data in hand. 220 tests green, clippy clean. Co-Authored-By: Claude Opus 5 --- src/host/taint/owner.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/host/taint/owner.rs b/src/host/taint/owner.rs index ac7d0eb..464d4d0 100644 --- a/src/host/taint/owner.rs +++ b/src/host/taint/owner.rs @@ -300,6 +300,28 @@ fn keys_of(node: &NodeSnapshot, ctx: &OwnerCtx) -> Vec<(OwnerKey, KeyValue)> { /// ⚠️ Do this **with the §5.1 matrix data in hand**, not before: the whole /// question is how much over-exclusion the rule actually causes on a real /// graph, and that is measurable rather than arguable. +/// +/// ## Round 12 — the deferral holds, and "resolved" has a trap in it +/// +/// Codex re-examined this and agreed the deferral is defensible while +/// `evaluate()` is audit-only, and that the rule above closes the recorded path +/// without unbounding normal Pulse-emulated apps — **but only under one +/// reading of "resolves"**, and the wrong reading reintroduces the hole: +/// +/// - ✅ "Resolved" must mean **an unambiguous Client that yields +/// `Some(pipewire.sec.pid)`**, taken *before* the pipewire-pulse suppression +/// step. A Pulse app then still has the daemon's protected PID as +/// provenance, even though that value is omitted from the bridge keys, so it +/// stays bounded and the eligible half survives. +/// - ❌ **Do not** implement it as "a unique Client object exists". A unique +/// Client with `sec_pid = None` would satisfy that test while providing no +/// protected identity at all, leaving exactly the self-claimed-PID hole this +/// rule is meant to close. +/// +/// So the matrix needs five Client cases, not two: **absent**, **ambiguous**, +/// **unique but pid-less**, **resolved-native**, and +/// **resolved-to-pipewire-pulse**. The third is the one that distinguishes the +/// two readings, and it is the row a two-case matrix would silently skip. pub fn owner_is_bounded(node: &NodeSnapshot, ctx: &OwnerCtx) -> bool { keys_of(node, ctx) .iter()