From 28973350163750bb954ee7064524be696eda5e9e Mon Sep 17 00:00:00 2001 From: Breadway Date: Mon, 31 Aug 2026 15:37:52 +0800 Subject: [PATCH] bread-polkit: only auth as an identity polkit actually offered; fail fast without the lock - The agent prompt's username field is user-editable. It was passed straight to `auth::authenticate`, so a request scoped to specific accounts (e.g. root only) could have its PAM conversation redirected to any local user. `resolve_user` now accepts only the `unix-user` identities from `BeginAuthentication` (empty = the prefilled default); anything else re-shows the prompt with an explanation. PAM still has to clear polkit's own authorization, but this closes the foot-gun at the one place the identity list is known. - A failed single-instance lock now exits(1) instead of continuing: a second agent would `serve_at` the same object path, and a prompt held by a process that couldn't take the lock is ambiguous state. --- bread-polkit/src/agent.rs | 82 ++++++++++++++++++++++++++++++++++++--- bread-polkit/src/main.rs | 9 ++++- 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/bread-polkit/src/agent.rs b/bread-polkit/src/agent.rs index 94840eb..27cda84 100644 --- a/bread-polkit/src/agent.rs +++ b/bread-polkit/src/agent.rs @@ -90,6 +90,7 @@ impl Agent { let username = pick_user(&users, current_uid()) .map(|u| u.name.clone()) .ok_or_else(|| AgentError::Failed("no unix-user identity".into()))?; + let allowed_users: Vec = users.iter().map(|u| u.name.clone()).collect(); let (tx, mut rx) = mpsc::channel(4); *self.pending.lock().await = Some(tx.clone()); @@ -107,7 +108,7 @@ impl Agent { } }); - let result = self.drive_prompt(&cookie, &username, &mut rx).await; + let result = self.drive_prompt(&cookie, &username, &allowed_users, &mut rx).await; *self.pending.lock().await = None; let cookie_close = cookie.clone(); @@ -129,6 +130,7 @@ impl Agent { &self, cookie: &str, default_user: &str, + allowed_users: &[String], rx: &mut mpsc::Receiver, ) -> Result<(), AgentError> { loop { @@ -140,12 +142,17 @@ impl Agent { return Err(AgentError::Cancelled("user cancelled".into())); } Some(UserAction::Submit { username, password }) => { - let user = if username.is_empty() { - default_user - } else { - username.as_str() + // The username field is user-editable; only accept it when + // it's one of the identities polkit offered (empty falls + // back to the prefilled user). Anything else is rejected + // and the prompt re-shown rather than starting a PAM + // conversation for an account the request never offered. + let Some(user) = resolve_user(default_user, allowed_users, &username) else { + let cookie = cookie.to_string(); + invoke_ui(move || ui::show_retry(&cookie, INVALID_USER_MESSAGE)); + continue; }; - match auth::authenticate(&self.transport, user, cookie, &password).await { + match auth::authenticate(&self.transport, &user, cookie, &password).await { Ok(Outcome::Success) => return Ok(()), Ok(Outcome::Failure { message }) => { let text = message @@ -166,6 +173,30 @@ impl Agent { } } +const INVALID_USER_MESSAGE: &str = + "That user is not one of the identities this request offered — use the prefilled user."; + +/// Choose the username to authenticate as from the prompt input. +/// +/// Empty input falls back to `default`. Non-empty input must be one of +/// `allowed` — the `unix-user` identities polkit actually offered in +/// `BeginAuthentication` — otherwise it returns `None`. Without this, a +/// request scoped to specific accounts (say, `root` only) could have its +/// editable username field redirected to an arbitrary local user, kicking +/// off a PAM conversation for an account the request never offered. +/// (`auth::authenticate`'s PAM result still has to clear polkit's own +/// authorization, but this closes the obvious foot-gun at the agent — the +/// one place the identity list is actually known.) +fn resolve_user(default: &str, allowed: &[String], input: &str) -> Option { + if input.is_empty() { + return Some(default.to_string()); + } + if allowed.iter().any(|u| u.as_str() == input) { + return Some(input.to_string()); + } + None +} + fn unix_users(identities: &[Identity]) -> Vec { let mut uids = Vec::new(); for identity in identities { @@ -298,3 +329,42 @@ async fn run() -> Result<()> { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn allowed() -> Vec { + vec!["root".to_string(), "1000".to_string()] + } + + #[test] + fn resolve_user_falls_back_to_default_on_empty_input() { + assert_eq!( + resolve_user("root", &allowed(), ""), + Some("root".to_string()) + ); + } + + #[test] + fn resolve_user_accepts_an_offered_identity() { + assert_eq!( + resolve_user("root", &allowed(), "1000"), + Some("1000".to_string()) + ); + } + + #[test] + fn resolve_user_rejects_a_user_polkit_did_not_offer() { + assert_eq!(resolve_user("root", &allowed(), "alice"), None); + assert_eq!(resolve_user("root", &allowed(), "daemon"), None); + } + + #[test] + fn resolve_user_is_case_exact() { + // Usernames are case-significant; "ROOT" is a different principle + // than the offered "root", so it must be rejected. + assert_eq!(resolve_user("root", &allowed(), "ROOT"), None); + } +} + diff --git a/bread-polkit/src/main.rs b/bread-polkit/src/main.rs index ca28048..4018b41 100644 --- a/bread-polkit/src/main.rs +++ b/bread-polkit/src/main.rs @@ -50,8 +50,13 @@ fn main() { std::process::exit(0); } Err(e) => { - eprintln!("bread-polkit: singleton lock unavailable ({e}); continuing"); - None + // Don't keep running without the single-instance lock: a second + // copy would attempt to `serve_at` the same PolicyKit agent + // object path on the system bus, and a password prompt held by a + // process whose lock couldn't be taken is ambiguous state. Fail + // fast and let a wrapper/autostart retry. + eprintln!("bread-polkit: singleton lock unavailable ({e}); exiting"); + std::process::exit(1); } };