fix(xmp): audit - bound decode recursion and growth, escape encoder output

src/libOpenImageIO/xmp.cpp

@@ -280,6 +280,40 @@ parse_rational(string_view s, int& n, int& d)
 
 
 
+// Bounds on the work an XMP decode may do. Real XMP is a few hundred bytes
+// to a few tens of KB, nested a handful of levels deep, holding tens of
+// attributes. These caps sit far above anything legitimate; they exist
+// because every one of those quantities is otherwise bounded only by the
+// length of an attacker-supplied packet.
+static constexpr int max_xmp_depth      = 64;
+static constexpr size_t max_xmp_attribs = 4096;
+static constexpr size_t max_xmp_bytes   = 1024 * 1024;
+
+
+// Running totals for one decode. `bytes` accumulates the size add_attrib()
+// reports for each attribute it writes; for a list attribute that size is the
+// whole re-joined list, so this total is the quadratic cost of growing the
+// list, and capping it caps that cost directly.
+struct XMPbudget {
+    size_t bytes   = 0;
+    size_t attribs = 0;
+
+    bool exhausted() const
+    {
+        return bytes > max_xmp_bytes || attribs > max_xmp_attribs;
+    }
+
+    // Charge one attribute write; return false once the budget is gone.
+    bool spend(size_t nbytes)
+    {
+        bytes += nbytes;
+        ++attribs;
+        return !exhausted();
+    }
+};
+
+
+
 // Utility: add an attribute to the spec with the given xml name and
 // value.  Search for it in xmptag, and if found that will tell us what
 // the type is supposed to be, as well as any special handling.  If not
