host/taint: carry both pids as owner keys; gate the wiring and the contract

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 <noreply@anthropic.com>
This commit is contained in:
2026-07-25 23:57:09 -04:00
co-authored by Claude Opus 5
parent abaf5d9c10
commit 993befdedd
5 changed files with 254 additions and 37 deletions
+112
View File
@@ -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 /// The other three boolean properties keep the lenient spelling, and that
/// is deliberate rather than an oversight: each is PipeWire's own, each /// is deliberate rather than an oversight: each is PipeWire's own, each
/// varies by producer, and each causes *exclusion* when true — so reading /// varies by producer, and each causes *exclusion* when true — so reading
+41 -19
View File
@@ -183,26 +183,48 @@ fn keys_of(node: &NodeSnapshot, ctx: &OwnerCtx) -> Vec<(OwnerKey, KeyValue)> {
if let Some(client) = node.props.client_id { if let Some(client) = node.props.client_id {
out.push((OwnerKey::ClientId, KeyValue::Num(u64::from(client.0)))); out.push((OwnerKey::ClientId, KeyValue::Num(u64::from(client.0))));
} }
// Key 4, from the node if it has one, otherwise from its Client // Key 4, from the node **and** from its Client (round 10, R10-3; made a
// (round 10, R10-3). The node's own property wins when both exist: it is // union rather than a fallback by the round-10 review, finding 1).
// 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 // ⚠️ **A union, not `node.or_else(client)`, and the difference is a leak.**
// risk of the fallback.** Measured on this host: 15 unrelated Clients // The node's `application.process.id` is client-controlled and optional;
// share `sec_pid` 2528, which is pipewire-pulse's own — every // the Client's `pipewire.sec.pid` is `pipewire.*`, protected, and the only
// Pulse-emulated app has one. Adding key 4 unguarded would fuse all // one that can carry a soundness argument (the same reason
// fifteen into a single owner and bridge taint between completely // `propagate_unresolved_owner` sweeps everything for an unbounded reader).
// unrelated applications, which is enormous over-exclusion. Guarded, the // Letting the node's value *replace* the Client's meant one process using
// fallback strictly *adds* correct bounding: it fires only for native // two Clients could escape the bridge entirely: its tainted reader reports
// clients, which are exactly the ones that carry a real per-app pid here. // a bogus node pid, its output leg omits the node pid and falls back to
if let Some(pid) = node.props.process_id.or_else(|| ctx.client_pid(node)) { // the Client's real one, the two legs are bounded by different values, so
// Note the fail-closed asymmetry when the daemon PID is unknown // they neither bridge nor trip the unbounded sweep — and the output stays
// (`None`): the exception does *not* fire, key 4 applies to // eligible while re-emitting the call. Carrying both values costs nothing
// everything, and Pulse modules fuse into one owner. That is broad // and closes it: a leg that presents *either* value bridges.
// over-exclusion — annoying and safe — which is the direction v3.4 //
// §6.1.2's failure-mode paragraph asks for. // ⚠️ **Exception 1 applies to each value independently, and that is the
if Some(pid) != ctx.pipewire_pulse_pid { // whole risk here.** Measured on this host: 15 unrelated Clients share
out.push((OwnerKey::ProcessId, KeyValue::Num(u64::from(pid)))); // `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 out
+5 -2
View File
@@ -111,8 +111,11 @@ impl MediaRole {
/// on this feature means "not tainted". /// on this feature means "not tainted".
#[derive(Clone, Debug, Default, PartialEq, Eq)] #[derive(Clone, Debug, Default, PartialEq, Eq)]
pub struct NodeProps { pub struct NodeProps {
/// `peerspeak.owned` is present and truthy (v3.4 §5.1). A correctness /// `peerspeak.owned` is present and **exactly**
/// mechanism, explicitly *not* a security boundary. /// [`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 /// ⚠️ **Ownership carrier 1 of 2, so this being `false` does not mean
/// "not peerspeak's".** Carrier 2 is the [`NodeSnapshot::name`] prefix /// "not peerspeak's".** Carrier 2 is the [`NodeSnapshot::name`] prefix
+85 -13
View File
@@ -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_PROP, get("prop_key"));
assert_eq!(super::PEERSPEAK_OWNED_NODE_PREFIX, get("node_name_prefix")); 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")); 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, // And the fixture's own worked example must be one this engine excludes,
// through carrier 2, exactly as written in the shared file. // through carrier 2, exactly as written in the shared file.
let mut graph = Graph::new(); 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 process using **two** Clients cannot escape the bridge by presenting a
/// a direct statement about the node; the Client's is a one-hop inference, and /// bogus pid on one leg and none on the other.
/// 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). /// ⚠️ **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] #[test]
fn the_nodes_own_process_id_wins_over_its_clients() { fn the_nodes_own_process_id_wins_over_its_clients() {
let mut graph = Graph::new(); let mut graph = Graph::new();
+11 -3
View File
@@ -20,9 +20,17 @@
# PipeWire registry `global` event and readable only via a node bind, so 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. # primary taint root must not rest on one observation mechanism alone.
# Carrier 1 — a node property. The consumer treats any value other than # Carrier 1 — a node property, matched EXACTLY: `prop_value` below is the
# "false"/"0" as truthy, which is the fail-closed direction; the producer # ONLY spelling the consumer reads as owned. A producer emitting "true", "yes"
# always emits exactly this value. # 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_key=peerspeak.owned
prop_value=1 prop_value=1