Merge pull request #5299 from storytold/fix/sec-icc-duplicate-tags

ICC: refuse a profile whose tag table lists a tag twice

crates/color/src/cms/icc.rs

@@ -106,6 +106,12 @@ impl IccProfile {
         if !localized_text_fits(bytes) {
             return Err(CmsError::BadProfile(format!("its localized names take more than {} KB", MAX_LOCALIZED_TEXT >> 10)));
         }
+        if let Some(tag) = repeated_tag(bytes) {
+            return Err(CmsError::BadProfile(format!(
+                "its tag table lists the `{}` tag more than once",
+                String::from_utf8_lossy(&tag.to_be_bytes())
+            )));
+        }
         let p = ColorProfile::new_from_slice(bytes).map_err(|e| CmsError::BadProfile(format!("{e:?}")))?;
         Ok(Self { source: bytes.to_vec(), ..Self::from_profile(name, p)? })
     }
@@ -417,6 +423,23 @@ pub(super) fn gray_profile(name: &str) -> ColorProfile {
     labelled(p, name)
 }
 
+/// The first tag signature the tag table of ICC `bytes` lists twice, which the ICC
+/// specification forbids. moxcms parses every entry of the table, so a profile listing one
+/// description, copyright or table tag thousands of times, each pointing at the same large data,
+/// would decode and copy that data once per entry. A table it can't read is left to moxcms.
+pub(crate) fn repeated_tag(bytes: &[u8]) -> Option<u32> {
+    let be32 = |at: usize| bytes.get(at..at.checked_add(4)?).map(|v| u32::from_be_bytes([v[0], v[1], v[2], v[3]]));
+    let tags = be32(128)? as usize;
+    let mut seen = std::collections::HashSet::new();
+    for i in 0..tags {
+        let sig = be32(i.checked_mul(12)?.checked_add(132)?)?;
+        if !seen.insert(sig) {
+            return Some(sig);
+        }
+    }
+    None
+}
+
 /// Most bytes of UTF-16 the localized names (`mluc` tags) of a profile may take together.
 const MAX_LOCALIZED_TEXT: usize = 1 << 20;
 

crates/color/src/cms/tests_icc.rs

@@ -181,6 +181,32 @@ fn aliased_localized_names_are_refused() {
     assert!(!super::icc::localized_text_fits(&profile(4096, 64 << 10)));
 }
 
+/// A tag table that lists one tag many times, every entry pointing at the same long text, is
+/// refused before moxcms decodes the text once per entry; distinct tags pass.
+#[test]
+fn repeated_tags_are_refused() {
+    let profile = |sigs: &[&[u8; 4]], text: usize| {
+        let mut b = vec![0u8; 128];
+        b.extend((sigs.len() as u32).to_be_bytes());
+        let tag_at = 128 + 4 + 12 * sigs.len();
+        let size = 8 + text;
+        for sig in sigs {
+            b.extend(*sig);
+            b.extend((tag_at as u32).to_be_bytes());
+            b.extend((size as u32).to_be_bytes());
+        }
+        b.extend(b"text");
+        b.extend([0; 4]);
+        b.extend(vec![b'a'; text]);
+        b
+    };
+    assert_eq!(super::icc::repeated_tag(&profile(&[b"desc", b"cprt", b"dmdd"], 64)), None);
+    let many: Vec<&[u8; 4]> = std::iter::repeat_n(b"desc", 4096).collect();
+    let bytes = profile(&many, 256 << 10);
+    assert_eq!(super::icc::repeated_tag(&bytes), Some(u32::from_be_bytes(*b"desc")));
+    assert!(matches!(super::icc::IccProfile::from_bytes(None, &bytes), Err(super::CmsError::BadProfile(_))));
+}
+
 /// Reloading an RGB profile under the name it had (a new file for "My RGB") changes what CMYK
 /// converts to in it: the direct CMYK → RGB transform isn't kept from the old file.
 #[test]