Stop the token file inheriting permissions, and three FFI fixes

crates/tapline-auth/src/local.rs

@@ -30,11 +30,28 @@ pub fn home_relative_roots() -> &'static [&'static str] {
     }
 }
 
-fn roots(home: &Path) -> Vec<PathBuf> {
-    let mut roots: Vec<PathBuf> = 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<PathBuf> {
+    std::env::var_os("HOME")
+        .or_else(|| std::env::var_os("USERPROFILE"))
+        .map(PathBuf::from)
+}
+
+fn roots(home: Option<&Path>) -> Vec<PathBuf> {
+    let mut roots: Vec<PathBuf> = 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<PathBuf> {
 
 #[must_use]
 pub fn discover() -> Vec<LocalAccount> {
-    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<PathBuf> {
-    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,14 +195,28 @@ 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
                 .iter()
                 .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#"

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<Item = &str> {
-    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<usize> = 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()));
+    }
+}

crates/tapline-auth/src/store.rs

@@ -256,11 +256,31 @@ fn resolve_config_dir(configured: Option<PathBuf>, home: Option<PathBuf>) -> 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");

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::<SECURITY_ATTRIBUTES>() as u32;
@@ -62,6 +62,15 @@ pub fn current_user_sid() -> io::Result<String> {
     // 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::<TOKEN_USER>() {
+        // 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::<TOKEN_USER>());
@@ -73,12 +82,20 @@ pub fn current_user_sid() -> io::Result<String> {
     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<File> {
     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<File> {
             GENERIC_WRITE,
             0,
             &attributes,
-            CREATE_ALWAYS,
+            CREATE_NEW,
             FILE_ATTRIBUTE_NORMAL,
             ptr::null_mut(),
         )