Security, performance and dead-code audit

SECURITY.md

@@ -16,17 +16,22 @@ how much a report matters:
   controlled. `..`, absolute paths, or a symlink that redirects a later write
   should all fail the install, not be skipped.
 - **Content accepted without verification.** Every chunk's SHA-1 is checked
-  against its content-addressed id before it reaches the disk. Anything that
-  writes bytes that were not checked is a hole, and it is what makes fetching
-  over a plain-HTTP lancache safe.
+  against its content-addressed id before it reaches the disk, and a manifest
+  must name the depot and manifest id that were asked for. Anything that writes
+  bytes that were not checked is a hole. Manifest signatures are not verified:
+  manifests, like everything else, are only ever fetched over TLS, and tapline
+  does not speak plain HTTP, so it cannot use a plain-HTTP lancache. Legacy
+  (pre-SteamPipe) Workshop files come with no hash; their declared size is
+  checked instead.
 - **A credential leaving where it should not.** Passwords are encrypted with
   Steam's RSA key and never written to disk; only the refresh token persists.
   A password reaching a log, an argument list, an error message or the token
   file is a bug.
 - **Anything executed from downloaded content.** Depots ship
   `installscript.vdf`. tapline parses and reports it and must never run it —
   installing a game server is not a remote-code-execution primitive.
-- **Decompression that is not bounded** by the size the manifest declares.
+- **Decompression that is not bounded** by the size the manifest or archive
+  declares, or that holds a whole entry in memory to do it.
 
 ## What is not a vulnerability
 

crates/tapline-auth/src/local.rs

