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..60dd41de 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,16 @@ 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 }; + 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. let mut file = match (status, start) { (206, s) if s == have => std::fs::OpenOptions::new().append(true).open(part), @@ -321,7 +331,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 +348,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 +576,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 +1055,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")); + } }