fix(jpeg): Improved safety and error reporting for jpeg and iptc (#5081)

* `decide_iptc_iim()` (semi-public) : change from taking a pointer &
length to a string_view, and internal safety checks.

* `jpeg_decode_iptc()` (non-public): change from taking an unlengthed
pointer (!) to a string_view, and internal safety checks.

* JpgInput::open(): a number of extra checks for memory bounds validity.

---------

Signed-off-by: Larry Gritz <lg@larrygritz.com>

Author: lgritz

src/include/OpenImageIO/tiffutils.h

@@ -210,6 +210,9 @@ OIIO_API bool gps_tag_lookup (string_view name, int &tag,
 /// metadata without having to duplicate functionality within each
 /// plugin.  Note that IIM is actually considered obsolete and is
 /// replaced by an XML scheme called XMP.
+OIIO_API bool decode_iptc_iim (string_view iptc, ImageSpec &spec);
+
+// DEPRECATED(3.2) -- unsafe version
 OIIO_API bool decode_iptc_iim (const void *iptc, int length, ImageSpec &spec);
 
 /// Find all the IPTC-amenable metadata in spec and assemble it into an

src/jpeg.imageio/jpeg_pvt.h

@@ -129,7 +129,9 @@ class JpgInput final : public ImageInput {
     // information and adding attributes to spec.  This assumes it's in
     // the form of an IIM (Information Interchange Model), which is actually
     // considered obsolete and is replaced by an XML scheme called XMP.
-    void jpeg_decode_iptc(const unsigned char* buf);
+    // Return true if ok, false if there were corruptions or errors while
+    // decoding the iptc.
+    bool jpeg_decode_iptc(string_view buf);
 
     bool read_icc_profile(j_decompress_ptr cinfo, ImageSpec& spec);
 

src/jpeg.imageio/jpeginput.cpp

@@ -62,11 +62,10 @@ my_error_exit(j_common_ptr cinfo)
 {
     /* cinfo->err really points to a my_error_mgr struct, so coerce pointer */
     JpgInput::my_error_ptr myerr = (JpgInput::my_error_ptr)cinfo->err;
+    OIIO_ASSERT(myerr);
 
-    /* Always display the message. */
-    /* We could postpone this until after returning, if we chose. */
-    //  (*cinfo->err->output_message) (cinfo);
-    myerr->jpginput->jpegerror(myerr, true);
+    if (myerr->jpginput)
+        myerr->jpginput->jpegerror(myerr, true);
 
     /* Return control to the setjmp point */
     longjmp(myerr->setjmp_buffer, 1);
@@ -78,11 +77,10 @@ static void
 my_output_message(j_common_ptr cinfo)
 {
     JpgInput::my_error_ptr myerr = (JpgInput::my_error_ptr)cinfo->err;
+    OIIO_ASSERT(myerr);
 
-    // Create the message
-    char buffer[JMSG_LENGTH_MAX];
-    (*cinfo->err->format_message)(cinfo, buffer);
-    myerr->jpginput->jpegerror(myerr, false);
+    if (myerr->jpginput)
+        myerr->jpginput->jpegerror(myerr, false);
 
     // This function is called only for non-fatal problems, so we don't
     // need to do the longjmp.
@@ -122,7 +120,7 @@ void
 JpgInput::jpegerror(my_error_ptr /*myerr*/, bool fatal)
 {
     // Send the error message to the ImageInput
-    char errbuf[JMSG_LENGTH_MAX];
+    char errbuf[JMSG_LENGTH_MAX] = "";
     (*m_cinfo.err->format_message)((j_common_ptr)&m_cinfo, errbuf);
     errorfmt("JPEG error: {} (\"{}\")", errbuf, filename());
 
@@ -273,21 +271,25 @@ JpgInput::open(const std::string& name, ImageSpec& newspec)
         m_spec.attribute(JPEG_SUBSAMPLING_ATTR, subsampling);
 
     for (jpeg_saved_marker_ptr m = m_cinfo.marker_list; m; m = m->next) {
-        if (m->marker == (JPEG_APP0 + 1)
-            && !strcmp((const char*)m->data, "Exif")) {
+        if (m->marker == (JPEG_APP0 + 1) && m->data_length >= 4
+            && !strncmp((const char*)m->data, "Exif", 4)) {
             // The block starts with "Exif\0\0", so skip 6 bytes to get
             // to the start of the actual Exif data TIFF directory
             decode_exif(string_view((char*)m->data + 6, m->data_length - 6),
                         m_spec);
-        } else if (m->marker == (JPEG_APP0 + 1)
-                   && !strcmp((const char*)m->data,
-                              "http://ns.adobe.com/xap/1.0/")) {  //NOSONAR
+        } else if (m->marker == (JPEG_APP0 + 1) && m->data_length >= 28
+                   && !strncmp((const char*)m->data,
+                               "http://ns.adobe.com/xap/1.0/", 28)) {  //NOSONAR
             std::string xml((const char*)m->data, m->data_length);
             decode_xmp(xml, m_spec);
-        } else if (m->marker == (JPEG_APP0 + 13)
-                   && !strcmp((const char*)m->data, "Photoshop 3.0"))
-            jpeg_decode_iptc((unsigned char*)m->data);
-        else if (m->marker == JPEG_COM) {
+        } else if (m->marker == (JPEG_APP0 + 13) && m->data_length >= 13
+                   && !strncmp((const char*)m->data, "Photoshop 3.0", 13)) {
+            if (!jpeg_decode_iptc(
+                    string_view((const char*)m->data, m->data_length))) {
+                errorfmt("Corrupted IPTC data");
+                return false;
+            }
+        } else if (m->marker == JPEG_COM) {
             std::string data((const char*)m->data, m->data_length);
             // Additional string metadata can be stored in JPEG files as
             // comment markers in the form "key:value" or "ident:key:value".
@@ -646,17 +648,17 @@ JpgInput::read_native_scanlines(int subimage, int miplevel, int ybegin,
         return false;
     }
 
-    int nscanlines     = yend - ybegin;
+    int64_t nscanlines = yend - ybegin;
     size_t sl_bytes    = m_spec.scanline_bytes(true /*native*/);
     JSAMPLE** readdata = OIIO_ALLOCA(JSAMPLE*, nscanlines);
-    for (int i = 0; i < nscanlines; ++i)
+    for (int64_t i = 0; i < nscanlines; ++i)
         readdata[i] = reinterpret_cast<JSAMPLE*>(&data[i * sl_bytes]);
 
     if (m_cmyk) {
         // If the file's data is CMYK, read into a 4-channel buffer, then
         // we'll have to convert.
         m_cmyk_buf.resize(m_spec.width * 4 * nscanlines);
-        for (int i = 0; i < nscanlines; ++i)
+        for (int64_t i = 0; i < nscanlines; ++i)
             readdata[i] = reinterpret_cast<JSAMPLE*>(m_cmyk_buf.data()
                                                      + i * m_spec.width * 4);
         OIIO_DASSERT(m_spec.nchannels == 3);
@@ -685,7 +687,7 @@ JpgInput::read_native_scanlines(int subimage, int miplevel, int ybegin,
     m_next_scanline = yend;
 
     if (m_cmyk) {
-        for (int i = 0; i < nscanlines; ++i)
+        for (int64_t i = 0; i < nscanlines; ++i)
             cmyk_to_rgb(make_cspan(readdata[i], m_spec.width * 4),
                         span_cast<unsigned char>(data).subspan(
                             m_spec.width * 3 * i, m_spec.width * 3));
@@ -717,34 +719,51 @@ JpgInput::close()
 
 
 
-void
-JpgInput::jpeg_decode_iptc(const unsigned char* buf)
+bool
+JpgInput::jpeg_decode_iptc(string_view buf)
 {
     // APP13 blob doesn't have to be IPTC info.  Look for the IPTC marker,
     // which is the string "Photoshop 3.0" followed by a null character.
-    if (strcmp((const char*)buf, "Photoshop 3.0"))
-        return;
-    buf += strlen("Photoshop 3.0") + 1;
+    if (!Strutil::starts_with(buf, "Photoshop 3.0"))
+        return false;
+    buf.remove_prefix(13);
+    if (buf.size() < 1 || buf[0] != '\0')
+        return false;
+    buf.remove_prefix(1);
 
     // Next are the 4 bytes "8BIM"
-    if (strncmp((const char*)buf, "8BIM", 4))
-        return;
-    buf += 4;
+    if (!Strutil::starts_with(buf, "8BIM"))
+        return false;
+    buf.remove_prefix(4);
 
     // Next two bytes are the segment type, in big endian.
     // We expect 1028 to indicate IPTC data block.
-    if (((buf[0] << 8) + buf[1]) != 1028)
-        return;
-    buf += 2;
+    if (buf.size() < 2
+        || ((static_cast<unsigned char>(buf[0]) << 8)
+            + static_cast<unsigned char>(buf[1]))
+               != 1028)
+        return false;
+    buf.remove_prefix(2);
 
     // Next are 4 bytes of 0 padding, just skip it.
-    buf += 4;
+    if (buf.size() < 4)
+        return false;
+    buf.remove_prefix(4);
 
     // Next is 2 byte (big endian) giving the size of the segment
-    int segmentsize = (buf[0] << 8) + buf[1];
-    buf += 2;
+    if (buf.size() < 2)
+        return false;
+    size_t segmentsize = (static_cast<unsigned char>(buf[0]) << 8)
+                         + static_cast<unsigned char>(buf[1]);
+    buf.remove_prefix(2);
+
+    // Ensure the declared segment size does not exceed the remaining buffer,
+    // then restrict buf to exactly segmentsize bytes.
+    if (segmentsize > buf.size())
+        return false;
+    buf = buf.substr(0, segmentsize);
 
-    decode_iptc_iim(buf, segmentsize, m_spec);
+    return decode_iptc_iim(buf, m_spec);
 }
 
 OIIO_PLUGIN_NAMESPACE_END

src/libOpenImageIO/iptc.cpp

@@ -103,16 +103,26 @@ OIIO_NAMESPACE_3_1_BEGIN
 bool
 decode_iptc_iim(const void* iptc, int length, ImageSpec& spec)
 {
-    const unsigned char* buf = (const unsigned char*)iptc;
+    if (!iptc || length <= 0)
+        return false;
+    return decode_iptc_iim(string_view(reinterpret_cast<const char*>(iptc),
+                                       static_cast<size_t>(length)),
+                           spec);
+}
+
+
 
+bool
+decode_iptc_iim(string_view iptc, ImageSpec& spec)
+{
 #if DEBUG_IPTC_READ
-    std::cerr << "IPTC dump:\n";
-    for (int i = 0; i < std::min(length, 100); ++i) {
-        if (buf[i] >= ' ' && buf[i] < 128)
-            std::cerr << (char)buf[i] << ' ';
-        std::cerr << "(" << int(buf[i]) << ") ";
+    print(stderr, "IPTC dump (len={}):\n", iptc.size());
+    for (size_t i = 0; i < std::min(iptc.size(), size_t(1000)); ++i) {
+        if (iptc[i] >= ' ' && iptc[i] < 128)
+            print(stderr, "{:c} ", iptc[i]);
+        print(stderr, "({:d}) ", uint32_t(static_cast<unsigned char>(iptc[i])));
     }
-    std::cerr << "\n";
+    print(stderr, "\n");
 #endif
 
     // Now there are a series of data blocks.  Each one starts with 1C
@@ -121,28 +131,27 @@ decode_iptc_iim(const void* iptc, int length, ImageSpec& spec)
     // repeats until we've used up the whole segment buffer, or I guess
     // until we don't find another 1C 02 tag start.
     // N.B. I don't know why, but Picasa sometimes uses 1C 01 !
-    while (length >= 5 && buf[0] == 0x1c
-           && (buf[1] == 0x02 || buf[1] == 0x01)) {
-        int secondbyte = buf[1];
-        int tagtype    = buf[2];
-        int tagsize    = (buf[3] << 8) + buf[4];
-        buf += 5;
-        length -= 5;
-        tagsize = std::min(tagsize, length);
+    while (iptc.size() >= 5 && iptc[0] == 0x1c
+           && (iptc[1] == 0x02 || iptc[1] == 0x01)) {
+        int secondbyte = static_cast<unsigned char>(iptc[1]);
+        int tagtype    = static_cast<unsigned char>(iptc[2]);
+        size_t tagsize = (static_cast<unsigned char>(iptc[3]) << 8)
+                         + static_cast<unsigned char>(iptc[4]);
+        iptc.remove_prefix(5);
+        tagsize = std::min(tagsize, iptc.size());
 
 #if DEBUG_IPTC_READ
-        std::cerr << "iptc tag " << tagtype << ", size=" << tagsize << ":\n";
-        for (int i = 0; i < tagsize; ++i) {
-            if (buf[i] >= ' ')
-                std::cerr << (char)buf[i] << ' ';
-            std::cerr << "(" << (int)(unsigned char)buf[i] << ") ";
+        print(stderr, "iptc tag {}, size={}:\n", tagtype, tagsize);
+        for (size_t i = 0; i < tagsize; ++i) {
+            if (iptc[i] >= ' ')
+                print(stderr, "{:c} ", iptc[i]);
+            print(stderr, "({:d}) ", static_cast<unsigned char>(iptc[i]));
         }
-        std::cerr << "\n";
+        print(stderr, "\n");
 #endif
 
         if (secondbyte == 0x02) {
-            std::string s((const char*)buf, tagsize);
-
+            std::string s = iptc.substr(0, tagsize);
             for (int i = 0; iimtag[i].name; ++i) {
                 if (tagtype == iimtag[i].tag) {
                     if (iimtag[i].repeatable) {
@@ -169,8 +178,7 @@ decode_iptc_iim(const void* iptc, int length, ImageSpec& spec)
             }
         }
 
-        buf += tagsize;
-        length -= tagsize;
+        iptc.remove_prefix(tagsize);
     }
 
     return true;

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

@@ -44,3 +44,5 @@ src/corrupt-icc-4552.jpg : 1500 x 1000, 3 channel, uint8 jpeg
     ICCProfile:rendering_intent: "Unknown"
     jpeg:subsampling: "4:2:0"
     oiio:ColorSpace: "srgb_rec709_scene"
+iinfo ERROR: "src/corrupt-iptc-8011.jpg" : JPEG error: Corrupt JPEG data: 256 extraneous bytes before marker 0xdb ("src/corrupt-iptc-8011.jpg")
+Corrupted IPTC data

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

@@ -43,3 +43,5 @@ src/corrupt-icc-4552.jpg : 1500 x 1000, 3 channel, uint8 jpeg
     ICCProfile:rendering_intent: "Unknown"
     jpeg:subsampling: "4:2:0"
     oiio:ColorSpace: "srgb_rec709_scene"
+iinfo ERROR: "src/corrupt-iptc-8011.jpg" : JPEG error: Corrupt JPEG data: 256 extraneous bytes before marker 0xdb ("src/corrupt-iptc-8011.jpg")
+Corrupted IPTC data

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

@@ -44,3 +44,5 @@ src/corrupt-icc-4552.jpg : 1500 x 1000, 3 channel, uint8 jpeg
     ICCProfile:rendering_intent: "Unknown"
     jpeg:subsampling: "4:2:0"
     oiio:ColorSpace: "srgb_rec709_scene"
+iinfo ERROR: "src/corrupt-iptc-8011.jpg" : JPEG error: Corrupt JPEG data: 256 extraneous bytes before marker 0xdb ("src/corrupt-iptc-8011.jpg")
+Corrupted IPTC data

testsuite/jpeg-corrupt/ref/out.txt

@@ -44,3 +44,5 @@ src/corrupt-icc-4552.jpg : 1500 x 1000, 3 channel, uint8 jpeg
     ICCProfile:rendering_intent: "Unknown"
     jpeg:subsampling: "4:2:0"
     oiio:ColorSpace: "srgb_rec709_scene"
+iinfo ERROR: "src/corrupt-iptc-8011.jpg" : JPEG error: Corrupt JPEG data: 256 extraneous bytes before marker 0xdb ("src/corrupt-iptc-8011.jpg")
+Corrupted IPTC data

testsuite/jpeg-corrupt/run.py

@@ -24,3 +24,6 @@
 
 # This file has a corrupted ICC profile block
 command += info_command ("src/corrupt-icc-4552.jpg", safematch=True)
+
+# This file had corrupted IPTC data
+command += info_command ("-v src/corrupt-iptc-8011.jpg", info_program="iinfo")