@@ -33,9 +33,7 @@ pub fn home_relative_roots() -> &'static [&'static str] {
 /// 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.
+/// a Steam install has to ask for both, or it finds nothing on Windows.
 fn home() -> Option<PathBuf> {
     std::env::var_os("HOME")
         .or_else(|| std::env::var_os("USERPROFILE"))
@@ -206,8 +204,8 @@ mod tests {
 
     #[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.
+        // No home directory is the ordinary case on Windows when only HOME is
+        // consulted, so this must not give up.
         let roots = roots(None);
         assert_eq!(
             roots.len(),

crates/tapline-auth/src/sddl.rs

@@ -119,9 +119,8 @@ mod tests {
 
     const OWNER: &str = "S-1-5-21-1111111111-2222222222-3333333333-1001";
 
-    /// What the comparison used to be, before Windows' alias substitution made
-    /// a real SID comparison necessary. It is still what every case below but
-    /// the alias one needs.
+    /// A plain string comparison of SIDs, without the alias handling the real
+    /// one needs. Every case below but the alias one only needs this.
     fn owned_by(owner: &str) -> impl Fn(&str) -> bool + '_ {
         move |holder| holder.eq_ignore_ascii_case(owner)
     }

crates/tapline-auth/src/store.rs

@@ -193,23 +193,28 @@ impl TokenStore {
 }
 
 fn read_all(path: &Path) -> Result<Vec<(String, String)>, TokenStoreError> {
-    let text = match std::fs::read_to_string(path) {
+    let mut text = match std::fs::read_to_string(path) {
         Ok(text) => text,
         Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()),
         Err(error) => return Err(TokenStoreError::Backend(error.to_string())),
     };
 
-    check_permissions(path)?;
-
-    let mut out = Vec::new();
-    for line in text.lines() {
-        if line.trim().is_empty() {
-            continue;
-        }
-        let (account, token) = line.split_once('\t').ok_or(TokenStoreError::Malformed)?;
-        out.push((account.to_owned(), token.to_owned()));
+    if let Err(error) = check_permissions(path) {
+        text.zeroize();
+        return Err(error);
     }
-    Ok(out)
+
+    let entries = text
+        .lines()
+        .filter(|line| !line.trim().is_empty())
+        .map(|line| {
+            line.split_once('\t')
+                .map(|(account, token)| (account.to_owned(), token.to_owned()))
+                .ok_or(TokenStoreError::Malformed)
+        })
+        .collect();
+    text.zeroize();
+    entries
 }
 
 fn write_all(path: &Path, entries: &[(String, String)]) -> Result<(), TokenStoreError> {
@@ -364,6 +369,14 @@ fn check_permissions(path: &Path) -> Result<(), TokenStoreError> {
 }
 
 fn write_file(path: &Path, token: &StoredToken) -> Result<(), TokenStoreError> {
+    // The file is one `account<TAB>token` line per entry; either separator in a
+    // field would corrupt every other entry on the next read.
+    if [&token.account, &token.refresh_token]
+        .iter()
+        .any(|field| field.contains(['\t', '\n', '\r']))
+    {
+        return Err(TokenStoreError::Malformed);
+    }
     let mut entries = read_all(path)?;
     entries.retain(|(account, _)| account != &token.account);
     entries.push((token.account.clone(), token.refresh_token.clone()));

crates/tapline-cdn/src/lib.rs

@@ -70,6 +70,16 @@ pub async fn fetch_chunk_bytes<F: Fetch>(
     depot: DepotId,
     chunk: &Chunk,
 ) -> Result<Vec<u8>, CdnError> {
+    // The stored form is the compressed container plus AES padding and IV; a
+    // manifest claiming more than that of the largest chunk would otherwise
+    // set how much the HTTP client is willing to buffer.
+    if chunk.compressed_size as usize > tapline_chunk::MAX_CHUNK + 4096 {
+        return Err(CdnError::Container(format!(
+            "the manifest claims a {}-byte chunk",
+            chunk.compressed_size
+        )));
+    }
+
     let id = chunk.id_hex();
     let url = format!("https://{host}/depot/{depot}/chunk/{id}");
 
@@ -125,10 +135,19 @@ pub fn decode_chunk_into(
     let container =
         tapline_crypto::decrypt_content_owned(depot_key, stored).map_err(|_| CdnError::Decrypt)?;
 
-    tapline_chunk::decode_into(&container, tapline_chunk::MAX_CHUNK, plaintext)
+    // Bounded by what the manifest declares, not only by the format's ceiling.
+    let limit = (chunk.uncompressed_size as usize).min(tapline_chunk::MAX_CHUNK);
+    tapline_chunk::decode_into(&container, limit, plaintext)
         .map_err(|e| CdnError::Container(e.to_string()))?;
     let plaintext = &*plaintext;
 
+    if plaintext.len() != chunk.uncompressed_size as usize {
+        return Err(CdnError::WrongLength {
+            expected: chunk.uncompressed_size,
+            actual: plaintext.len(),
+        });
+    }
+
     let digest = tapline_crypto::sha1(plaintext);
     if digest != chunk.id {
         return Err(CdnError::IntegrityFailure {
@@ -138,13 +157,6 @@ pub fn decode_chunk_into(
         });
     }
 
-    if plaintext.len() != chunk.uncompressed_size as usize {
-        return Err(CdnError::WrongLength {
-            expected: chunk.uncompressed_size,
-            actual: plaintext.len(),
-        });
-    }
-
     Ok(())
 }
 
@@ -173,6 +185,12 @@ pub async fn fetch_manifest<F: Fetch>(
     let manifest = Manifest::parse(&response.body, depot_key)
         .map_err(|e| CdnError::Container(e.to_string()))?;
 
+    if manifest.depot.get() != 0 && manifest.depot != depot {
+        return Err(CdnError::Container(format!(
+            "asked for a manifest of depot {depot}, received one of {}",
+            manifest.depot
+        )));
+    }
     if manifest.id.get() != manifest_id {
         return Err(CdnError::Container(format!(
             "asked for manifest {manifest_id}, received {}",

crates/tapline-chunk/src/lib.rs

@@ -79,9 +79,7 @@ pub fn decode_into(input: &[u8], max_output: usize, out: &mut Vec<u8>) -> Result
         return vz::decode_into(input, max_output, out);
     }
     if zip::matches(input) {
-        let decoded = zip::decode(input, max_output)?;
-        out.clear();
-        out.extend_from_slice(&decoded);
+        *out = zip::decode(input, max_output)?;
         return Ok(());
     }
 

crates/tapline-ext/src/lib.rs

@@ -126,6 +126,12 @@ pub trait EntrySink {
 
     fn end(&mut self) -> Result<(), ExtensionError>;
 
+    /// Called instead of `end` when an entry could not be decoded, so a sink
+    /// can throw away what it wrote of it.
+    fn abort(&mut self) -> Result<(), ExtensionError> {
+        Ok(())
+    }
+
     fn finish(&mut self) -> Result<(), ExtensionError> {
         Ok(())
     }
@@ -144,6 +150,9 @@ impl<S: EntrySink + ?Sized> EntrySink for &mut S {
     fn end(&mut self) -> Result<(), ExtensionError> {
         (**self).end()
     }
+    fn abort(&mut self) -> Result<(), ExtensionError> {
+        (**self).abort()
+    }
     fn finish(&mut self) -> Result<(), ExtensionError> {
         (**self).finish()
     }
@@ -162,6 +171,9 @@ impl<S: EntrySink + ?Sized> EntrySink for Box<S> {
     fn end(&mut self) -> Result<(), ExtensionError> {
         (**self).end()
     }
+    fn abort(&mut self) -> Result<(), ExtensionError> {
+        (**self).abort()
+    }
     fn finish(&mut self) -> Result<(), ExtensionError> {
         (**self).finish()
     }

crates/tapline-ffi/src/lib.rs

@@ -62,10 +62,10 @@ fn pool() -> Option<&'static std::sync::Arc<SessionPool>> {
     static POOL: OnceLock<std::sync::Arc<SessionPool>> = 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.
+        // the latch belongs. Set any later, asking for the current budget --
+        // which builds the pool -- would leave the latch down, and the next
+        // `tapline_set_total_concurrency` would answer 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 {
@@ -787,12 +787,12 @@ pub unsafe extern "C" fn tapline_job_next(
         return TAPLINE_BAD_ARGUMENT;
     };
 
-    // 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.
+    // One lock for the whole function. With separate critical sections for
+    // taking the held slot, reading the queue and putting the message back, 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;
@@ -883,9 +883,8 @@ pub unsafe extern "C" fn tapline_last_error(buf: *mut u8, cap: usize, out_len: *
             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.
+            // Nothing to copy. No error set is the ordinary case, and asking
+            // for its length with a null buffer is not a buffer too small.
             return TAPLINE_OK;
         }
         // SAFETY: `buf` is non-null and writable for `cap` bytes per the

crates/tapline-fs/src/file.rs

@@ -128,6 +128,19 @@ pub fn remove_existing(path: &Path) -> io::Result<()> {
     }
 }
 
+/// Removes `path` if it is a symlink, leaving what it points at alone.
+///
+/// For a path the manifest now names as a file or directory where an earlier
+/// install left a link: writing through the link would land at its target.
+pub fn remove_symlink(path: &Path) -> io::Result<()> {
+    match std::fs::symlink_metadata(path) {
+        Ok(metadata) if metadata.file_type().is_symlink() => remove_existing(path),
+        Ok(_) => Ok(()),
+        Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()),
+        Err(e) => Err(e),
+    }
+}
+
 #[cfg(unix)]
 fn names_a_directory(file_type: &std::fs::FileType) -> bool {
     file_type.is_dir()
@@ -281,6 +294,25 @@ mod tests {
         );
     }
 
+    #[test]
+    fn remove_symlink_takes_only_links() {
+        let scratch = Scratch::new("file-remove-symlink");
+        let target = scratch.join("real.txt");
+        std::fs::write(&target, b"kept").expect("seed");
+        let link = scratch.join("link.txt");
+        symlink(Path::new("real.txt"), &link).expect("symlink");
+
+        remove_symlink(&target).expect("a regular file is left alone");
+        remove_symlink(&link).expect("remove");
+        remove_symlink(&scratch.join("never-existed")).expect("nothing to remove");
+
+        assert!(
+            std::fs::symlink_metadata(&link).is_err(),
+            "the link is still there"
+        );
+        assert_eq!(std::fs::read(&target).expect("read"), b"kept");
+    }
+
     #[test]
     fn removing_a_path_that_is_not_there_is_not_an_error() {
         let scratch = Scratch::new("file-remove-missing");

crates/tapline-fs/src/lib.rs

@@ -1,5 +1,5 @@
 mod file;
 mod path;
 
-pub use file::{read_exact_at, remove_existing, set_mode, symlink, write_all_at};
+pub use file::{read_exact_at, remove_existing, remove_symlink, set_mode, symlink, write_all_at};
 pub use path::{PathError, SafePath, validate_path, validate_symlink};