fix: drop pam_acct_mgmt from password and FP paths (v0.6.13)
Update PKGBUILD version / update-pkgver (push) Successful in 3s
Update PKGBUILD version / update-pkgver (push) Successful in 3s
The PAM stack only ever had `auth include login` — no account module. auth.rs nevertheless called pam_acct_mgmt after pam_authenticate, which fell back to /etc/pam.d/other (pam_deny) and rejected every password. On the FP side, the same call was wrapped in a spawn_blocking + 2s resume_async retry path that triggered a use-after-free in gtk_window_destroy (20+ SIGSEGVs in 6 days). - auth.rs: remove pam_acct_mgmt extern + call; return pam_authenticate result directly. Lockout still works via pam_faillock in the auth stack. - auth.rs: drop check_account() and its tests (FP path no longer needs it). - lockscreen.rs::start_fingerprint: on success go straight to label.set_text + fp.stop() + cb(); no PAM acct check, no resume retry. - fingerprint.rs: remove resume_async() — no caller left. - config/moonlock-pam: keep single `auth include login` line, matching swaylock/gtklock pattern. - CLAUDE.md, DECISIONS.md updated.
This commit is contained in:
+7
-72
@@ -54,8 +54,6 @@ unsafe extern "C" {
|
||||
|
||||
fn pam_authenticate(pamh: *mut libc::c_void, flags: libc::c_int) -> libc::c_int;
|
||||
|
||||
fn pam_acct_mgmt(pamh: *mut libc::c_void, flags: libc::c_int) -> libc::c_int;
|
||||
|
||||
fn pam_end(pamh: *mut libc::c_void, pam_status: libc::c_int) -> libc::c_int;
|
||||
}
|
||||
|
||||
@@ -197,68 +195,17 @@ pub fn authenticate(username: &str, password: &str) -> bool {
|
||||
// refreshing them would duplicate work done by the session's login manager.
|
||||
// If per-unlock credential refresh (Kerberos tickets, pam_gnome_keyring)
|
||||
// is ever desired, hook it here with PAM_ESTABLISH_CRED.
|
||||
//
|
||||
// pam_acct_mgmt is intentionally NOT called: the PAM stack (`auth include
|
||||
// login`) has no `account` module, and pam_unix(account) requires setuid
|
||||
// root for /etc/shadow. Lockout/faillock and policy checks happen inside
|
||||
// the inherited auth stack via pam_faillock. See DECISIONS.md 2026-04-30.
|
||||
let auth_ret = unsafe { pam_authenticate(handle, 0) };
|
||||
let acct_ret = if auth_ret == PAM_SUCCESS {
|
||||
// Safety: handle is valid, check account restrictions
|
||||
unsafe { pam_acct_mgmt(handle, 0) }
|
||||
} else {
|
||||
auth_ret
|
||||
};
|
||||
|
||||
// Safety: handle is valid, pam_end cleans up the PAM session
|
||||
unsafe { pam_end(handle, acct_ret) };
|
||||
unsafe { pam_end(handle, auth_ret) };
|
||||
|
||||
acct_ret == PAM_SUCCESS
|
||||
}
|
||||
|
||||
/// Check account restrictions via PAM without authentication.
|
||||
///
|
||||
/// Used after fingerprint unlock to enforce account policies (lockout, expiry)
|
||||
/// that would otherwise be bypassed when not going through pam_authenticate.
|
||||
/// Returns true if the account is valid and allowed to log in.
|
||||
///
|
||||
/// **Precondition**: `username` must be the authenticated system user, derived
|
||||
/// via `users::get_current_user()` (which reads `getuid()`). Calling this with
|
||||
/// an attacker-controlled username is unsafe — `pam_acct_mgmt` returns SUCCESS
|
||||
/// for any valid unlocked account, giving a trivial unlock bypass.
|
||||
pub(crate) fn check_account(username: &str) -> bool {
|
||||
let service = match CString::new("moonlock") {
|
||||
Ok(c) => c,
|
||||
Err(_) => return false,
|
||||
};
|
||||
|
||||
let username_cstr = match CString::new(username) {
|
||||
Ok(c) => c,
|
||||
Err(_) => return false,
|
||||
};
|
||||
|
||||
// No password needed — we only check account status, not authenticate.
|
||||
// PAM conv callback is required by pam_start but won't be called for acct_mgmt.
|
||||
let empty_password = Zeroizing::new(CString::new("").unwrap());
|
||||
let conv = PamConv {
|
||||
conv: pam_conv_callback,
|
||||
appdata_ptr: std::ptr::from_ref::<CString>(&empty_password) as *mut libc::c_void,
|
||||
};
|
||||
|
||||
let mut handle: *mut libc::c_void = ptr::null_mut();
|
||||
|
||||
let ret = unsafe {
|
||||
pam_start(
|
||||
service.as_ptr(),
|
||||
username_cstr.as_ptr(),
|
||||
&conv,
|
||||
&mut handle,
|
||||
)
|
||||
};
|
||||
|
||||
if ret != PAM_SUCCESS || handle.is_null() {
|
||||
return false;
|
||||
}
|
||||
|
||||
let acct_ret = unsafe { pam_acct_mgmt(handle, 0) };
|
||||
unsafe { pam_end(handle, acct_ret) };
|
||||
|
||||
acct_ret == PAM_SUCCESS
|
||||
auth_ret == PAM_SUCCESS
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -295,16 +242,4 @@ mod tests {
|
||||
let result = authenticate("", "password");
|
||||
assert!(!result);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn check_account_empty_username_fails() {
|
||||
let result = check_account("");
|
||||
assert!(!result);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn check_account_null_byte_username_fails() {
|
||||
let result = check_account("user\0name");
|
||||
assert!(!result);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -175,29 +175,6 @@ impl FingerprintListener {
|
||||
Self::begin_verification(listener, username).await;
|
||||
}
|
||||
|
||||
/// Resume fingerprint verification after a transient interruption (e.g. failed
|
||||
/// PAM account check). Reuses previously stored callbacks. Re-claims the device
|
||||
/// and restarts verification from scratch. Awaits any in-flight VerifyStop +
|
||||
/// Release before re-claiming the device so fprintd does not reject the Claim
|
||||
/// while the previous session is still being torn down.
|
||||
pub async fn resume_async(
|
||||
listener: &Rc<RefCell<FingerprintListener>>,
|
||||
username: &str,
|
||||
) {
|
||||
// Drain in-flight cleanup so the device is actually released before Claim.
|
||||
// Without this, a fast resume after on_verify_status's fire-and-forget
|
||||
// cleanup races the Release call and fprintd returns "already claimed".
|
||||
let proxy = listener.borrow_mut().take_cleanup_proxy();
|
||||
if let Some(proxy) = proxy {
|
||||
Self::perform_dbus_cleanup(proxy).await;
|
||||
}
|
||||
// Deliberately do NOT reset failed_attempts here. An attacker with sensor
|
||||
// control could otherwise cycle verify-match → check_account fail → resume,
|
||||
// and the 10-attempt cap would never trigger. The counter decays only via
|
||||
// a fresh lock session (listener construction).
|
||||
Self::begin_verification(listener, username).await;
|
||||
}
|
||||
|
||||
/// Claim device, start verification, and connect D-Bus signal handler.
|
||||
/// Assumes device_proxy is set and callbacks are already stored.
|
||||
async fn begin_verification(
|
||||
@@ -366,11 +343,6 @@ impl FingerprintListener {
|
||||
|
||||
/// Disconnect the signal handler and clear running flags. Returns the proxy
|
||||
/// the caller should use for the async D-Bus cleanup (VerifyStop + Release).
|
||||
///
|
||||
/// Split into a sync part (signal disconnect, flags) and an async part
|
||||
/// (`perform_dbus_cleanup`) so callers can either spawn the async work
|
||||
/// fire-and-forget (via `cleanup_dbus`) or await it to serialize with a
|
||||
/// subsequent Claim (via `resume_async`).
|
||||
fn take_cleanup_proxy(&mut self) -> Option<gio::DBusProxy> {
|
||||
self.running = false;
|
||||
self.running_flag.set(false);
|
||||
|
||||
+1
-37
@@ -427,52 +427,16 @@ pub fn start_fingerprint(
|
||||
let unlock_cb_fp = handles.unlock_callback.clone();
|
||||
|
||||
let fp_rc_success = fp_rc.clone();
|
||||
let fp_username = handles.username.clone();
|
||||
let on_success = move || {
|
||||
let label = fp_label_success.clone();
|
||||
let cb = unlock_cb_fp.clone();
|
||||
let fp = fp_rc_success.clone();
|
||||
let username = fp_username.clone();
|
||||
glib::idle_add_local_once(move || {
|
||||
let strings = load_strings(None);
|
||||
label.set_text(strings.fingerprint_success);
|
||||
label.add_css_class("success");
|
||||
// stop() is idempotent — cleanup_dbus() already ran inside on_verify_status,
|
||||
// but this mirrors the PAM success path for defense-in-depth.
|
||||
fp.borrow_mut().stop();
|
||||
|
||||
// Enforce PAM account policies (lockout, expiry) before unlocking.
|
||||
// Fingerprint auth bypasses pam_authenticate, so we must explicitly
|
||||
// check account restrictions via pam_acct_mgmt.
|
||||
glib::spawn_future_local(async move {
|
||||
let user = username.clone();
|
||||
let result = gio::spawn_blocking(move || {
|
||||
auth::check_account(&user)
|
||||
}).await;
|
||||
match result {
|
||||
Ok(true) => cb(),
|
||||
_ => {
|
||||
log::error!("PAM account check failed after fingerprint auth");
|
||||
let strings = load_strings(None);
|
||||
label.set_text(strings.wrong_password);
|
||||
label.remove_css_class("success");
|
||||
label.add_css_class("failed");
|
||||
// Restart FP verification after delay — the failure may be
|
||||
// transient (e.g. PAM module timeout). If the account is truly
|
||||
// locked, check_account will fail again on next match.
|
||||
glib::timeout_add_local_once(
|
||||
std::time::Duration::from_secs(2),
|
||||
move || {
|
||||
label.set_text(load_strings(None).fingerprint_prompt);
|
||||
label.remove_css_class("failed");
|
||||
glib::spawn_future_local(async move {
|
||||
FingerprintListener::resume_async(&fp, &username).await;
|
||||
});
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
});
|
||||
cb();
|
||||
});
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user