Skip to main content

max / alloy

Close the helper out however the conversation ends Found by the security review db8d4e07 and the wiki both ask for before tier 3 ships, and which had not been run. converse has six places it can leave by `?` -- stdin, stdout, the cookie write, each line read, the answer write, and the `ask` that hands the question to the screen -- and std::process::Child implements no Drop, so a helper let go of on any of them is neither killed nor reaped. Eight deliberate exits called finish(); those six did not. The `ask` path is not hypothetical. It is the one Agent::drop creates on purpose: closing the prompt receiver turns a parked send into an error so the conversation ends rather than holding the console open. So the case is "the console is quitting" and what it left behind was a setuid helper sitting inside pam_authenticate for a console that no longer exists. Measured rather than reasoned about. The new test drives converse with an ask that fails, against a script that would outlive it, and reads this process's children out of /proc: it failed with one helper left behind, and passes now. A guard replaces the eight hand-placed calls, so the invariant is structural instead of remembered. Two more from the same pass, neither reachable today and both stated where they are relied on, which is what this file does everywhere else. The cookie is now refused if it carries a newline, the same check the answer already had and for the same reason: the helper's protocol is one line per message, so a newline in the cookie makes everything after it the response to the first PAM prompt. And the Listener now records why nothing here checks the D-Bus caller -- system.conf denies method calls by default and polkit's own policy file opens that interface to user polkitd alone, both measured -- plus the two ways that protection would disappear, the session bus being one of them.
Author: Max Johnson <me@maxj.phd> · 2026-08-23 00:25 UTC
Signed with PGP, not checked
Commit: fcd988b48648c9df1b3f34dfded2bbedcc66314f
Parent: 54f14f8
1 file changed, +169 insertions, -30 deletions
M crates/alloy/src/polkit.rs +169 -30
@@ -351,6 +351,37 @@
351 351 }
352 352
353 353 /// The object polkit calls into.
354 + ///
355 + /// **Nothing here checks who is calling, and that is the bus's job rather than
356 + /// an omission.** Worth stating because it is the one load-bearing invariant in
357 + /// this file that lives entirely in somebody else's configuration, and because
358 + /// the consequence of losing it is severe: a caller that could reach
359 + /// [`begin_authentication`](Listener::begin_authentication) could put a message
360 + /// of its own choosing in front of the user as an Akari modal, and read back
361 + /// from the D-Bus reply whether the password typed into it was accepted. A
362 + /// prompt-spoofing oracle, in other words, in the one screen the console asks
363 + /// people to trust with a password.
364 + ///
365 + /// Measured on fw13 2026-08-22 rather than assumed, in two files that ship with
366 + /// dbus and polkit:
367 + ///
368 + /// - `/usr/share/dbus-1/system.conf`, default policy: `<deny
369 + /// send_type="method_call"/>`. On the system bus a method call is refused
370 + /// unless some policy allows it, which is the opposite of the session bus.
371 + /// - `/usr/share/dbus-1/system.d/org.freedesktop.PolicyKit1.conf`: `<policy
372 + /// user="polkitd"> <allow send_interface="org.freedesktop.PolicyKit1.AuthenticationAgent"/>`.
373 + /// That is the only hole punched for this interface, and it names one user.
374 + ///
375 + /// So the agent is unreachable by anything but polkitd, and adding a sender
376 + /// check here would be re-implementing a check the bus has already made with
377 + /// better information than this process has.
378 + ///
379 + /// Two ways that stops being true, both worth recognising on sight. Serving
380 + /// this object on the **session** bus would remove the protection entirely,
381 + /// since the session bus allows method calls by default — the test at the
382 + /// bottom of this file does exactly that, deliberately, and is why it drives
383 + /// [`Listener`] directly rather than through a registered agent. And a distro
384 + /// that shipped a wider policy for this interface would too.
354 385 struct Listener {
355 386 helper: PathBuf,
356 387 sender: SyncSender<Prompt>,
@@ -719,25 +750,52 @@
719 750 withdrawn: &Arc<AtomicBool>,
720 751 ask: impl Fn(Prompt) -> Result<()>,
721 752 ) -> Result<()> {
722 - let mut child = child_command(helper)
723 - .arg(user)
724 - .stdin(Stdio::piped())
725 - .stdout(Stdio::piped())
726 - .stderr(Stdio::null())
727 - .spawn()
728 - .with_context(|| format!("could not run {}", helper.display()))?;
753 + // Wrapped in the guard on the same expression that spawns it, and that is
754 + // load-bearing rather than style. Everything below this line can leave by
755 + // `?`, and `std::process::Child` has no `Drop`: a helper let go of that way
756 + // is neither killed nor reaped. Measured, not reasoned about — see
757 + // `a_question_nobody_can_receive_still_closes_the_helper`, which failed
758 + // with one process left behind before this existed.
759 + let mut child = Helper(
760 + child_command(helper)
761 + .arg(user)
762 + .stdin(Stdio::piped())
763 + .stdout(Stdio::piped())
764 + .stderr(Stdio::null())
765 + .spawn()
766 + .with_context(|| format!("could not run {}", helper.display()))?,
767 + );
729 768
730 - let mut stdin = child.stdin.take().context("the helper took no stdin")?;
731 - let stdout = child.stdout.take().context("the helper wrote no stdout")?;
769 + let mut stdin = child.0.stdin.take().context("the helper took no stdin")?;
770 + let stdout = child
771 + .0
772 + .stdout
773 + .take()
774 + .context("the helper wrote no stdout")?;
732 775 let mut lines = BufReader::new(stdout).lines();
733 776
734 777 // The cookie first, on its own line. It is what polkit gave us and what the
735 778 // helper hands back to prove this conversation is the one polkit asked for.
779 + //
780 + // Checked for a newline for exactly the reason the answer is, further down,
781 + // and it was the half that was not. The helper's protocol is one line per
782 + // message, so a cookie carrying a newline ends the line early and everything
783 + // after it is read as the *next* message — which at that point in the
784 + // exchange is the response to the first PAM prompt. That is a password
785 + // supplied by the caller without anyone being asked.
786 + //
787 + // Unreachable today and stated anyway, which is this file's habit
788 + // everywhere else: polkitd generates the cookie, the bus lets nobody else
789 + // call this interface (see [`Listener`]), and both of those are somebody
790 + // else's file rather than an invariant this code holds. The same argument
791 + // the `printable` calls are made under.
792 + if cookie.contains('\n') {
793 + bail!("polkit sent a cookie containing a newline");
794 + }
736 795 writeln!(stdin, "{cookie}").context("the helper closed before the cookie")?;
737 796
738 797 while let Some(line) = lines.next().transpose().context("the helper stopped")? {
739 798 if withdrawn.load(Ordering::Relaxed) {
740 - finish(&mut child);
741 799 bail!("withdrawn");
742 800 }
743 801 match Directive::parse(&line) {
@@ -764,7 +822,6 @@
764 822 // is a dismissal rather than an answer.
765 823 let answer = answers.recv().unwrap_or(None);
766 824 let Some(answer) = answer else {
767 - finish(&mut child);
768 825 bail!("dismissed");
769 826 };
770 827 // Checked again here, and not only at the top of the loop.
@@ -791,7 +848,6 @@
791 848 // after the bytes leave is one that lands after the helper has
792 849 // them, and no amount of locking on this side changes that.
793 850 if withdrawn.load(Ordering::Relaxed) {
794 - finish(&mut child);
795 851 bail!("withdrawn");
796 852 }
797 853 // The caller's obligation from [`Prompt::answer`], enforced
@@ -801,7 +857,6 @@
801 857 // an answer carrying one is refused rather than written. The
802 858 // value is not named in the error: it is the password.
803 859 if answer.expose().contains(&b'\n') {
804 - finish(&mut child);
805 860 bail!("an answer cannot contain a newline");
806 861 }
807 862 // Written as bytes rather than through `writeln!`, because the
@@ -813,14 +868,8 @@
813 868 .and_then(|()| stdin.write_all(b"\n"))
814 869 .context("the helper closed mid-answer")?;
815 870 }
816 - Some(Directive::Success) => {
817 - finish(&mut child);
818 - return Ok(());
819 - }
820 - Some(Directive::Failure) => {
821 - finish(&mut child);
822 - bail!("not authorized");
823 - }
871 + Some(Directive::Success) => return Ok(()),
872 + Some(Directive::Failure) => bail!("not authorized"),
824 873 // PAM_ERROR_MSG and PAM_TEXT_INFO carry text for the user, and
825 874 // anything unrecognized is a helper newer than this code. Neither
826 875 // is a reason to abandon a conversation that is still going: the
@@ -830,19 +879,35 @@
830 879 }
831 880 }
832 881
833 - finish(&mut child);
834 882 bail!("the helper ended without saying whether it worked")
835 883 }
836 884
837 - /// Close the helper out.
885 + /// The helper, closed out however the conversation ends.
838 886 ///
839 - /// Killing rather than waiting politely, because the paths that reach here have
840 - /// already decided the conversation is over and a helper mid-`pam_authenticate`
841 - /// can sit for as long as its PAM stack wants. The wait is what stops it
842 - /// becoming a zombie for the life of the console.
843 - fn finish(child: &mut Child) {
844 - let _ = child.kill();
845 - let _ = child.wait();
887 + /// A guard rather than a call at each exit, and the difference was a real leak
888 + /// rather than a tidiness argument. [`converse`] has six places it can leave by
889 + /// `?` — stdin, stdout, the cookie write, the read of each line, the write of
890 + /// the answer, and the `ask` that hands the question to the screen — and
891 + /// `std::process::Child` implements no `Drop`, so a helper let go of on any of
892 + /// them is neither killed nor reaped.
893 + ///
894 + /// The `ask` one is not hypothetical. It is the path [`Agent::drop`]
895 + /// deliberately creates: closing the prompt receiver turns a parked `send` into
896 + /// an error so the conversation ends rather than holding the console open. So
897 + /// the case was "the console is quitting", and what it left behind was a setuid
898 + /// helper inside `pam_authenticate` for a console that no longer exists.
899 + ///
900 + /// Killing rather than waiting politely, because every path here has already
901 + /// decided the conversation is over and a helper mid-`pam_authenticate` can sit
902 + /// for as long as its PAM stack wants. The wait is what stops it becoming a
903 + /// zombie for the life of the console.
904 + struct Helper(Child);
905 +
906 + impl Drop for Helper {
907 + fn drop(&mut self) {
908 + let _ = self.0.kill();
909 + let _ = self.0.wait();
910 + }
846 911 }
847 912
848 913 /// Strip control and format characters out of text this code did not write.
@@ -1443,6 +1508,80 @@
1443 1508 );
1444 1509 }
1445 1510
1511 + // A conversation whose question cannot be delivered still has to close the
1512 + // helper out. This is not a hypothetical path: it is the one
1513 + // [`Agent::drop`] deliberately creates, by closing the prompt receiver so a
1514 + // parked `send` fails rather than holding the console open. The helper is
1515 + // setuid and sits inside `pam_authenticate`, which `finish`'s own doc notes
1516 + // "can sit for as long as its PAM stack wants", so leaving it is a live
1517 + // process holding a PAM conversation for a console that has exited.
1518 + //
1519 + // Uses a script that would outlive the conversation on its own, so the
1520 + // assertion is about this code closing it rather than about the child
1521 + // happening to end.
1522 + #[test]
1523 + fn a_question_nobody_can_receive_still_closes_the_helper() {
1524 + const SLEEPS: &str = "#!/bin/sh\n\
1525 + read -r cookie\n\
1526 + printf 'PAM_PROMPT_ECHO_OFF Password: \\n'\n\
1527 + sleep 30\n\
1528 + printf 'FAILURE\\n'\n";
1529 +
1530 + let helper = scripted_helper("undeliverable", SLEEPS);
1531 + let before = descendants();
1532 +
1533 + let outcome = converse(
1534 + helper.path(),
1535 + "someone",
1536 + "cookie",
1537 + "an.action",
1538 + "a message",
1539 + &Arc::new(AtomicBool::new(false)),
1540 + // Exactly what `begin_authentication` passes when the receiver has
1541 + // gone: the console stopped listening.
1542 + |_prompt| bail!("the console stopped listening"),
1543 + );
1544 + assert!(
1545 + outcome.is_err(),
1546 + "the conversation should not have succeeded"
1547 + );
1548 +
1549 + // The child is closed out synchronously by `finish`, so by the time
1550 + // converse has returned there is nothing left to wait for.
1551 + let after = descendants();
1552 + let leaked: Vec<&u32> = after.iter().filter(|pid| !before.contains(pid)).collect();
1553 + assert!(
1554 + leaked.is_empty(),
1555 + "converse returned leaving {} helper process(es) behind: {leaked:?}",
1556 + leaked.len(),
1557 + );
1558 + }
1559 +
1560 + /// This process's children, by pid, from /proc.
1561 + ///
1562 + /// Reads the process table rather than trusting a handle, because the thing
1563 + /// under test is precisely whether a handle was dropped without being
1564 + /// waited on. A zombie is still a child here, which is the point: an
1565 + /// unreaped helper is a leak even once it has stopped running.
1566 + fn descendants() -> Vec<u32> {
1567 + let me = std::process::id();
1568 + let Ok(entries) = std::fs::read_dir("/proc") else {
1569 + return Vec::new();
1570 + };
1571 + entries
1572 + .filter_map(|entry| {
1573 + let name = entry.ok()?.file_name();
1574 + let pid: u32 = name.to_str()?.parse().ok()?;
1575 + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?;
1576 + // Field 4 is the parent pid; same hostile-comm rule as
1577 + // `start_time`, so read past the last `)`.
1578 + let after_comm = stat.rfind(')').map(|end| &stat[end + 1..])?;
1579 + let ppid: u32 = after_comm.split_whitespace().nth(1)?.parse().ok()?;
1580 + (ppid == me).then_some(pid)
1581 + })
1582 + .collect()
1583 + }
1584 +
1446 1585 // ---- the conversation registry ----
1447 1586
1448 1587 fn listener() -> Listener {