From ab5e83ffd10e1b55c7389cffa15f72c41fcf4c45 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 21:47:13 -0400 Subject: [PATCH 1/2] security: bound decoded HTTP responses and screenshot decoding (F-006, F-008, F-009) --- crates/core/src/details.rs | 41 +++++++++++++++++++++++---- crates/native/src/http.rs | 58 ++++++++++++++++++++++++++++++++------ 2 files changed, 86 insertions(+), 13 deletions(-) diff --git a/crates/core/src/details.rs b/crates/core/src/details.rs index 333f195b..c6ee8c7c 100644 --- a/crates/core/src/details.rs +++ b/crates/core/src/details.rs @@ -83,10 +83,11 @@ pub fn jpeg_dimensions(bytes: &[u8]) -> Option<(u16, u16)> { return None; } let mut i = 2; // past the SOI (FF D8). + let mut dims = None; while i + 1 < bytes.len() { // Markers are FF, with any number of FF fill bytes before the type byte. if bytes[i] != 0xFF { - return None; + break; } let mut j = i; while j < bytes.len() && bytes[j] == 0xFF { @@ -97,21 +98,32 @@ pub fn jpeg_dimensions(bytes: &[u8]) -> Option<(u16, u16)> { match marker { // Standalone markers with no length: RSTn, SOI, EOI, TEM. 0x01 | 0xD0..=0xD9 => continue, + // SOS (Start of Scan): marker headers end, entropy-coded scan data begins. + 0xDA => break, _ => {} } let len = u16::from_be_bytes([*bytes.get(i)?, *bytes.get(i + 1)?]) as usize; if len < 2 { return None; } - // SOF0..SOF15, excluding DHT (C4), DAC (CC) and the RSTn range already handled. - if matches!(marker, 0xC0..=0xCF) && !matches!(marker, 0xC4 | 0xC8 | 0xCC) { + // Only SOF0 (0xC0: baseline), SOF1 (0xC1: extended sequential), and SOF2 (0xC2: progressive) + // are supported by standard image rendering decoders. + if matches!(marker, 0xC0..=0xC2) { + if dims.is_some() { + // Ambiguous / multiple frame markers: reject to prevent frame smuggling (CWE-789). + return None; + } let h = u16::from_be_bytes([*bytes.get(i + 3)?, *bytes.get(i + 4)?]); let w = u16::from_be_bytes([*bytes.get(i + 5)?, *bytes.get(i + 6)?]); - return Some((w, h)); + dims = Some((w, h)); + } else if matches!(marker, 0xC3 | 0xC5..=0xCF) && !matches!(marker, 0xC4 | 0xC8 | 0xCC) { + // Unsupported frame marker (e.g. lossless 0xC3): the rendering decoder will skip it or fail, + // so accepting its dimensions would let a subsequent large frame bypass the budget (CWE-789). + return None; } i += len; } - None + dims } /// Whether `bytes` is a JPEG whose declared dimensions are within [`MAX_SCREENSHOT_PIXELS`], so it @@ -552,6 +564,25 @@ mod tests { // Not a JPEG, or no frame header present. assert_eq!(jpeg_dimensions(b""), None); assert_eq!(jpeg_dimensions(&[0xFF, 0xD8]), None); + + // Unsupported C3 frame marker is rejected (CWE-789). + let mut c3 = vec![0xFF, 0xD8, 0xFF, 0xC3, 0x00, 0x11, 0x08]; + c3.extend_from_slice(&100u16.to_be_bytes()); + c3.extend_from_slice(&100u16.to_be_bytes()); + c3.push(0x03); + assert_eq!(jpeg_dimensions(&c3), None); + + // Multiple / conflicting SOF markers are rejected. + let mut multi = vec![0xFF, 0xD8]; + multi.extend_from_slice(&[0xFF, 0xC0, 0x00, 0x0B, 0x08]); + multi.extend_from_slice(&100u16.to_be_bytes()); + multi.extend_from_slice(&100u16.to_be_bytes()); + multi.extend_from_slice(&[0x01, 0x00, 0x00, 0x00]); + multi.extend_from_slice(&[0xFF, 0xC0, 0x00, 0x0B, 0x08]); + multi.extend_from_slice(&9000u16.to_be_bytes()); + multi.extend_from_slice(&9000u16.to_be_bytes()); + multi.extend_from_slice(&[0x01, 0x00, 0x00, 0x00]); + assert_eq!(jpeg_dimensions(&multi), None); } #[test] diff --git a/crates/native/src/http.rs b/crates/native/src/http.rs index 2132d2f1..7a6073aa 100644 --- a/crates/native/src/http.rs +++ b/crates/native/src/http.rs @@ -171,9 +171,9 @@ impl Http { if announced.is_some_and(|n| n > max as u64) { return Err(too_big()); } - // ureq errors once the limit is reached, so ask for one byte more than we accept. - let body = resp.body_mut().with_config().limit(max as u64 + 1).read_to_vec().map_err(|e| format!("unreadable answer ({e})"))?; - if body.len() > max { Err(too_big()) } else { Ok(Some(body)) } + let body = read_bytes_capped(&mut resp, max) + .map_err(|e| if e.contains("larger than") { too_big() } else { format!("unreadable answer ({e})") })?; + Ok(Some(body)) } 404 => Ok(None), s => Err(format!("{} answered HTTP {s}", host(url))), @@ -312,6 +312,15 @@ impl Http { if size > 0 && total != size { return Err(AttemptError::Fatal(format!("the server offered {total} bytes, but the release lists {size}"))); } + if let Some(enc) = header("content-encoding") + && !enc.eq_ignore_ascii_case("identity") + { + return Err(AttemptError::Fatal(format!("unexpected Content-Encoding '{enc}' on binary artifact download"))); + } + let limit = if total > 0 { total } else { MAX_DOWNLOAD }; + if have > limit { + return Err(AttemptError::Fatal(format!("the download exceeds the permitted size of {limit} bytes"))); + } // Open the part file at the right place: a 200 restarts it, a 206 continues it. let mut file = match (status, start) { (206, s) if s == have => std::fs::OpenOptions::new().append(true).open(part), @@ -321,7 +330,7 @@ impl Http { let mut done = if status == 206 && start == have { have } else { 0 }; // We check the length and checksum ourselves; the reader's limit is only a backstop // against an endless body. (ureq errors once the limit is *reached*, so leave headroom.) - let remaining = if total > 0 { total.saturating_sub(done) } else { MAX_DOWNLOAD }; + let remaining = limit.saturating_sub(done); let mut reader = resp.body_mut().with_config().limit(remaining.saturating_add(1 << 20)).reader(); let mut buf = vec![0u8; 256 * 1024]; let at_start = done; @@ -338,12 +347,13 @@ impl Http { if n == 0 { break; } + let next_done = done.saturating_add(n as u64); + if next_done > limit { + return Err(AttemptError::Fatal(format!("the server sent more than the permitted size of {limit} bytes"))); + } let chunk = buf.get(..n).ok_or_else(|| AttemptError::Fatal("read past the buffer".into()))?; file.write_all(chunk).map_err(|e| AttemptError::Fatal(format!("couldn't save the download ({e}). Is the disk full?")))?; - done = done.saturating_add(n as u64); - if total > 0 && done > total { - return Err(retry("the server sent more than the file's size".into(), false)); - } + done = next_done; progress(Transfer::Bytes { done, total }); } file.sync_all().map_err(|e| AttemptError::Fatal(format!("couldn't save the download ({e})")))?; @@ -565,6 +575,17 @@ fn read_text_capped(resp: &mut ureq::http::Response, max: u64) -> Re Ok(text) } +/// Reads a binary body, rejecting one whose decoded size exceeds `max` (CWE-409). +fn read_bytes_capped(resp: &mut ureq::http::Response, max: usize) -> Result, String> { + let mut reader = resp.body_mut().with_config().reader().take((max as u64).saturating_add(1)); + let mut bytes = Vec::new(); + reader.read_to_end(&mut bytes).map_err(|e| e.to_string())?; + if bytes.len() > max { + return Err(format!("the answer is larger than {max} bytes")); + } + Ok(bytes) +} + #[cfg(test)] pub(crate) mod tests { use super::*; @@ -1033,4 +1054,25 @@ pub(crate) mod tests { fn partial_files_are_named_after_the_file_and_size() { assert_eq!(part_path(Path::new("/d/photocraft.dmg"), 9), Path::new("/d/photocraft.dmg.9.part")); } + + #[test] + fn get_bytes_rejects_oversized_payload() { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + std::thread::spawn(move || { + let Ok((mut stream, _)) = listener.accept() else { return }; + let mut line = String::new(); + let mut reader = BufReader::new(stream.try_clone().unwrap()); + while reader.read_line(&mut line).unwrap_or(0) > 0 && line != "\r\n" { + line.clear(); + } + let body = vec![b'A'; 200]; + let _ = write!(stream, "HTTP/1.1 200 OK\r\nContent-Length: 200\r\nConnection: close\r\n\r\n"); + let _ = stream.write_all(&body); + }); + let http = Http::new().unwrap(); + let result = http.get_bytes(&format!("http://127.0.0.1:{port}/test.bin"), 100); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("more than 100 bytes")); + } } From 6f2c7e7b4420758b48dcf28aa7bd76e28dc488a8 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 22:12:07 -0400 Subject: [PATCH 2/2] fix(http): restrict oversized check to resuming partial downloads Allows a full 200 response to replace an existing oversized partial file instead of erroneously rejecting it. --- crates/native/src/http.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/native/src/http.rs b/crates/native/src/http.rs index 7a6073aa..60dd41de 100644 --- a/crates/native/src/http.rs +++ b/crates/native/src/http.rs @@ -318,7 +318,8 @@ impl Http { return Err(AttemptError::Fatal(format!("unexpected Content-Encoding '{enc}' on binary artifact download"))); } let limit = if total > 0 { total } else { MAX_DOWNLOAD }; - if have > limit { + let appends = status == 206 && start == have; + if appends && have > limit { return Err(AttemptError::Fatal(format!("the download exceeds the permitted size of {limit} bytes"))); } // Open the part file at the right place: a 200 restarts it, a 206 continues it.