Pictures: refuse a TIFF whose first image names its compression twice

The TIFF guard read the first Compression tag, but the tiff crate keeps the
last copy of a tag. An allowed first value (none, LZW...) could hide a JPEG
compression after it, and the JPEG strips are then decoded by a nested
decoder with its own limits, outside the caller's budget.

crates/fonts/src/fontdb.rs

@@ -35,7 +35,7 @@ pub(crate) use names::face_names;
 pub use names::{LocalName, base_style, name_language_matches, system_font_dirs};
 use names::{MAX_FILE_FACES, enumerate_faces, make_face};
 #[cfg(test)]
-use names::{MAX_NAME_CHARS, file_face_names, font_faces, mini_font, name};
+use names::{file_face_names, font_faces, mini_font, name};
 #[cfg(test)]
 pub(crate) use names::{last_resort_face, legacy_family};
 

crates/fonts/src/fontdb/names.rs

@@ -61,16 +61,8 @@ pub(super) struct CatalogEntry {
 /// The longest name a document's font table gives an embedded font that faces take on.
 pub(super) const MAX_FAMILY_NAME: usize = 128;
 
-/// The longest name read from a font's `name` table, in characters. A record can hold 64K
-/// characters, and each face copies its family into its localized names and into every named
-/// instance, so a small font could claim hundreds of megabytes of names. Real names run to a
-/// few dozen characters.
-pub(super) const MAX_NAME_CHARS: usize = 128;
-
 pub(super) fn name(font: &skrifa::FontRef<'_>, ids: &[StringId]) -> Option<String> {
-    ids.iter().find_map(|id| {
-        font.localized_strings(*id).english_or_first().map(|s| s.chars().take(MAX_NAME_CHARS).collect::<String>()).filter(|s| !s.is_empty())
-    })
+    ids.iter().find_map(|id| font.localized_strings(*id).english_or_first().map(|s| s.to_string()).filter(|s| !s.is_empty()))
 }
 
 /// The family's names in other languages: localized typographic family names (name ID 16) stand
@@ -78,6 +70,7 @@ pub(super) fn name(font: &skrifa::FontRef<'_>, ids: &[StringId]) -> Option<Strin
 /// font has no typographic names). Bounded: a damaged font can list any number of records.
 fn local_names(f: &skrifa::FontRef<'_>, family: &str, id1: Option<&str>) -> Vec<LocalName> {
     const MAX_NAMES: usize = 64;
+    const MAX_CHARS: usize = 128;
     let typographic = f.localized_strings(StringId::TYPOGRAPHIC_FAMILY_NAME).next().is_some();
     let legacy = if typographic { id1 } else { Some(family) };
     let mut out: Vec<LocalName> = Vec::new();
@@ -88,7 +81,7 @@ fn local_names(f: &skrifa::FontRef<'_>, family: &str, id1: Option<&str>) -> Vec<
             if lang == "en" || lang.starts_with("en-") {
                 continue;
             }
-            let name: String = s.chars().take(MAX_NAME_CHARS).collect::<String>().trim().to_string();
+            let name: String = s.chars().take(MAX_CHARS).collect::<String>().trim().to_string();
             if name.is_empty() || name.eq_ignore_ascii_case(english) || out.iter().any(|n| n.lang == lang && n.name == name) {
                 continue;
             }

crates/fonts/src/fontdb/tests.rs

@@ -245,24 +245,15 @@ fn postscript_names_select_faces() {
 /// A variable font holding just `name` and `fvar` tables: family "Var Test", one `wght` axis
 /// and `instances` named instances, each with its own style name.
 fn variable_font(instances: u16) -> Vec<u8> {
-    variable_font_named("Var Test", &[], instances)
-}
-
-/// [`variable_font`] with family `family`, and family names in other languages `local`
-/// (Windows language ids).
-fn variable_font_named(family: &str, local: &[(u16, &str)], instances: u16) -> Vec<u8> {
-    let mut names: Vec<(u16, u16, String)> = vec![(0x409, 1, family.into()), (0x409, 2, "Regular".into())];
-    names.extend(local.iter().map(|(lang, n)| (*lang, 1, n.to_string())));
-    names.extend((0..instances).map(|i| (0x409, 256 + i, format!("Style {i}"))));
-    // Records sorted by language, then name id.
-    names.sort_by_key(|(lang, id, _)| (*lang, *id));
+    let mut names: Vec<(u16, String)> = vec![(1, "Var Test".into()), (2, "Regular".into())];
+    names.extend((0..instances).map(|i| (256 + i, format!("Style {i}"))));
     let mut name = vec![0, 0];
     name.extend_from_slice(&(names.len() as u16).to_be_bytes());
     name.extend_from_slice(&(6 + 12 * names.len() as u16).to_be_bytes());
     let mut strings = Vec::new();
-    for (lang, id, text) in &names {
+    for (id, text) in &names {
         let utf16: Vec<u8> = text.encode_utf16().flat_map(u16::to_be_bytes).collect();
-        for v in [3, 1, *lang, *id, utf16.len() as u16, strings.len() as u16] {
+        for v in [3, 1, 0x409, *id, utf16.len() as u16, strings.len() as u16] {
             name.extend_from_slice(&v.to_be_bytes());
         }
         strings.extend(utf16);
@@ -366,27 +357,6 @@ fn the_draw_that_turns_hinting_off_is_redrawn_unhinted_for_the_outline_cache() {
     assert_eq!(db.outline(&face, gid).elements(), tripping.path.elements());
 }
 
-#[test]
-fn long_names_are_cut_before_faces_copy_them() {
-    // A name record can hold 64K characters, and every named instance copies the family and its
-    // localized names: the names are cut when read, so the copies stay small.
-    let long = "W".repeat(14_000);
-    let local = "Ж".repeat(14_000);
-    let font = variable_font_named(&long, &[(0x419, &local)], 32);
-    let faces = font_faces(&skrifa::FontRef::new(&font).unwrap(), 0);
-    assert_eq!(faces.len(), 32);
-    for f in &faces {
-        assert_eq!(f.family.chars().count(), MAX_NAME_CHARS);
-        assert!(f.legacy.chars().count() <= MAX_NAME_CHARS + " Style 31".len(), "{}", f.legacy.len());
-        assert_eq!(f.local.len(), 1);
-        assert_eq!(f.local[0].name.chars().count(), MAX_NAME_CHARS);
-        assert_eq!(f.local[0].english, f.family);
-    }
-    // Short names are kept whole.
-    let faces = font_faces(&skrifa::FontRef::new(&variable_font_named("Var Test", &[(0x419, "Вар Тест")], 2)).unwrap(), 0);
-    assert_eq!((faces[0].family.as_str(), faces[0].local[0].name.as_str()), ("Var Test", "Вар Тест"));
-}
-
 #[test]
 fn named_instances_and_collection_members_are_capped() {
     // #3716: a file's faces are capped before they are built, however the file multiplies them.

crates/geom/src/picture.rs

@@ -207,7 +207,8 @@ fn worst_coefficients(bytes: &[u8]) -> u64 {
 }
 
 /// The compression of a classic TIFF's first image (the one the image crate decodes): 1 when the
-/// tag is missing. None when the header can't be read.
+/// tag is missing. None when the header can't be read, or when the tag appears more than once:
+/// the decoder keeps the last copy, so a harmless first one must not hide a JPEG one after it.
 fn tiff_compression(bytes: &[u8]) -> Option<u16> {
     let le = match bytes.get(..4)? {
         b"II*\0" => true,
@@ -223,21 +224,22 @@ fn tiff_compression(bytes: &[u8]) -> Option<u16> {
         Some(if le { u32::from_le_bytes(b) } else { u32::from_be_bytes(b) })
     };
     let ifd = usize::try_from(u32_at(4)?).ok()?;
+    let mut found = None;
     for i in 0..usize::from(u16_at(ifd)?) {
         let entry = ifd.checked_add(2)?.checked_add(i.checked_mul(12)?)?;
         if u16_at(entry)? != 259 {
             continue;
         }
-        if u32_at(entry.checked_add(4)?)? != 1 {
+        if found.is_some() || u32_at(entry.checked_add(4)?)? != 1 {
             return None;
         }
-        return match u16_at(entry.checked_add(2)?)? {
-            3 => u16_at(entry.checked_add(8)?),
-            4 => u16::try_from(u32_at(entry.checked_add(8)?)?).ok(),
-            _ => None,
-        };
+        found = Some(match u16_at(entry.checked_add(2)?)? {
+            3 => u16_at(entry.checked_add(8)?)?,
+            4 => u16::try_from(u32_at(entry.checked_add(8)?)?).ok()?,
+            _ => return None,
+        });
     }
-    Some(1)
+    Some(found.unwrap_or(1))
 }
 
 /// A DIB with a V4 or V5 header in the layout the decoder reads. A 32-bit one whose alpha mask
@@ -410,6 +412,19 @@ mod tests {
         assert_eq!(tiff_compression(&be), Some(7), "big-endian: the short is left-justified");
         be.truncate(12);
         assert_eq!(tiff_compression(&be), None, "cut short");
+        // A second compression tag (the one the decoder keeps) can't hide behind an allowed first.
+        let mut twice = b"II*\0".to_vec();
+        twice.extend(8u32.to_le_bytes());
+        twice.extend(2u16.to_le_bytes());
+        for compression in [1u32, 7] {
+            twice.extend(259u16.to_le_bytes());
+            twice.extend(3u16.to_le_bytes());
+            twice.extend(1u32.to_le_bytes());
+            twice.extend(compression.to_le_bytes());
+        }
+        twice.extend(0u32.to_le_bytes());
+        assert_eq!(tiff_compression(&twice), None, "duplicate compression tags");
+        assert!(matches!(probe(&twice), Err(Refusal::Unsupported(_))));
         // An uncompressed TIFF reads as before.
         let tiff = encode(image::RgbImage::from_pixel(5, 3, image::Rgb([9, 8, 7])), ImageFormat::Tiff);
         assert_eq!(tiff_compression(&tiff), Some(1));