diff --git a/crates/tapline-auth/src/local.rs b/crates/tapline-auth/src/local.rs index f79f5b3..d570f3e 100644 --- a/crates/tapline-auth/src/local.rs +++ b/crates/tapline-auth/src/local.rs @@ -30,11 +30,28 @@ pub fn home_relative_roots() -> &'static [&'static str] { } } -fn roots(home: &Path) -> Vec { - let mut roots: Vec = home_relative_roots() - .iter() - .map(|relative| home.join(relative)) - .collect(); +/// Where the account's home directory is. +/// +/// Windows does not set `HOME`; it sets `USERPROFILE`. Anything that looks for +/// a Steam install has to ask for both, which is what this is for -- looking +/// for only one of them is how `libraries()` used to come back empty on every +/// Windows machine. +fn home() -> Option { + std::env::var_os("HOME") + .or_else(|| std::env::var_os("USERPROFILE")) + .map(PathBuf::from) +} + +fn roots(home: Option<&Path>) -> Vec { + let mut roots: Vec = match home { + Some(home) => home_relative_roots() + .iter() + .map(|relative| home.join(relative)) + .collect(), + // A machine with no home directory named in the environment can still + // have Steam installed where the installer puts it. + None => Vec::new(), + }; for key in ["ProgramFiles(x86)", "ProgramFiles"] { if let Some(base) = std::env::var_os(key) { roots.push(PathBuf::from(base).join("Steam")); @@ -45,13 +62,7 @@ fn roots(home: &Path) -> Vec { #[must_use] pub fn discover() -> Vec { - let Some(home) = std::env::var_os("HOME") - .or_else(|| std::env::var_os("USERPROFILE")) - .map(PathBuf::from) - else { - return Vec::new(); - }; - discover_in(&roots(&home)) + discover_in(&roots(home().as_deref())) } #[must_use] @@ -127,11 +138,7 @@ fn sort_most_recent_first(accounts: &mut [LocalAccount]) { #[must_use] pub fn libraries() -> Vec { - let Some(home) = std::env::var_os("HOME").map(PathBuf::from) else { - return Vec::new(); - }; - - for root in roots(&home) { + for root in roots(home().as_deref()) { let path = root.join("config/libraryfolders.vdf"); let Ok(text) = std::fs::read_to_string(&path) else { continue; @@ -188,7 +195,7 @@ mod tests { #[test] fn a_home_yields_one_root_per_known_layout() { let home = std::path::Path::new("/home/someone"); - let roots = roots(home); + let roots = roots(Some(home)); assert!(roots.len() >= home_relative_roots().len()); assert!( roots @@ -196,6 +203,20 @@ mod tests { .all(|root| root.starts_with(home) || root.is_absolute()) ); } + + #[test] + fn no_home_still_leaves_the_install_directories_to_look_in() { + // `libraries()` used to give up here, which on Windows was every time: + // it asked only for HOME, which Windows does not set. + let roots = roots(None); + assert_eq!( + roots.len(), + ["ProgramFiles(x86)", "ProgramFiles"] + .iter() + .filter(|key| std::env::var_os(key).is_some()) + .count(), + ); + } use super::*; const TWO_ACCOUNTS: &str = r#" diff --git a/crates/tapline-auth/src/sddl.rs b/crates/tapline-auth/src/sddl.rs index 3447972..70a9301 100644 --- a/crates/tapline-auth/src/sddl.rs +++ b/crates/tapline-auth/src/sddl.rs @@ -65,11 +65,52 @@ fn dacl_body(descriptor: &str) -> Option<&str> { Some(body) } -fn aces(body: &str) -> impl Iterator { - body.split('(').skip(1).filter_map(|ace| { - let end = ace.find(')')?; - ace.get(..end) - }) +/// The access-control entries in a DACL body, each without its brackets. +/// +/// Splitting on `(` and cutting at the first `)` looks like it would do, and +/// does for an ordinary ACE, but a conditional one carries a bracketed +/// expression of its own: `(XA;;FA;;;WD;(Member_of {SID}))`. The naive split +/// left the grant itself unterminated, so it was dropped, and a file shared +/// with everyone through a conditional ACE read back as private. `XA` and `ZA` +/// are in `ALLOW_ACE_TYPES`, so those are entries this is meant to see. +/// Counting brackets keeps each ACE whole. +fn aces(body: &str) -> Vec<&str> { + let mut found = Vec::new(); + let mut depth: usize = 0; + let mut start: Option = None; + for (at, byte) in body.bytes().enumerate() { + match byte { + b'(' => { + if depth == 0 { + start = Some(at.saturating_add(1)); + } + depth = depth.saturating_add(1); + } + b')' => { + if depth == 0 { + continue; + } + depth -= 1; + if depth == 0 + && let Some(from) = start.take() + && let Some(ace) = body.get(from..at) + { + found.push(ace); + } + } + _ => {} + } + } + // A bracket that never closes is a descriptor we do not understand. Keeping + // what it opened means the grantee inside it is still read and can still be + // reported, rather than a malformed list quietly reading as private. + if depth > 0 + && let Some(from) = start + && let Some(ace) = body.get(from..) + { + found.push(ace); + } + found } #[cfg(test)] @@ -180,3 +221,37 @@ mod tests { assert_eq!(shared_with("D:P", owned_by(OWNER)), None); } } + +#[cfg(test)] +mod conditional_ace_tests { + use super::{aces, shared_with}; + + const OWNER: &str = "S-1-5-21-1111111111-2222222222-3333333333-1001"; + + fn owned_by(owner: &str) -> impl Fn(&str) -> bool + '_ { + move |holder| holder.eq_ignore_ascii_case(owner) + } + + #[test] + fn a_conditional_ace_keeps_its_own_brackets() { + let body = "P(A;;FA;;;S-1-5-21-1)(XA;;FA;;;WD;(Member_of {SID(BA)}))"; + assert_eq!( + aces(body), + vec!["A;;FA;;;S-1-5-21-1", "XA;;FA;;;WD;(Member_of {SID(BA)})"] + ); + } + + #[test] + fn a_conditional_grant_to_everyone_is_reported() { + // Before brackets were counted this came back `None`: the grant was cut + // in half, dropped, and the file read as the owner's alone. + let dacl = format!("D:P(A;;FA;;;{OWNER})(XA;;FA;;;WD;(Member_of {{SID(BA)}}))"); + assert_eq!(shared_with(&dacl, owned_by(OWNER)), Some("WD".to_owned())); + } + + #[test] + fn an_unterminated_ace_is_not_silently_dropped() { + let dacl = format!("D:P(A;;FA;;;{OWNER})(A;;FA;;;WD"); + assert_eq!(shared_with(&dacl, owned_by(OWNER)), Some("WD".to_owned())); + } +} diff --git a/crates/tapline-auth/src/store.rs b/crates/tapline-auth/src/store.rs index 1b130ce..ceb74f1 100644 --- a/crates/tapline-auth/src/store.rs +++ b/crates/tapline-auth/src/store.rs @@ -256,11 +256,31 @@ fn resolve_config_dir(configured: Option, home: Option) -> Pat configured.unwrap_or_else(|| home.unwrap_or_else(|| PathBuf::from(".")).join(".config")) } +/// Remove whatever is sitting at the temporary path before the token is +/// written there. +/// +/// Both `create_private` arms below ask the operating system to create a new +/// file with permissions of our choosing, and on both platforms those +/// permissions are applied at creation and only at creation. Writing into a +/// file that is already there -- a temporary a crashed run left behind, or one +/// another local account put there first -- would put the refresh token under +/// whatever permissions that file already carries. So the old one goes first, +/// and the create that follows refuses to open anything it did not make, which +/// turns a race into an error rather than into a readable secret. +fn clear_temporary(temporary: &Path) -> Result<(), TokenStoreError> { + match std::fs::remove_file(temporary) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(TokenStoreError::Backend(e.to_string())), + } +} + #[cfg(windows)] fn create_private(path: &Path, contents: &str) -> Result<(), TokenStoreError> { use std::io::Write; let temporary = path.with_extension("tmp"); + clear_temporary(&temporary)?; let owner = crate::windows_acl::current_user_sid() .map_err(|e| TokenStoreError::Backend(e.to_string()))?; let mut file = @@ -281,8 +301,15 @@ fn create_private(path: &Path, contents: &str) -> Result<(), TokenStoreError> { use std::io::Write; let temporary = path.with_extension("tmp"); + clear_temporary(&temporary)?; let mut options = std::fs::OpenOptions::new(); - options.write(true).create(true).truncate(true); + // `create_new` rather than `create` + `truncate`: the mode below is applied + // when the file is created and not when an existing one is opened, and + // `open` follows symlinks, so opening whatever is already there could put + // the token in a world-readable file or somewhere else entirely. O_CREAT + // with O_EXCL, which is what `create_new` is, refuses a symlink at the last + // component as well, so this closes both. + options.write(true).create_new(true); #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; @@ -457,6 +484,37 @@ mod tests { assert_eq!(names, vec!["one".to_owned(), "two".to_owned()]); } + #[cfg(unix)] + #[test] + fn a_temporary_left_behind_does_not_lend_the_token_its_permissions() { + use std::os::unix::fs::PermissionsExt; + + let scratch = Scratch::new("tokens-stale-temporary"); + let store = scratch.store(); + let TokenStore::File { path } = &store else { + unreachable!("the scratch store is a file store") + }; + + // A crashed run -- or another account that can write here -- leaves a + // readable file where the next save wants to put the token. The save + // used to open it as it was, keeping mode 0644, and then rename it over + // the real store, so the refresh token ended up world-readable. + let temporary = path.with_extension("tmp"); + std::fs::write(&temporary, "left behind").expect("stale temporary"); + std::fs::set_permissions(&temporary, std::fs::Permissions::from_mode(0o644)) + .expect("make it readable"); + + store.save(&token("someone")).expect("save"); + + let mode = std::fs::metadata(path) + .expect("metadata") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "the token file must be the owner's alone"); + assert!(store.load("someone").expect("load").is_some()); + } + #[test] fn forget_all_clears_the_store() { let scratch = Scratch::new("tokens-forget-all"); diff --git a/crates/tapline-auth/src/windows_acl.rs b/crates/tapline-auth/src/windows_acl.rs index b296888..ad44659 100644 --- a/crates/tapline-auth/src/windows_acl.rs +++ b/crates/tapline-auth/src/windows_acl.rs @@ -20,7 +20,7 @@ use windows_sys::Win32::Security::{ DACL_SECURITY_INFORMATION, EqualSid, GetTokenInformation, PSECURITY_DESCRIPTOR, PSID, SECURITY_ATTRIBUTES, TOKEN_QUERY, TOKEN_USER, TokenUser, }; -use windows_sys::Win32::Storage::FileSystem::{CREATE_ALWAYS, CreateFileW, FILE_ATTRIBUTE_NORMAL}; +use windows_sys::Win32::Storage::FileSystem::{CREATE_NEW, CreateFileW, FILE_ATTRIBUTE_NORMAL}; use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; const ATTRIBUTES_SIZE: u32 = size_of::() as u32; @@ -62,6 +62,15 @@ pub fn current_user_sid() -> io::Result { // the allocation happened to land unaligned, so copy the header out first. // The SID it points at stays where it is, inside `buffer`, which outlives // the call below. + if buffer.len() < size_of::() { + // Windows says how much room it wants before it writes, so this cannot + // happen; reading a short buffer as a TOKEN_USER would be out of + // bounds, which is worth one comparison to rule out rather than trust. + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "the token's user information was shorter than a TOKEN_USER", + )); + } let mut text: *mut u16 = ptr::null_mut(); let converted = unsafe { let user = ptr::read_unaligned(buffer.as_ptr().cast::()); @@ -73,12 +82,20 @@ pub fn current_user_sid() -> io::Result { Ok(unsafe { take_wide(text) }) } -/// Creates (or replaces) a file carrying `dacl`, an SDDL access-control list, -/// from the moment it exists. +/// Creates a file carrying `dacl`, an SDDL access-control list, from the moment +/// it exists. /// /// Setting the list afterwards would leave a window in which the file is /// readable by whoever the parent directory says, so it is passed to the /// creation call instead. +/// +/// The disposition is `CREATE_NEW`, and that is load-bearing: Windows applies +/// `lpSecurityAttributes` only when the call actually creates the file. Asking +/// it to replace an existing one instead truncates that file and keeps the list +/// it already had, so a temporary left behind by a crashed run -- or one +/// another account pre-created in a writable directory -- would have taken the +/// secret under its own permissions. Failing with `AlreadyExists` puts that +/// decision on the caller, which removes the previous file first. pub fn create_with_dacl(path: &Path, dacl: &str) -> io::Result { let mut descriptor: PSECURITY_DESCRIPTOR = ptr::null_mut(); let wide_dacl = wide(OsStr::new(dacl)); @@ -106,7 +123,7 @@ pub fn create_with_dacl(path: &Path, dacl: &str) -> io::Result { GENERIC_WRITE, 0, &attributes, - CREATE_ALWAYS, + CREATE_NEW, FILE_ATTRIBUTE_NORMAL, ptr::null_mut(), ) diff --git a/crates/tapline-ffi/src/lib.rs b/crates/tapline-ffi/src/lib.rs index af3e040..8209704 100644 --- a/crates/tapline-ffi/src/lib.rs +++ b/crates/tapline-ffi/src/lib.rs @@ -61,6 +61,12 @@ fn shared() -> Option<&'static std::sync::Arc> { fn pool() -> Option<&'static std::sync::Arc> { static POOL: OnceLock> = OnceLock::new(); Some(POOL.get_or_init(|| { + // This is the moment the budget stops being changeable, so it is where + // the latch belongs. Setting it in `spawn_job` instead meant that + // asking for the current budget -- which builds the pool -- left the + // latch down, and the next `tapline_set_total_concurrency` answered + // TAPLINE_OK while the pool went on running at the old size. + STARTED.store(true, std::sync::atomic::Ordering::Relaxed); let configured = TOTAL_CONCURRENCY.load(std::sync::atomic::Ordering::Relaxed) as usize; let budget = if configured == 0 { Shared::new(InstallOptions::default().concurrency) @@ -73,11 +79,13 @@ fn pool() -> Option<&'static std::sync::Arc> { #[unsafe(no_mangle)] pub extern "C" fn tapline_set_total_concurrency(chunks: u32) -> i32 { - TOTAL_CONCURRENCY.store(chunks, std::sync::atomic::Ordering::Relaxed); + // Checked before the store, so a rejected call leaves the value alone + // rather than recording a size that will never be used. if STARTED.load(std::sync::atomic::Ordering::Relaxed) { set_error("the concurrency budget is already in use and cannot be resized"); return TAPLINE_BAD_ARGUMENT; } + TOTAL_CONCURRENCY.store(chunks, std::sync::atomic::Ordering::Relaxed); TAPLINE_OK } @@ -174,8 +182,6 @@ where return TAPLINE_BAD_ARGUMENT; }; - STARTED.store(true, std::sync::atomic::Ordering::Relaxed); - let (sender, receiver) = tokio::sync::mpsc::unbounded_channel(); let handle = runtime.spawn(build(sender)); @@ -184,7 +190,11 @@ where held: std::sync::Mutex::new(None), handle, }); - unsafe { *out = Box::into_raw(job) }; + // SAFETY: `out` was null-checked above and the header promises it is + // writable for one pointer. `write_unaligned` so a caller whose out-slot is + // inside a packed struct is not undefined behaviour; C has no way to + // promise us the alignment and we have no way to check it. + unsafe { out.write_unaligned(Box::into_raw(job)) }; TAPLINE_OK } @@ -777,8 +787,17 @@ pub unsafe extern "C" fn tapline_job_next( return TAPLINE_BAD_ARGUMENT; }; - let held = job.held.lock().ok().and_then(|mut slot| slot.take()); - let message = match held { + // One lock for the whole function. Taking the held slot, reading the queue + // and putting the message back used to be three separate critical + // sections, so two threads calling this on one job could both find the slot + // empty, both take a message, and both write the slot -- losing one of + // them. The header says every function here is safe to call from any + // thread, so this has to serialise rather than interleave. + let Ok(mut slot) = job.held.lock() else { + set_error("the job's event queue was poisoned"); + return TAPLINE_BAD_ARGUMENT; + }; + let message = match slot.take() { Some(message) => Some(message), None => { let Ok(mut receiver) = job.events.lock() else { @@ -808,15 +827,23 @@ pub unsafe extern "C" fn tapline_job_next( }; if !out_len.is_null() { - unsafe { *out_len = message.len() }; + // SAFETY: null-checked, and the header promises a writable `size_t`. + // Unaligned for the same reason as `spawn_job`'s out-pointer. + unsafe { out_len.write_unaligned(message.len()) }; } if buf.is_null() || cap < message.len() { - if let Ok(mut slot) = job.held.lock() { - *slot = Some(message); - } + // The message is kept rather than dropped, so a caller that asked for + // the length first can ask again with a big enough buffer. + *slot = Some(message); return TAPLINE_BUFFER_TOO_SMALL; } + if message.is_empty() { + return TAPLINE_OK; + } + // SAFETY: `buf` is non-null and `cap` is at least `message.len()`, both + // checked immediately above, and the header promises `buf` is writable for + // `cap` bytes. A `String`'s bytes cannot overlap a buffer the caller owns. unsafe { std::ptr::copy_nonoverlapping(message.as_ptr(), buf, message.len()) }; TAPLINE_OK } @@ -849,11 +876,20 @@ pub unsafe extern "C" fn tapline_last_error(buf: *mut u8, cap: usize, out_len: * LAST_ERROR.with(|slot| { let message = slot.borrow(); if !out_len.is_null() { - unsafe { *out_len = message.len() }; + // SAFETY: null-checked; the header promises a writable `size_t`. + unsafe { out_len.write_unaligned(message.len()) }; } - if buf.is_null() || cap < message.len() { + if cap < message.len() || (buf.is_null() && !message.is_empty()) { return TAPLINE_BUFFER_TOO_SMALL; } + if message.is_empty() { + // Nothing to copy, and no error set is the ordinary case -- asking + // for the length with a null buffer used to answer + // TAPLINE_BUFFER_TOO_SMALL for the zero bytes it needed. + return TAPLINE_OK; + } + // SAFETY: `buf` is non-null and writable for `cap` bytes per the + // header, and `cap` is at least `message.len()`, checked above. unsafe { std::ptr::copy_nonoverlapping(message.as_ptr(), buf, message.len()) }; TAPLINE_OK }) diff --git a/crates/tapline-steamworks/src/lib.rs b/crates/tapline-steamworks/src/lib.rs index 7f0d2d5..679a863 100644 --- a/crates/tapline-steamworks/src/lib.rs +++ b/crates/tapline-steamworks/src/lib.rs @@ -1,4 +1,4 @@ -use std::ffi::{CStr, c_char, c_int, c_void}; +use std::ffi::{c_char, c_int, c_void}; use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; use tapline_ids::{AppId, PublishedFileId}; @@ -169,7 +169,63 @@ fn hunt(root: &Path, depth: usize) -> Option { None } +/// Make sure the Steam client can see which app this is. +/// +/// `SteamAPI_InitFlat` reads `SteamAppId` from the process environment, and +/// there is no other way to tell it. Skipping the call whenever the variable is +/// already set means a host that set it up front never reaches the `unsafe` +/// below. +fn announce_app_id(app: AppId) { + static ONCE: std::sync::Once = std::sync::Once::new(); + ONCE.call_once(|| { + for name in ["SteamAppId", "SteamGameId"] { + if std::env::var_os(name).is_some_and(|value| !value.is_empty()) { + continue; + } + // SAFETY: `set_var` is unsound while another thread may be reading + // the environment -- glibc can reallocate `environ` underneath a + // concurrent `getenv` -- and this crate, being a library, cannot + // prove on its own that nothing else is running. That is why + // `Steam::connect` documents the requirement that it is called + // before the host starts other threads, and why this is skipped + // entirely when the host has already set the variable. There is no + // safe way to set an environment variable in Rust 1.94 and no way + // to hand the value to the Steam client except through it. + unsafe { std::env::set_var(name, app.get().to_string()) }; + } + }); +} + +/// Whatever Steam wrote into an error buffer, read as text. +/// +/// Bounded by the buffer rather than by a terminator: `CStr::from_ptr` would +/// scan past the end if Steam ever filled all of it without writing a NUL. No +/// `unsafe` is needed for any of it -- a `c_char` is one byte, whichever sign +/// the target gives it. +fn message_in(buffer: &[c_char]) -> String { + let end = buffer + .iter() + .position(|byte| *byte == 0) + .unwrap_or(buffer.len()); + let bytes: Vec = buffer + .get(..end) + .unwrap_or_default() + .iter() + .map(|byte| *byte as u8) + .collect(); + String::from_utf8_lossy(&bytes).trim().to_owned() +} + impl Steam { + /// Connect to a running Steam client. + /// + /// Call this before the host starts any other thread. The Steam client + /// reads `SteamAppId` out of the process environment while it initialises, + /// so if it is not already set this has to set it, and setting an + /// environment variable is only sound while nothing else can be reading + /// one. If the host cannot promise that, it should put `SteamAppId` and + /// `SteamGameId` in the environment itself before starting threads, and + /// this will find them there and leave them alone. pub fn connect(app: AppId) -> Result { let roots = steam_roots(); let path = find_library(&roots).ok_or_else(|| { @@ -182,11 +238,7 @@ impl Steam { ) })?; - // SAFETY: still single-threaded; no other thread observes the environment. - unsafe { - std::env::set_var("SteamAppId", app.get().to_string()); - std::env::set_var("SteamGameId", app.get().to_string()); - } + announce_app_id(app); // SAFETY: every signature below is the documented flat-API one. unsafe { @@ -212,13 +264,14 @@ impl Steam { let init_flat = std::mem::transmute:: c_int>( sym(b"SteamAPI_InitFlat\0")?, ); - let mut err = [0_i8; 1024]; + // `c_char` is `i8` on x86-64 and on Apple silicon but `u8` on + // aarch64 Linux, so the element type has to follow it rather than + // be spelled out, or this crate stops compiling on a Steam-capable + // ARM server. + let mut err = [0 as c_char; 1024]; let rc = init_flat(err.as_mut_ptr()); if rc != 0 { - let why = CStr::from_ptr(err.as_ptr()) - .to_string_lossy() - .trim() - .to_owned(); + let why = message_in(&err); return Err(SteamError::InitFailed(if why.is_empty() { format!("code {rc}") } else {