diff --git a/crates/pdfs-core/src/auth.rs b/crates/pdfs-core/src/auth.rs index 2f08a96..46e909a 100644 --- a/crates/pdfs-core/src/auth.rs +++ b/crates/pdfs-core/src/auth.rs @@ -364,11 +364,11 @@ fn keep_passphrases( let outcome = if remember { stored.key_passphrases = Some(encode(passphrases)); // Best-effort: a system without a kernel keyring has nothing to clear. - let _ = kernelkey::clear(); + let _ = kernelkey::PASSPHRASES.clear(); Ok(()) } else { stored.key_passphrases = None; - kernelkey::store(&encode_secret(passphrases)).map_err(|e| { + kernelkey::PASSPHRASES.store(&encode_secret(passphrases)).map_err(|e| { Error::Other(format!( "the kernel keyring is unavailable, so the unlock cannot be kept for this session: {e}" )) @@ -469,7 +469,7 @@ fn find_passphrases(stored: &StoredSession) -> Result Ok(decode_secret(&secret).map(|p| (p, KeyState::Session))), Ok(None) => Ok(None), // No keyring on this system: nothing was ever kept there. @@ -495,8 +495,7 @@ pub async fn unlock(mailbox: &str, remember: bool) -> Result<()> { return Err(Error::ReloginRequired); } let mailbox = Zeroizing::new(mailbox.to_owned()); - let session = ProtonApiSession::resume(client_config(), stored.to_params())?; - register_refresh_handler(&session, stored.refresh_token.clone()); + let session = resume_stored(&stored)?; let passphrases = derive_verified(&session, mailbox.as_bytes(), stored.key_salts.clone()).await?; // Read again: verifying may have rotated the tokens, and the copy read @@ -507,7 +506,7 @@ pub async fn unlock(mailbox: &str, remember: bool) -> Result<()> { /// Drop the passphrases held in kernel memory, so the account is locked again /// until [`unlock`]. With `forget`, also those in the keyring blob. pub fn lock(forget: bool) -> Result<()> { - kernelkey::clear()?; + kernelkey::PASSPHRASES.clear()?; if forget { let mut stored = load()?; if stored.key_passphrases.take().is_some() { @@ -535,9 +534,10 @@ pub fn logout() -> Result<()> { let _ = std::fs::remove_file(format!("{}{suffix}", path.display())); } } - if let Err(e) = kernelkey::clear() { + if let Err(e) = kernelkey::PASSPHRASES.clear() { tracing::warn!(error = %e, "clearing the kernel-keyring unlock on logout failed"); } + let _ = kernelkey::UNSAVED_TOKENS.clear(); match keyring_entry()?.delete_credential() { Ok(()) | Err(keyring_core::Error::NoEntry) => Ok(()), Err(e) => Err(e.into()), @@ -557,8 +557,7 @@ pub fn logout() -> Result<()> { /// a daemon starting offline can still mount from its cache. pub async fn resume_client() -> Result<(ProtonDriveClient, ProtonApiSession)> { let stored = load()?; - let session = ProtonApiSession::resume(client_config(), stored.to_params())?; - register_refresh_handler(&session, stored.refresh_token.clone()); + let session = resume_stored(&stored)?; let passphrases = match find_passphrases(&stored)? { Some((passphrases, _)) => passphrases, @@ -573,6 +572,48 @@ pub async fn resume_client() -> Result<(ProtonDriveClient, ProtonApiSession)> { Ok((client, session)) } +/// Resume `stored`, with the rotation the keyring refused last time put in +/// first, if there is one: Proton has spent the stored refresh token then, +/// and the kernel keyring holds the one that replaced it (B61). +fn resume_stored(stored: &StoredSession) -> Result { + let unsaved = recover_unsaved(stored); + let mut resumed = stored.clone(); + if let Some(tokens) = &unsaved { + tracing::info!("resuming with session tokens the keyring refused last time"); + resumed.access_token = tokens.access_token.clone(); + resumed.refresh_token = tokens.refresh_token.clone(); + } + let session = ProtonApiSession::resume(client_config(), resumed.to_params())?; + let sink = register_refresh_handler(&session, stored.refresh_token.clone()); + if let Some(tokens) = unsaved { + let _ = sink.rotated(tokens); + } + Ok(session) +} + +/// The rotation the kernel keyring holds for `stored`, if it belongs there. +/// One that does not is from an older login, or was written since, and is +/// dropped. +fn recover_unsaved(stored: &StoredSession) -> Option { + let kept = match kernelkey::UNSAVED_TOKENS.load() { + Ok(kept) => kept?, + Err(e) => { + tracing::debug!(error = %e, "kernel keyring unavailable; no unsaved tokens to recover"); + return None; + } + }; + match serde_json::from_slice::(&kept) { + Ok(unsaved) if unsaved.belongs_in(stored) => Some(Tokens { + access_token: unsaved.access_token, + refresh_token: unsaved.refresh_token, + }), + _ => { + let _ = kernelkey::UNSAVED_TOKENS.clear(); + None + } + } +} + /// Turn a blob written by 3.0.x — which holds the mailbox password — into one /// that holds passphrases, so the password stops being stored. /// @@ -747,6 +788,27 @@ struct SinkState { stored_refresh: String, /// The latest rotation, until the keyring has taken it. unsaved: Option, + /// Whether `unsaved` is also in [`kernelkey::UNSAVED_TOKENS`]. + kept: bool, +} + +/// A rotation the keyring refused, as kept in the kernel keyring until it is +/// written: the tokens, and the blob they go into. +#[derive(Serialize, Deserialize)] +struct UnsavedRotation { + session_id: String, + /// The refresh token this rotation spent, which the blob still holds. + spent: String, + access_token: String, + refresh_token: String, +} + +impl UnsavedRotation { + /// Whether `stored` is the blob this rotation was refused for, still + /// holding the refresh token it spent. + fn belongs_in(&self, stored: &StoredSession) -> bool { + self.session_id == stored.session_id && self.spent == stored.refresh_token + } } /// What became of a rotation offered to a stored blob. @@ -784,6 +846,7 @@ impl TokenSink { state: parking_lot::Mutex::new(SinkState { stored_refresh, unsaved: None, + kept: false, }), retrying: AtomicBool::new(false), } @@ -796,12 +859,44 @@ impl TokenSink { fn rotated(self: &Arc, tokens: Tokens) -> Result<()> { self.state.lock().unsaved = Some(tokens); let written = self.flush(); - if written.is_err() { + if let Err(e) = &written { + tracing::error!( + error = %e, + "the keyring refused the refreshed session tokens; retrying" + ); + self.keep_in_kernel(); self.retry_in_background(); } written } + /// Keep the unsaved rotation in the kernel keyring too, so a process that + /// dies before the retry lands does not take the only live refresh token + /// with it. The next resume puts it back (see [`resume_stored`]). + fn keep_in_kernel(&self) { + let mut state = self.state.lock(); + let Some(tokens) = &state.unsaved else { + return; + }; + let unsaved = UnsavedRotation { + session_id: self.session_id.clone(), + spent: state.stored_refresh.clone(), + access_token: tokens.access_token.clone(), + refresh_token: tokens.refresh_token.clone(), + }; + let Ok(json) = serde_json::to_vec(&unsaved).map(Zeroizing::new) else { + return; + }; + match kernelkey::UNSAVED_TOKENS.store(&json) { + Ok(()) => state.kept = true, + Err(e) => tracing::error!( + error = %e, + "the kernel keyring refused them too; if this process stops first, \ + the next start needs `pdfs login`" + ), + } + } + /// Retry the unsaved rotation on [`KEYRING_RETRY`] until it is written or /// has nowhere left to go. One thread per sink; a newer rotation simply /// replaces what it is retrying. @@ -850,7 +945,7 @@ impl TokenSink { let mut stored = match load() { Ok(stored) => stored, Err(Error::NotLoggedIn) => { - state.unsaved = None; + state.settle(); return Ok(()); } Err(e) => return Err(e), @@ -872,11 +967,22 @@ impl TokenSink { ); } } - state.unsaved = None; + state.settle(); Ok(()) } } +impl SinkState { + /// The unsaved rotation has gone where it can: drop it, and its copy in + /// the kernel keyring. + fn settle(&mut self) { + self.unsaved = None; + if std::mem::take(&mut self.kept) { + let _ = kernelkey::UNSAVED_TOKENS.clear(); + } + } +} + /// Persist rotated tokens the moment the session obtains them. /// /// `stored_refresh` is the refresh token the keyring holds for this session @@ -884,21 +990,18 @@ impl TokenSink { /// cannot write back a stale copy over a later unlock or lock, and it writes /// only into a blob that is still this session's (see [`TokenSink`]), so it /// cannot resurrect a logged-out session or overwrite a newer login. -fn register_refresh_handler(session: &ProtonApiSession, stored_refresh: String) { +fn register_refresh_handler(session: &ProtonApiSession, stored_refresh: String) -> Arc { let session_id = session.session_id().as_str(); let sink = Arc::new(TokenSink::new(session_id, stored_refresh)); TOKEN_SINKS .lock() .insert(session_id.to_owned(), Arc::clone(&sink)); + let handler = Arc::clone(&sink); session.http().set_on_tokens_refreshed(move |tokens| { - if let Err(e) = sink.rotated(tokens) { - tracing::error!( - error = %e, - "the keyring refused the refreshed session tokens; retrying. \ - If the daemon stops first, the next start needs `pdfs login`" - ) - } + // A refusal is logged and retried by the sink itself. + let _ = handler.rotated(tokens); }); + sink } #[cfg(test)] @@ -1118,6 +1221,15 @@ mod tests { std::io::Error::other("locked").into(), )); assert!(sink.rotated(tokens("a2", "r2")).is_err()); + // Until it is written, a stop does not lose it: the next resume finds + // it in the kernel keyring, where one is offered. + let kept = sink.state.lock().kept; + if kept { + assert_eq!( + recover_unsaved(&stored_with("r")).unwrap().refresh_token, + "r2" + ); + } let deadline = std::time::Instant::now() + Duration::from_secs(5); while load().unwrap().refresh_token != "r2" { @@ -1128,5 +1240,24 @@ mod tests { std::thread::sleep(Duration::from_millis(50)); } assert!(sink.state.lock().unsaved.is_none()); + if kept { + assert!(kernelkey::UNSAVED_TOKENS.load().unwrap().is_none()); + } + } + + #[test] + fn an_unsaved_rotation_goes_only_into_the_blob_it_was_refused_for() { + let unsaved = UnsavedRotation { + session_id: "s".into(), + spent: "r".into(), + access_token: "a2".into(), + refresh_token: "r2".into(), + }; + assert!(unsaved.belongs_in(&stored_with("r"))); + // Written since, by a retry or another process. + assert!(!unsaved.belongs_in(&stored_with("r2"))); + let mut login = stored_with("r"); + login.session_id = "new".into(); + assert!(!unsaved.belongs_in(&login)); } } diff --git a/crates/pdfs-core/src/kernelkey.rs b/crates/pdfs-core/src/kernelkey.rs index a090cb8..d0ea652 100644 --- a/crates/pdfs-core/src/kernelkey.rs +++ b/crates/pdfs-core/src/kernelkey.rs @@ -5,6 +5,8 @@ //! kept in kernel memory — not on disk, not in the Secret Service — and vanish //! when the user's last session ends or the machine reboots. Both the daemon and //! `pdfs unlock` reach the same entry because it hangs off the *user* keyring. +//! It is also where session tokens wait while the Secret Service refuses them +//! ([`UNSAVED_TOKENS`]), so they outlive the process that holds them. //! //! Kernel keys are not paged to swap, are invisible to other users, and are //! wiped by [`clear`] (`KEYCTL_REVOKE`), which makes them a better home than a @@ -20,12 +22,34 @@ use std::io; use zeroize::Zeroizing; -/// Key description. Namespaced so it cannot collide with another program's key. +/// One secret in the user keyring, by its description. Descriptions are +/// namespaced so they cannot collide with another program's keys. +pub struct Key { + description: &'static [u8], +} + +/// The account's key passphrases (see the module docs). #[cfg(not(test))] -const DESCRIPTION: &[u8] = b"pdfs:key-passphrases\0"; +pub const PASSPHRASES: Key = Key { + description: b"pdfs:key-passphrases\0", +}; /// Tests must never touch a real unlock. #[cfg(test)] -const DESCRIPTION: &[u8] = b"pdfs:key-passphrases:test\0"; +pub const PASSPHRASES: Key = Key { + description: b"pdfs:key-passphrases:test\0", +}; + +/// Session tokens the Secret Service refused to store, until it takes them. +/// A refresh token held only in a process's memory dies with the process. +#[cfg(not(test))] +pub const UNSAVED_TOKENS: Key = Key { + description: b"pdfs:unsaved-tokens\0", +}; +#[cfg(test)] +pub const UNSAVED_TOKENS: Key = Key { + description: b"pdfs:unsaved-tokens:test\0", +}; + const KEY_TYPE: &[u8] = b"user\0"; const KEY_SPEC_USER_KEYRING: i32 = -4; @@ -49,7 +73,7 @@ fn last_error() -> io::Result { Err(io::Error::last_os_error()) } -fn find() -> io::Result> { +fn find(description: &[u8]) -> io::Result> { // SAFETY: both strings are NUL-terminated and outlive the call. let id = unsafe { libc::syscall( @@ -57,7 +81,7 @@ fn find() -> io::Result> { KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, KEY_TYPE.as_ptr(), - DESCRIPTION.as_ptr(), + description.as_ptr(), 0, ) }; @@ -83,100 +107,102 @@ pub fn forbid_dumps() { } } -/// Store `secret`, replacing any earlier one. -pub fn store(secret: &[u8]) -> io::Result<()> { - // SAFETY: strings are NUL-terminated; the payload pointer/length pair - // describes `secret`, which outlives the call. - let id = unsafe { - libc::syscall( - libc::SYS_add_key, - KEY_TYPE.as_ptr(), - DESCRIPTION.as_ptr(), - secret.as_ptr(), - secret.len(), - KEY_SPEC_USER_KEYRING, - ) - }; - if id < 0 { - return last_error(); - } - // SAFETY: plain integer arguments. - let rc = unsafe { - libc::syscall( - libc::SYS_keyctl, - KEYCTL_SETPERM, - id, - PERM_OWNER_AND_POSSESSOR, - ) - }; - if rc < 0 { - // A key whose permissions are unknown is not one to leave behind. - let err = io::Error::last_os_error(); - let _ = clear(); - return Err(err); - } - Ok(()) -} - -/// Read the stored secret, or `None` when there is none. -pub fn load() -> io::Result>>> { - let Some(id) = find()? else { - return Ok(None); - }; - // Size first, then read; retry if the key was replaced in between. - for _ in 0..3 { - // SAFETY: a null buffer asks the kernel only for the payload length. - let len = unsafe { +impl Key { + /// Store `secret`, replacing any earlier one. + pub fn store(&self, secret: &[u8]) -> io::Result<()> { + // SAFETY: strings are NUL-terminated; the payload pointer/length pair + // describes `secret`, which outlives the call. + let id = unsafe { libc::syscall( - libc::SYS_keyctl, - KEYCTL_READ, - id, - std::ptr::null_mut::(), - 0usize, + libc::SYS_add_key, + KEY_TYPE.as_ptr(), + self.description.as_ptr(), + secret.as_ptr(), + secret.len(), + KEY_SPEC_USER_KEYRING, ) }; - if len < 0 { + if id < 0 { + return last_error(); + } + // SAFETY: plain integer arguments. + let rc = unsafe { + libc::syscall( + libc::SYS_keyctl, + KEYCTL_SETPERM, + id, + PERM_OWNER_AND_POSSESSOR, + ) + }; + if rc < 0 { + // A key whose permissions are unknown is not one to leave behind. + let err = io::Error::last_os_error(); + let _ = self.clear(); + return Err(err); + } + Ok(()) + } + + /// Read the stored secret, or `None` when there is none. + pub fn load(&self) -> io::Result>>> { + let Some(id) = find(self.description)? else { + return Ok(None); + }; + // Size first, then read; retry if the key was replaced in between. + for _ in 0..3 { + // SAFETY: a null buffer asks the kernel only for the payload length. + let len = unsafe { + libc::syscall( + libc::SYS_keyctl, + KEYCTL_READ, + id, + std::ptr::null_mut::(), + 0usize, + ) + }; + if len < 0 { + return match io::Error::last_os_error().raw_os_error() { + Some(libc::ENOKEY | libc::EKEYREVOKED | libc::EKEYEXPIRED) => Ok(None), + _ => last_error(), + }; + } + let mut buf = Zeroizing::new(vec![0u8; len as usize]); + // SAFETY: `buf` is `len` writable bytes. + let read = unsafe { + libc::syscall( + libc::SYS_keyctl, + KEYCTL_READ, + id, + buf.as_mut_ptr(), + buf.len(), + ) + }; + if read < 0 { + return last_error(); + } + if read as usize <= buf.len() { + buf.truncate(read as usize); + return Ok(Some(buf)); + } + } + Err(io::Error::other("the key changed while it was being read")) + } + + /// Remove the stored secret. Succeeds when there is none. + pub fn clear(&self) -> io::Result<()> { + let Some(id) = find(self.description)? else { + return Ok(()); + }; + // SAFETY: plain integer arguments. + let rc = unsafe { libc::syscall(libc::SYS_keyctl, KEYCTL_REVOKE, id) }; + if rc < 0 { return match io::Error::last_os_error().raw_os_error() { - Some(libc::ENOKEY | libc::EKEYREVOKED | libc::EKEYEXPIRED) => Ok(None), + Some(libc::ENOKEY | libc::EKEYREVOKED) => Ok(()), _ => last_error(), }; } - let mut buf = Zeroizing::new(vec![0u8; len as usize]); - // SAFETY: `buf` is `len` writable bytes. - let read = unsafe { - libc::syscall( - libc::SYS_keyctl, - KEYCTL_READ, - id, - buf.as_mut_ptr(), - buf.len(), - ) - }; - if read < 0 { - return last_error(); - } - if read as usize <= buf.len() { - buf.truncate(read as usize); - return Ok(Some(buf)); - } + Ok(()) } - Err(io::Error::other("the key changed while it was being read")) -} - -/// Remove the stored secret. Succeeds when there is none. -pub fn clear() -> io::Result<()> { - let Some(id) = find()? else { - return Ok(()); - }; - // SAFETY: plain integer arguments. - let rc = unsafe { libc::syscall(libc::SYS_keyctl, KEYCTL_REVOKE, id) }; - if rc < 0 { - return match io::Error::last_os_error().raw_os_error() { - Some(libc::ENOKEY | libc::EKEYREVOKED) => Ok(()), - _ => last_error(), - }; - } - Ok(()) } #[cfg(test)] @@ -187,18 +213,21 @@ mod tests { // so one test covers the whole round trip and skips when it is unavailable. #[test] fn stored_secret_round_trips_and_clears() { - if store(b"probe").is_err() { + if PASSPHRASES.store(b"probe").is_err() { eprintln!("kernel keyring unavailable; skipping"); return; } - let saved = load().unwrap().expect("stored secret is readable"); + let saved = PASSPHRASES + .load() + .unwrap() + .expect("stored secret is readable"); assert_eq!(saved.as_slice(), b"probe"); - store(b"replaced").unwrap(); - assert_eq!(load().unwrap().unwrap().as_slice(), b"replaced"); + PASSPHRASES.store(b"replaced").unwrap(); + assert_eq!(PASSPHRASES.load().unwrap().unwrap().as_slice(), b"replaced"); - clear().unwrap(); - assert!(load().unwrap().is_none()); - clear().unwrap(); + PASSPHRASES.clear().unwrap(); + assert!(PASSPHRASES.load().unwrap().is_none()); + PASSPHRASES.clear().unwrap(); } } diff --git a/docs/BUGS.md b/docs/BUGS.md index 2d92b8d..7dab2d2 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -4951,8 +4951,7 @@ daemon/tray/FUSE, upgrade, rollback, and uninstall without deleting user state. ### B61 — Rotated single-use credentials can be lost (HIGH-18) -**Status:** Partly fixed. A process that dies before a retried write lands still loses the -rotation, and only the log says so. +**Status:** Partly fixed. Only the log says that the keyring refused a rotation. **Found:** 2026-07-22, 1.0 authentication audit **Where:** `crates/pdfs-core/src/auth.rs`, refresh callback @@ -4968,10 +4967,17 @@ unavailable keyrings, callback races, repeated rotations, death, and restart. in the background after 1 s, 5 s, 30 s and then every 5 minutes, until it is written or the blob has moved on (B201). The refusal is an error in the log naming `pdfs login`, and the daemon's stop writes a rotation still unsaved, logging an error if that fails too. The re-login path is B201's: -an old session never writes into a newer login. Left: a crash or SIGKILL before a retry lands, -and telling the GUI. +an old session never writes into a newer login. -**Test:** `auth::tests::a_rotation_the_keyring_refused_is_written_once_it_takes_it`. +Until it is written, the rotation also waits in the kernel keyring (`kernelkey::UNSAVED_TOKENS`), +so a crash or SIGKILL before a retry lands no longer loses it. The next resume, by the daemon or +`pdfs unlock`, puts it back in when the blob still holds the refresh token it spent, and writes +it. One that belongs to an older login is dropped. The kernel keyring lasts until the user's last +session ends, so a refused write followed by a crash and a logout still loses the rotation. Left: +telling the GUI. + +**Test:** `auth::tests::a_rotation_the_keyring_refused_is_written_once_it_takes_it` and +`auth::tests::an_unsaved_rotation_goes_only_into_the_blob_it_was_refused_for`. ---