@@ -433,9 +467,15 @@ add_attrib(ImageSpec& spec, string_view xmlname, string_view xmlvalue,
 // Return value is the size of the resulting attribute (can be used to
 // catch runaway or corrupt XML).
 static size_t
-decode_xmp_node(pugi::xml_node node, ImageSpec& spec, int level = 1,
-                const char* parentname = NULL, bool isList = false)
+decode_xmp_node(pugi::xml_node node, ImageSpec& spec, XMPbudget& budget,
+                int level = 1, const char* parentname = NULL,
+                bool isList = false)
 {
+    // Each level of XML nesting costs a stack frame here, and a packet can
+    // nest as deep as its own length allows, so cap the descent.
+    if (level > max_xmp_depth)
+        return 0;
+
     std::string mylist;  // will accumulate for list items
     size_t totalsize = 0;
     for (; node; node = node.next_sibling()) {
@@ -459,10 +499,12 @@ decode_xmp_node(pugi::xml_node node, ImageSpec& spec, int level = 1,
                 totalsize += sz;
                 // As a guard against runaway lists or corrupt XMP blocks,
                 // don't let attribute lists grow to more than 64KB each.
-                if (sz > 64 * 1024)
+                if (!budget.spend(sz) || sz > 64 * 1024)
                     break;
             }
         }
+        if (budget.exhausted())
+            break;
         if (Strutil::iequals(node.name(), "xmpMM::History")) {
             // FIXME -- image history is complicated. Come back to it.
             continue;
@@ -482,12 +524,12 @@ decode_xmp_node(pugi::xml_node node, ImageSpec& spec, int level = 1,
             || Strutil::iequals(node.name(), "rdf:li")) {
             // Various kinds of lists.  Recurse, pass the parent name
             // down, and let the child know it's part of a list.
-            totalsize += decode_xmp_node(node.first_child(), spec, level + 1,
-                                         parentname, true);
+            totalsize += decode_xmp_node(node.first_child(), spec, budget,
+                                         level + 1, parentname, true);
         } else {
             // Not a list, but it's got children.  Recurse.
-            totalsize += decode_xmp_node(node.first_child(), spec, level + 1,
-                                         node.name(), isList);
+            totalsize += decode_xmp_node(node.first_child(), spec, budget,
+                                         level + 1, node.name(), isList);
         }
 
         // If this node has a value but no name, it's definitely part
@@ -498,16 +540,22 @@ decode_xmp_node(pugi::xml_node node, ImageSpec& spec, int level = 1,
                 mylist += ";";
             mylist += node.value();
             totalsize += mylist.size();
+            if (mylist.size() > max_xmp_bytes)
+                break;
         }
         // As a guard against runaway lists or corrupt XMP blocks,
         // don't let attribute lists grow to more than 64KB each.
         if (isList && totalsize > 64 * 1024)
             break;
+        if (budget.exhausted())
+            break;
     }
 
     // If we have accumulated a list, turn it into an attribute
     if (parentname && mylist.size()) {
-        totalsize += add_attrib(spec, parentname, mylist, true);
+        size_t sz = add_attrib(spec, parentname, mylist, true);
+        totalsize += sz;
+        budget.spend(sz);
     }
     return totalsize;
 }
@@ -565,8 +613,15 @@ decode_xmp(string_view xml, ImageSpec& spec)
     auto first_desc = doc.find_node([](pugi::xml_node n) {
         return strcmp(n.name(), "rdf:Description") == 0;
     });
-    if (first_desc)
-        decode_xmp_node(first_desc, spec);
+    if (first_desc) {
+        XMPbudget budget;
+        // A file may hold many XMP packets (png allows any number of zTXt
+        // chunks), and ImageSpec::attribute() is a linear scan, so the cost is
+        // quadratic in the spec's total attribute count, not in any one
+        // packet's share. Charge what the spec already holds.
+        budget.attribs = spec.extra_attribs.size();
+        decode_xmp_node(first_desc, spec, budget);
+    }
 #if DEBUG_XMP_READ
     std::cerr << "XMP total parse time " << timer() << "\n";
 #endif
@@ -576,6 +631,31 @@ decode_xmp(string_view xml, ImageSpec& spec)
 
 
 
+// Escape the XML metacharacters so a value cannot close its own quote (or its
+// element) and turn into markup. Values reaching the encoder are ordinary
+// metadata, which may equally well have arrived from a file read by
+// decode_xmp(), so they are not trusted to be XML-safe. Apostrophes are left
+// alone: every value we emit is inside double quotes, so escaping them would
+// only churn the output of files that are already fine.
+static std::string
+xml_escape(string_view s)
+{
+    std::string out;
+    out.reserve(s.size());
+    for (char c : s) {
+        switch (c) {
+        case '&': out += "&amp;"; break;
+        case '<': out += "&lt;"; break;
+        case '>': out += "&gt;"; break;
+        case '"': out += "&quot;"; break;
+        default: out += c; break;
+        }
+    }
+    return out;
+}
+
+
+
 // Turn one ParamValue (whose xmp info we know) into a properly
 // serialized xmp string.
 static std::string
@@ -676,16 +756,18 @@ encode_xmp_category(std::vector<std::pair<const XMPtag*, std::string>>& list,
         if (Strutil::istarts_with(xmpname, pattern)) {
             std::string x;
             if (control == XMP_attribs)
-                x = Strutil::fmt::format("{}=\"{}\"", xmpname, val);
+                x = Strutil::fmt::format("{}=\"{}\"", xmpname, xml_escape(val));
             else if (control == XMP_AltList || control == XMP_BagList) {
                 std::vector<std::string> vals;
                 Strutil::split(val, vals, ";");
                 for (auto& val : vals) {
                     val = Strutil::strip(val);
-                    x += Strutil::fmt::format("<rdf:li>{}</rdf:li>", val);
+                    x += Strutil::fmt::format("<rdf:li>{}</rdf:li>",
+                                              xml_escape(val));
                 }
             } else
-                x = Strutil::fmt::format("<{}>{}</{}>", xmpname, val, xmpname);
+                x = Strutil::fmt::format("<{}>{}</{}>", xmpname,
+                                         xml_escape(val), xmpname);
             if (!x.empty() && control != XMP_suppress) {
                 if (!found) {
                     // if (nodename && nodename[0]) {

testsuite/jpeg-corrupt/ref/out-alt2.txt

@@ -38,6 +38,11 @@ src/corrupt-exif-deep-ifds.jpg :    1 x    1, 1 channel, uint8 jpeg
     SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
     channel list: Y
     oiio:ColorSpace: "srgb_rec709_scene"
+Reading src/corrupt-xmp-deep-nesting.jpg
+src/corrupt-xmp-deep-nesting.jpg :    1 x    1, 1 channel, uint8 jpeg
+    SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
+    channel list: Y
+    oiio:ColorSpace: "srgb_rec709_scene"
 corrupt-icc-4551.jpg
 DCT coefficient (lossy) or spatial difference (lossless) out of range
 Reading src/corrupt-icc-4552.jpg

testsuite/jpeg-corrupt/ref/out-alt3.txt

@@ -38,6 +38,11 @@ src/corrupt-exif-deep-ifds.jpg :    1 x    1, 1 channel, uint8 jpeg
     SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
     channel list: Y
     oiio:ColorSpace: "srgb_rec709_scene"
+Reading src/corrupt-xmp-deep-nesting.jpg
+src/corrupt-xmp-deep-nesting.jpg :    1 x    1, 1 channel, uint8 jpeg
+    SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
+    channel list: Y
+    oiio:ColorSpace: "srgb_rec709_scene"
 corrupt-icc-4551.jpg
 Reading src/corrupt-icc-4552.jpg
 src/corrupt-icc-4552.jpg : 1500 x 1000, 3 channel, uint8 jpeg

testsuite/jpeg-corrupt/ref/out-alt4.txt

@@ -38,6 +38,11 @@ src/corrupt-exif-deep-ifds.jpg :    1 x    1, 1 channel, uint8 jpeg
     SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
     channel list: Y
     oiio:ColorSpace: "srgb_rec709_scene"
+Reading src/corrupt-xmp-deep-nesting.jpg
+src/corrupt-xmp-deep-nesting.jpg :    1 x    1, 1 channel, uint8 jpeg
+    SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
+    channel list: Y
+    oiio:ColorSpace: "srgb_rec709_scene"
 corrupt-icc-4551.jpg
 DCT coefficient out of range
 Reading src/corrupt-icc-4552.jpg

testsuite/jpeg-corrupt/ref/out-alt5.txt

@@ -38,6 +38,11 @@ src/corrupt-exif-deep-ifds.jpg :    1 x    1, 1 channel, uint8 jpeg
     SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
     channel list: Y
     oiio:ColorSpace: "srgb_rec709_scene"
+Reading src/corrupt-xmp-deep-nesting.jpg
+src/corrupt-xmp-deep-nesting.jpg :    1 x    1, 1 channel, uint8 jpeg
+    SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
+    channel list: Y
+    oiio:ColorSpace: "srgb_rec709_scene"
 corrupt-icc-4551.jpg
 iconvert ERROR copying "src/corrupt-icc-4551.jpg" to "out-4551.jpg" :
 	JPEG error: Corrupt JPEG data: bad Huffman code ("src/corrupt-icc-4551.jpg")

testsuite/jpeg-corrupt/ref/out.txt

@@ -38,6 +38,11 @@ src/corrupt-exif-deep-ifds.jpg :    1 x    1, 1 channel, uint8 jpeg
     SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
     channel list: Y
     oiio:ColorSpace: "srgb_rec709_scene"
+Reading src/corrupt-xmp-deep-nesting.jpg
+src/corrupt-xmp-deep-nesting.jpg :    1 x    1, 1 channel, uint8 jpeg
+    SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
+    channel list: Y
+    oiio:ColorSpace: "srgb_rec709_scene"
 corrupt-icc-4551.jpg
 DCT coefficient (lossy) or spatial difference (lossless) out of range
 Reading src/corrupt-icc-4552.jpg

testsuite/jpeg-corrupt/run.py

@@ -39,6 +39,13 @@
 # the decoder now stops descending after a fixed depth.
 command += info_command ("src/corrupt-exif-deep-ifds.jpg", safematch=True)
 
+# This file's APP1 XMP marker nests ~9000 XML elements, all a 64 KB marker
+# has room for. decode_xmp_node() recursed once per level with nothing
+# bounding the descent, enough to exhaust the stack of a sanitizer build or of
+# a worker thread with a small stack. The decoder now stops descending after a
+# fixed depth.
+command += info_command ("src/corrupt-xmp-deep-nesting.jpg", safematch=True)
+
 # This file has a corrupted ICC profile block that has tags that say they
 # extend beyond the boundaries of the ICC block itself.
 command += run_app (oiiotool("--echo corrupt-icc-4551.jpg"))

testsuite/jpeg-corrupt/src/make-xmp-fixtures.py

@@ -0,0 +1,77 @@
+#!/usr/bin/env python3
+
+# Copyright Contributors to the OpenImageIO project.
+# SPDX-License-Identifier: Apache-2.0
+# https://github.com/AcademySoftwareFoundation/OpenImageIO
+
+"""Generator for the malformed-XMP .jpg fixture in this directory.
+
+The file it writes is committed, so this only needs to be run if it must be
+regenerated:
+
+    python3 make-xmp-fixtures.py .
+
+  corrupt-xmp-deep-nesting.jpg  A valid 1x1 JPEG whose APP1 XMP marker holds
+                                an element chain nested one level per stack
+                                frame in decode_xmp_node(). This is the JPEG
+                                route to the shared decoder the .png fixtures
+                                in testsuite/png-damaged/src reach through a
+                                zTXt chunk. An APP1 marker caps out at 64 KB,
+                                which buys ~9000 levels -- enough to exhaust
+                                the stack of a debug or sanitizer build, or of
+                                any worker thread with a small stack, and
+                                enough to confirm the depth cap is not a
+                                PNG-only property.
+"""
+
+import struct
+import sys
+import os
+
+# The same valid 1x1 grayscale JPEG that make-exif-fixtures.py uses, written
+# by oiiotool with its APP1 Exif segment stripped, embedded here so the
+# fixture does not depend on a JPEG writer and carries no version-dependent
+# metadata of its own.
+BASE_JPEG = bytes.fromhex(
+    'ffd8ffe000104a46494600010100000100010000ffdb004300010101010101010101'
+    '01010101010102010101010102010101020202020202020202030304030303030302'
+    '020304030304040404040203050504040504040404ffc0000b080001000101011100'
+    'ffc4001f0000010501010101010100000000000000000102030405060708090a0bff'
+    'c400b5100002010303020403050504040000017d0102030004110512213141061351'
+    '6107227114328191a1082342b1c11552d1f02433627282090a161718191a25262728'
+    '292a3435363738393a434445464748494a535455565758595a636465666768696a73'
+    '7475767778797a838485868788898a92939495969798999aa2a3a4a5a6a7a8a9aab2'
+    'b3b4b5b6b7b8b9bac2c3c4c5c6c7c8c9cad2d3d4d5d6d7d8d9dae1e2e3e4e5e6e7e8'
+    'e9eaf1f2f3f4f5f6f7f8f9faffda0008010100003f00ff003ffaffd9')
+
+XMP_URI = b'http://ns.adobe.com/xap/1.0/\0'
+
+HEAD = ('<?xpacket begin="" id="W5M0MpCehiHzreSzNTczkc9d"?>'
+        '<x:xmpmeta xmlns:x="adobe:ns:meta/">'
+        '<rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#">'
+        '<rdf:Description rdf:about="" '
+        'xmlns:foo="http://example.com/foo/">')
+FOOT = '</rdf:Description></rdf:RDF></x:xmpmeta><?xpacket end="w"?>'
+
+
+def app1_xmp(xmp):
+    body = XMP_URI + xmp.encode('utf-8')
+    assert len(body) + 2 <= 0xffff, 'APP1 payload exceeds the marker limit'
+    return b'\xff\xe1' + struct.pack('>H', len(body) + 2) + body
+
+
+def deep_nesting_xmp():
+    # Fill the marker. The tags are left unclosed so a level costs 7 bytes
+    # rather than 15; pugixml's fragment mode builds the nesting either way.
+    budget = 0xffff - 2 - len(XMP_URI) - len(HEAD) - len(FOOT)
+    return HEAD + '<foo:a>' * (budget // 7) + FOOT
+
+
+def main(dir):
+    path = os.path.join(dir, 'corrupt-xmp-deep-nesting.jpg')
+    with open(path, 'wb') as f:
+        f.write(BASE_JPEG[:2] + app1_xmp(deep_nesting_xmp()) + BASE_JPEG[2:])
+
+
+if __name__ == '__main__':
+    main(sys.argv[1] if len(sys.argv) > 1 else '.')

testsuite/jpeg-metadata/ref/out.txt

@@ -110,3 +110,10 @@ with-colon-desc.jpg  :  640 x  480, 3 channel, uint8 jpeg
     Exif:FlashPixVersion: "0100"
     jpeg:subsampling: "4:2:0"
     oiio:ColorSpace: "srgb_rec709_scene"
+Reading xmp-escape.jpg
+xmp-escape.jpg       :    1 x    1, 3 channel, uint8 jpeg
+    SHA-1: 29E2DCFBB16F63BB0254DF7585A15BB6FB5E927D
+    channel list: R, G, B
+    IPTC:RightsUsageTerms: "a<b&c>d"
+    jpeg:subsampling: "4:2:0"
+    oiio:ColorSpace: "srgb_rec709_scene"

testsuite/jpeg-metadata/run.py

@@ -36,3 +36,12 @@
                        extraargs="--attrib:type=string ImageDescription \"Example:Text\"")
 command += info_command ("with-colon-desc.jpg", safematch=True,
                          extraargs="--oiioattrib:type=int jpeg:com_attributes 0")
+
+# The XMP encoder interpolates attribute values into XML text, so a value
+# holding XML metacharacters has to be escaped or it becomes markup. This one
+# is carried only by XMP (not also by the IPTC IIM block); unescaped, it
+# truncated to "a" and took the rest of the packet with it.
+command += oiiotool ("--create 1x1 3 "
+                     "--attrib:type=string IPTC:RightsUsageTerms \"a<b&c>d\" "
+                     "-o xmp-escape.jpg")
+command += info_command ("xmp-escape.jpg", safematch=True)

testsuite/png-damaged/ref/out.txt

@@ -22,3 +22,14 @@ src/exif-deep-ifds.png :    1 x    1, 1 channel, uint8 png
     SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
     channel list: Y
     oiio:ColorSpace: "srgb_rec709_scene"
+Reading src/xmp-deep-nesting.png
+src/xmp-deep-nesting.png :    1 x    1, 1 channel, uint8 png
+    SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
+    channel list: Y
+    oiio:ColorSpace: "srgb_rec709_scene"
+src/xmp-list-blowup.png :    1 x    1, 1 channel, uint8 png
+    SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
+src/xmp-many-attribs.png :    1 x    1, 1 channel, uint8 png
+    SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F
+src/xmp-multi-packet.png :    1 x    1, 1 channel, uint8 png
+    SHA-1: 5BA93C9DB0CFF93F52B521D7420E43F6EDA2784F