From f29598af3c3ce2cc8185bfa99cd30536130d79a0 Mon Sep 17 00:00:00 2001
From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com>
Date: Mon, 24 Aug 2026 02:02:34 +0800
Subject: [PATCH] harden(kitty): bound the image allocation on its own, not on
 the check above it
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`read_file` refuses a file larger than a frame on its metadata, then reads
through `take` and refuses again on what came back. The second refusal is
what makes the *read* safe, so the first looks like a fast path — and it is,
except for one thing it was also quietly doing: `Vec::with_capacity` was
reserving `meta.len()` verbatim.

`name` arrives in an escape sequence, so that length is an attacker's number
and costs nothing to make enormous — a sparse file is one `set_len`. Between
the two caps, a file that is merely *named* could still ask for its own size
in memory before a byte of it was read. Reserving `min(len, MAX_IMAGE_BYTES)`
makes the bound the allocation's own, so it survives whatever happens to the
check above it.

The new test pins the refusal of an oversized file (sparse, so it costs
nothing) and that a small one is still read. It does not distinguish the two
caps — with the metadata check removed it still passes, because `take` gets
there — and that is said here rather than implied, since the allocation is
the part no test can see.
---
 crates/tty7-core/src/core/kitty_graphics.rs | 44 ++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/crates/tty7-core/src/core/kitty_graphics.rs b/crates/tty7-core/src/core/kitty_graphics.rs
index 7152d71f..cd304aa6 100644
--- a/crates/tty7-core/src/core/kitty_graphics.rs
+++ b/crates/tty7-core/src/core/kitty_graphics.rs
@@ -953,7 +953,13 @@ impl MediumTransfer {
         if !meta.is_file() || meta.len() as usize > MAX_IMAGE_BYTES {
             return None;
         }
-        let mut bytes = Vec::with_capacity(meta.len() as usize);
+        // Capped a second time on the way into the allocation, and not because
+        // the check above is in doubt. `name` is an arbitrary path out of an
+        // escape sequence, so `meta.len()` is an attacker's number: reserving
+        // it verbatim is the one place a file that is merely *named* — never
+        // read, since `take` below bounds that — can still ask for its own size
+        // in memory. A hundred-gigabyte sparse file costs nothing to create.
+        let mut bytes = Vec::with_capacity((meta.len() as usize).min(MAX_IMAGE_BYTES));
         file.take(MAX_IMAGE_BYTES as u64 + 1)
             .read_to_end(&mut bytes)
             .ok()?;
@@ -1749,6 +1755,42 @@ mod tests {
         }
     }
 
+    /// A file bigger than the wire could carry is refused on its size alone,
+    /// before any of it is read.
+    ///
+    /// `name` comes out of an escape sequence, so the size is an attacker's
+    /// number and costs nothing to make enormous — a sparse file is a single
+    /// `set_len`. The read below the check is bounded by `take` either way, so
+    /// what the check is really holding is the allocation that reserves the
+    /// file's own length; that is capped a second time now, and this pins the
+    /// refusal itself.
+    #[test]
+    #[cfg(unix)]
+    fn a_file_larger_than_a_frame_is_refused_on_its_size() {
+        let dir = std::env::temp_dir();
+        let path = dir.join(format!("tty7-kitty-huge-{}.rgba", std::process::id()));
+        let file = std::fs::File::create(&path).unwrap();
+        // Sparse: no blocks are allocated, so this is as cheap as an empty file.
+        file.set_len(MAX_IMAGE_BYTES as u64 + 1).unwrap();
+        drop(file);
+
+        assert!(
+            temp_file_transfer(&path).resolve().is_none(),
+            "a file past the frame bound is not an image"
+        );
+
+        // One byte under the bound is read, so the refusal is the size and not
+        // something else about the file.
+        let file = std::fs::File::options().write(true).open(&path).unwrap();
+        file.set_len(8).unwrap();
+        drop(file);
+        assert!(
+            temp_file_transfer(&path).resolve().is_some(),
+            "the same file within the bound is read"
+        );
+        let _ = std::fs::remove_file(&path);
+    }
+
     /// A transfer naming a fifo is refused, and refused *promptly*.
     ///
     /// `open(O_RDONLY)` on a fifo blocks until a writer arrives, and for one