Harden pack extraction, registry pulls, secret resolution, and published-socket collisions

crates/smolvm-pack/src/assets.rs

@@ -664,8 +664,12 @@ pub fn decompress_assets(compressed: &[u8], output_dir: &Path) -> Result<()> {
         .map_err(|e| PackError::Compression(e.to_string()))?;
     let mut archive = tar::Archive::new(decoder);
 
-    archive
-        .unpack(output_dir)
+    // Route through safe_unpack (not the raw tar `unpack`): a pulled pack's
+    // asset blob is untrusted, and its libs are later dlopen'd — the raw path
+    // would create escaping symlinks (e.g. `lib/libkrun → /tmp/evil.so`) and
+    // has no entry/size caps. safe_unpack rejects symlinks/hardlinks/traversal
+    // and bounds the extraction.
+    crate::extract::safe_unpack(&mut archive, output_dir)
         .map_err(|e| PackError::Tar(e.to_string()))?;
 
     Ok(())
@@ -680,8 +684,9 @@ pub fn decompress_assets_from_file(compressed_path: &Path, output_dir: &Path) ->
         zstd::stream::Decoder::new(file).map_err(|e| PackError::Compression(e.to_string()))?;
     let mut archive = tar::Archive::new(decoder);
 
-    archive
-        .unpack(output_dir)
+    // See decompress_assets: use safe_unpack so an untrusted pack can't plant
+    // escaping symlinks or blow past the resource caps.
+    crate::extract::safe_unpack(&mut archive, output_dir)
         .map_err(|e| PackError::Tar(e.to_string()))?;
 
     Ok(())
@@ -737,6 +742,34 @@ pub fn crc32_file_range(path: &Path, offset: u64, size: u64) -> Result<u32> {
 mod tests {
     use super::*;
 
+    // A pulled pack's asset blob is untrusted; decompress must go through
+    // safe_unpack, which rejects a symlink that escapes the output dir (the raw
+    // tar `unpack` would create it, enabling a `lib/... -> /evil` dlopen swap).
+    #[test]
+    fn decompress_assets_rejects_escaping_symlink() {
+        let mut builder = tar::Builder::new(Vec::new());
+        let mut header = tar::Header::new_gnu();
+        header.set_entry_type(tar::EntryType::Symlink);
+        header.set_size(0);
+        header.set_mode(0o777);
+        builder
+            .append_link(&mut header, "evil", "../../../../../../etc/passwd")
+            .unwrap();
+        let tar_bytes = builder.into_inner().unwrap();
+        let compressed = zstd::encode_all(&tar_bytes[..], ZSTD_LEVEL).unwrap();
+
+        let tmp = tempfile::tempdir().unwrap();
+        let out = tmp.path().join("out");
+        assert!(
+            decompress_assets(&compressed, &out).is_err(),
+            "escaping symlink must be rejected"
+        );
+        assert!(
+            std::fs::symlink_metadata(out.join("evil")).is_err(),
+            "no escaping symlink should be created"
+        );
+    }
+
     #[test]
     fn digest_to_filename_rejects_path_traversal() {
         // A valid hex digest is accepted.

crates/smolvm-pack/src/detect.rs

@@ -42,6 +42,18 @@ pub enum PackedMode {
     },
 }
 
+/// Minimum section byte length required to hold the header plus a manifest and
+/// assets of the declared sizes, or `None` if the sum overflows `usize` (a
+/// crafted `assets_size` must not wrap the bound into a small value that a
+/// later `size >= expected` check would accept). Pure and platform-independent
+/// so the overflow guard is unit-testable off macOS.
+#[cfg(any(target_os = "macos", test))]
+fn embedded_section_extent(manifest_size: u32, assets_size: u64) -> Option<usize> {
+    crate::format::SECTION_HEADER_SIZE
+        .checked_add(manifest_size as usize)
+        .and_then(|s| s.checked_add(usize::try_from(assets_size).ok()?))
+}
+
 /// Detect whether this process is running as a packed binary.
 ///
 /// Checks in order:
@@ -203,12 +215,15 @@ fn read_embedded_section() -> Option<EmbeddedData> {
         let header_bytes = std::slice::from_raw_parts(data_ptr, SECTION_HEADER_SIZE);
         let section_header = SectionHeader::from_bytes(header_bytes).ok()?;
 
-        // Validate sizes
-        let expected_size = SECTION_HEADER_SIZE
-            + section_header.manifest_size as usize
-            + section_header.assets_size as usize;
-        if size < expected_size {
-            return None;
+        // Validate sizes with checked arithmetic. A crafted or corrupt
+        // `assets_size` (u64) could otherwise overflow the sum, wrapping the
+        // expected size small so `size <` passes and a bogus `assets_size`
+        // reaches the raw `assets_ptr` below as an out-of-bounds length. The
+        // file-parsing path (`packer.rs::validate_footer`) already uses
+        // `checked_add`; keep them consistent.
+        match embedded_section_extent(section_header.manifest_size, section_header.assets_size) {
+            Some(exp) if size >= exp => {}
+            _ => return None,
         }
 
         // Parse manifest
@@ -233,6 +248,19 @@ fn read_embedded_section() -> Option<EmbeddedData> {
 mod tests {
     use super::*;
 
+    // A crafted assets_size (u64 near max) must not overflow the section-extent
+    // sum: an unchecked `+` wraps it small, defeating the `size >= expected`
+    // bound and handing a huge assets_size to the raw pointer read.
+    #[test]
+    fn embedded_section_extent_rejects_overflow() {
+        assert_eq!(
+            embedded_section_extent(100, 200),
+            Some(crate::format::SECTION_HEADER_SIZE + 300)
+        );
+        assert_eq!(embedded_section_extent(100, u64::MAX), None);
+        assert_eq!(embedded_section_extent(u32::MAX, u64::MAX), None);
+    }
+
     #[test]
     fn test_detect_returns_none_for_normal_binary() {
         // The test binary is a normal Rust executable, not a packed binary.

crates/smolvm-pack/src/extract.rs

@@ -274,7 +274,10 @@ fn verify_parent_within_dest(path: &Path, real_dest: &Path) -> std::io::Result<(
 /// `lib/libkrun.dylib → /tmp/evil.so`, and subsequent `dlopen()` would
 /// load the attacker's library. This function rejects any entry that is
 /// not a regular file or directory.
-fn safe_unpack<R: Read>(archive: &mut tar::Archive<R>, dest: &Path) -> std::io::Result<()> {
+pub(crate) fn safe_unpack<R: Read>(
+    archive: &mut tar::Archive<R>,
+    dest: &Path,
+) -> std::io::Result<()> {
     safe_unpack_with_limits(archive, dest, &SafeUnpackLimits::from_env())
 }
 

crates/smolvm-pack/src/macho.rs

@@ -443,16 +443,41 @@ impl MachoFile {
             ));
         }
 
-        // Read load commands
-        let mut load_commands = Vec::with_capacity(header.ncmds as usize);
+        // Read load commands. Cap the pre-allocation by the byte budget (each
+        // command is at least LoadCommand::SIZE), so a bogus `ncmds` can't demand
+        // a huge allocation before a single byte is validated.
+        let mut load_commands =
+            Vec::with_capacity((header.ncmds as usize).min(data.len() / LoadCommand::SIZE + 1));
         for _ in 0..header.ncmds {
             let cmd_start = cursor.position() as usize;
             let lc = LoadCommand::read(&mut cursor)?;
 
+            // Validate the declared command size before trusting it: at least its
+            // own 8-byte header (a smaller value underflows `cmdsize - SIZE` below
+            // into an enormous allocation) and within the file (so the fallback
+            // `vec![0u8; remaining]` and `set_position` stay bounded by the input).
+            let cmd_end = cmd_start.checked_add(lc.cmdsize as usize);
+            if (lc.cmdsize as usize) < LoadCommand::SIZE
+                || cmd_end.is_none_or(|end| end > data.len())
+            {
+                return Err(io::Error::new(
+                    io::ErrorKind::InvalidData,
+                    format!(
+                        "malformed Mach-O load command 0x{:x}: cmdsize {}",
+                        lc.cmd, lc.cmdsize
+                    ),
+                ));
+            }
+
             let parsed = match lc.cmd {
                 LC_SEGMENT_64 => {
                     let segment = SegmentCommand64::read(&mut cursor, lc.cmd, lc.cmdsize)?;
-                    let mut sections = Vec::with_capacity(segment.nsects as usize);
+                    // Cap by the byte budget: `nsects` is attacker-controlled and
+                    // each section is Section64::SIZE bytes, so an inflated count
+                    // must not drive the pre-allocation.
+                    let mut sections = Vec::with_capacity(
+                        (segment.nsects as usize).min(data.len() / Section64::SIZE + 1),
+                    );
                     for _ in 0..segment.nsects {
                         sections.push(Section64::read(&mut cursor)?);
                     }
@@ -1162,6 +1187,43 @@ mod tests {
         assert_eq!(header.ncmds, parsed.ncmds);
     }
 
+    fn header_with_one_cmd() -> Vec<u8> {
+        let header = MachHeader64 {
+            magic: MH_MAGIC_64,
+            cputype: 0x0100000c, // ARM64
+            cpusubtype: 0,
+            filetype: 2, // MH_EXECUTE
+            ncmds: 1,
+            sizeofcmds: 0,
+            flags: 0,
+            reserved: 0,
+        };
+        let mut buf = Vec::new();
+        header.write(&mut buf).unwrap();
+        buf
+    }
+
+    // A load command claiming cmdsize < its own 8-byte header would underflow
+    // `cmdsize - SIZE` into an enormous allocation; parse must reject it cleanly
+    // so the packer's "not a Mach-O, fall back" path works instead of aborting.
+    #[test]
+    fn parse_rejects_undersized_load_command() {
+        let mut buf = header_with_one_cmd();
+        buf.extend_from_slice(&0x99u32.to_le_bytes()); // unknown cmd
+        buf.extend_from_slice(&4u32.to_le_bytes()); // cmdsize = 4 (< 8)
+        assert!(MachoFile::parse(&buf).is_err());
+    }
+
+    // A cmdsize that runs past the end of the file must error rather than drive a
+    // multi-gigabyte `vec![0u8; cmdsize - SIZE]` allocation.
+    #[test]
+    fn parse_rejects_out_of_bounds_cmdsize() {
+        let mut buf = header_with_one_cmd();
+        buf.extend_from_slice(&0x99u32.to_le_bytes()); // unknown cmd
+        buf.extend_from_slice(&u32::MAX.to_le_bytes()); // cmdsize ~4 GiB, no data
+        assert!(MachoFile::parse(&buf).is_err());
+    }
+
     #[test]
     fn test_page_align() {
         // Test exact page boundaries

crates/smolvm-pack/src/packer.rs

@@ -858,7 +858,7 @@ mod tests {
 
         // Add a fake layer (digest must be at least 12 chars after sha256:)
         collector
-            .add_layer("sha256:embedded123456", b"embedded layer content")
+            .add_layer("sha256:beefcafe1234", b"embedded layer content")
             .unwrap();
 
         // Create manifest
@@ -879,7 +879,7 @@ mod tests {
         // Verify we can read the manifest with layer info
         let manifest = read_manifest(&output_path).unwrap();
         assert_eq!(manifest.assets.layers.len(), 1);
-        assert_eq!(manifest.assets.layers[0].digest, "sha256:embedded123456");
+        assert_eq!(manifest.assets.layers[0].digest, "sha256:beefcafe1234");
 
         // Verify footer indicates embedded mode
         let footer = read_footer(&output_path).unwrap();
@@ -890,7 +890,7 @@ mod tests {
         let extract_dir = temp_dir.path().join("extracted");
         extract_assets(&output_path, &extract_dir).unwrap();
 
-        let layer_file = extract_dir.join("layers/embedded1234.tar"); // First 12 chars
+        let layer_file = extract_dir.join("layers/beefcafe1234.tar"); // First 12 chars
         assert!(layer_file.exists());
         assert_eq!(
             fs::read_to_string(&layer_file).unwrap(),

crates/smolvm-registry/src/cache.rs

@@ -59,6 +59,12 @@ impl BlobCache {
         Ok(Self { root, max_size })
     }
 
+    /// The cache's byte cap — an absolute upper bound for a single blob, used to
+    /// bound a streamed download when the manifest declares no usable size.
+    pub fn max_size(&self) -> u64 {
+        self.max_size
+    }
+
     /// Look up a blob by digest. Returns the path if it exists.
     pub fn get(&self, digest: &str) -> Option<PathBuf> {
         let path = self.blob_path(digest);

crates/smolvm-registry/src/peer.rs

@@ -116,13 +116,14 @@ pub(crate) async fn fetch_blob_from_peers(
     client: &reqwest::Client,
     peers: &[String],
     digest: &str,
+    max_bytes: u64,
     output: Option<&Path>,
     cache: &BlobCache,
 ) -> Option<PullResult> {
     let partial_path = cache.blob_path_for(digest).with_extension("partial");
 
     for peer in peers {
-        match fetch_one(client, peer, digest, output, cache).await {
+        match fetch_one(client, peer, digest, max_bytes, output, cache).await {
             Ok(result) => {
                 tracing::info!(peer = %peer, digest = %digest, "fetched layer blob from peer (P2P)");
                 return Some(result);
@@ -149,6 +150,7 @@ async fn fetch_one(
     client: &reqwest::Client,
     peer: &str,
     digest: &str,
+    max_bytes: u64,
     output: Option<&Path>,
     cache: &BlobCache,
 ) -> Result<PullResult> {
@@ -167,7 +169,7 @@ async fn fetch_one(
 
     // Identical verification + cache accounting to the registry path: the digest
     // is re-checked while streaming, so a lying peer can never poison the cache.
-    stream_verify_adopt(resp.bytes_stream(), digest, output, cache).await
+    stream_verify_adopt(resp.bytes_stream(), digest, max_bytes, output, cache).await
 }
 
 #[cfg(test)]
@@ -210,7 +212,7 @@ mod tests {
         mount_blob(&peer, &digest, 200, data.clone()).await;
 
         let client = reqwest::Client::new();
-        let result = fetch_blob_from_peers(&client, &[peer.uri()], &digest, None, &cache)
+        let result = fetch_blob_from_peers(&client, &[peer.uri()], &digest, u64::MAX, None, &cache)
             .await
             .expect("peer hit must yield a result");
 
@@ -237,7 +239,7 @@ mod tests {
 
         let client = reqwest::Client::new();
         let result =
-            fetch_blob_from_peers(&client, &[miss.uri(), hit.uri()], &digest, None, &cache)
+            fetch_blob_from_peers(&client, &[miss.uri(), hit.uri()], &digest, u64::MAX, None, &cache)
                 .await
                 .expect("second peer must satisfy the fetch");
         assert_eq!(result.size, data.len() as u64);
@@ -257,7 +259,7 @@ mod tests {
 
         let client = reqwest::Client::new();
         let result =
-            fetch_blob_from_peers(&client, &[liar.uri(), honest.uri()], &digest, None, &cache)
+            fetch_blob_from_peers(&client, &[liar.uri(), honest.uri()], &digest, u64::MAX, None, &cache)
                 .await
                 .expect("honest peer must satisfy the fetch");
         assert_eq!(result.size, data.len() as u64);
@@ -293,7 +295,7 @@ mod tests {
             .build()
             .unwrap();
         let result =
-            fetch_blob_from_peers(&client, &[slow.uri(), fast.uri()], &digest, None, &cache)
+            fetch_blob_from_peers(&client, &[slow.uri(), fast.uri()], &digest, u64::MAX, None, &cache)
                 .await
                 .expect("fast peer must satisfy the fetch after the slow one times out");
         assert_eq!(result.size, data.len() as u64);
@@ -313,7 +315,7 @@ mod tests {
 
         let client = reqwest::Client::new();
         let result =
-            fetch_blob_from_peers(&client, &[a.uri(), b.uri()], &digest, None, &cache).await;
+            fetch_blob_from_peers(&client, &[a.uri(), b.uri()], &digest, u64::MAX, None, &cache).await;
         assert!(result.is_none(), "all-peers-fail must return None");
         assert!(cache.get(&digest).is_none(), "cache must stay empty");
         let partial = cache.blob_path_for(&digest).with_extension("partial");
@@ -334,7 +336,7 @@ mod tests {
         let client = reqwest::Client::new();
         // Port 1 has nothing listening → connection refused.
         let dead = "http://127.0.0.1:1".to_string();
-        let result = fetch_blob_from_peers(&client, &[dead, hit.uri()], &digest, None, &cache)
+        let result = fetch_blob_from_peers(&client, &[dead, hit.uri()], &digest, u64::MAX, None, &cache)
             .await
             .expect("live peer must satisfy the fetch");
         assert_eq!(result.size, data.len() as u64);