Fix EXIF bugs where corrupted exif blocks could overrun memory

src/libOpenImageIO/exif.cpp

@@ -157,7 +157,7 @@ tiff_data_size(TIFFDataType tifftype)
     // Sizes of TIFFDataType members
     static size_t sizes[]    = { 0, 1, 1, 2, 4, 8, 1, 1, 2, 4, 8, 4, 8, 4 };
     const int num_data_sizes = sizeof(sizes) / sizeof(*sizes);
-    int dir_index            = (int)tifftype;
+    int dir_index            = bitcast<int>(tifftype);
     if (dir_index < 0 || dir_index >= num_data_sizes) {
         // Inform caller about corrupted entry.
         return -1;
@@ -360,9 +360,8 @@ makernote_handler(const TagInfo& /*taginfo*/, const TIFFDirEntry& dir,
     if (spec.get_string_attribute("Make") == "Canon") {
         std::vector<size_t> ifdoffsets { 0 };
         std::set<size_t> offsets_seen;
-        decode_ifd((unsigned char*)buf.data() + dir.tdir_offset, buf, spec,
-                   pvt::canon_maker_tagmap_ref(), offsets_seen, swapendian,
-                   offset_adjustment);
+        decode_ifd(buf, dir.tdir_offset, spec, pvt::canon_maker_tagmap_ref(),
+                   offsets_seen, swapendian, offset_adjustment);
     } else {
         // Maybe we just haven't parsed the Maker metadata yet?
         // Allow a second try later by just stashing the maker note offset.
@@ -668,8 +667,10 @@ add_exif_item_to_spec(ImageSpec& spec, const char* name,
         float* f = OIIO_ALLOCA(float, n);
         for (int i = 0; i < n; ++i) {
             unsigned int num, den;
-            num = ((const unsigned int*)dataptr)[2 * i + 0];
-            den = ((const unsigned int*)dataptr)[2 * i + 1];  //NOSONAR
+            memcpy(&num, dataptr + (2 * i) * sizeof(unsigned int),
+                   sizeof(unsigned int));
+            memcpy(&den, dataptr + (2 * i + 1) * sizeof(unsigned int),
+                   sizeof(unsigned int));  //NOSONAR
             if (swab) {
                 swap_endian(&num);
                 swap_endian(&den);
@@ -687,8 +688,9 @@ add_exif_item_to_spec(ImageSpec& spec, const char* name,
         float* f = OIIO_ALLOCA(float, n);
         for (int i = 0; i < n; ++i) {
             int num, den;
-            num = ((const int*)dataptr)[2 * i + 0];
-            den = ((const int*)dataptr)[2 * i + 1];  //NOSONAR
+            memcpy(&num, dataptr + (2 * i) * sizeof(int), sizeof(int));
+            memcpy(&den, dataptr + (2 * i + 1) * sizeof(int),
+                   sizeof(int));  //NOSONAR
             if (swab) {
                 swap_endian(&num);
                 swap_endian(&den);
@@ -767,7 +769,9 @@ read_exif_tag(ImageSpec& spec, const TIFFDirEntry* dirp, cspan<uint8_t> buf,
 
     // Make a copy of the pointed-to TIFF directory, swab the components
     // if necessary.
-    TIFFDirEntry dir = *dirp;
+    TIFFDirEntry dir;
+    memcpy(&dir, dirp, sizeof(TIFFDirEntry));
+    unsigned int unswapped_tdir_offset = dir.tdir_offset;
     if (swab) {
         swap_endian(&dir.tdir_tag);
         swap_endian(&dir.tdir_type);
@@ -785,7 +789,7 @@ read_exif_tag(ImageSpec& spec, const TIFFDirEntry* dirp, cspan<uint8_t> buf,
     if (dir.tdir_tag == TIFFTAG_EXIFIFD || dir.tdir_tag == TIFFTAG_GPSIFD) {
         // Special case: It's a pointer to a private EXIF directory.
         // Handle the whole thing recursively.
-        unsigned int offset = dirp->tdir_offset;  // int stored in offset itself
+        auto offset = unswapped_tdir_offset;  // int stored in offset itself
         if (swab)
             swap_endian(&offset);
         if (offset >= size_t(buf.size())) {
@@ -840,7 +844,7 @@ read_exif_tag(ImageSpec& spec, const TIFFDirEntry* dirp, cspan<uint8_t> buf,
     } else if (dir.tdir_tag == TIFFTAG_INTEROPERABILITYIFD) {
         // Special case: It's a pointer to a private EXIF directory.
         // Handle the whole thing recursively.
-        unsigned int offset = dirp->tdir_offset;  // int stored in offset itself
+        auto offset = unswapped_tdir_offset;  // int stored in offset itself
         if (swab)
             swap_endian(&offset);
         if (offset >= size_t(buf.size())) {
@@ -1004,23 +1008,32 @@ encode_exif_entry(const ParamValue& p, int tag, std::vector<TIFFDirEntry>& dirs,
 
 
 
-// Decode a raw Exif data block and save all the metadata in an
-// ImageSpec.  Return true if all is ok, false if the exif block was
+// Decode a raw Exif data block and save all the metadata in an ImageSpec. The
+// data is all inside buf. The ifd itself is at the position `ifd_offset`
+// within the buf. Return true if all is ok, false if the exif block was
 // somehow malformed.
-void
-pvt::decode_ifd(const unsigned char* ifd, cspan<uint8_t> buf, ImageSpec& spec,
+bool
+pvt::decode_ifd(cspan<uint8_t> buf, size_t ifd_offset, ImageSpec& spec,
                 const TagMap& tag_map, std::set<size_t>& ifd_offsets_seen,
                 bool swab, int offset_adjustment)
 {
     // Read the directory that the header pointed to.  It should contain
     // some number of directory entries containing tags to process.
-    unsigned short ndirs = *(const unsigned short*)ifd;
+    if (ifd_offset + 2 > std::size(buf)) {
+        return false;  // asking us to read beyond the buffer
+    }
+    auto ifd             = buf.data() + ifd_offset;
+    unsigned short ndirs = *(const unsigned short*)(buf.data() + ifd_offset);
     if (swab)
         swap_endian(&ndirs);
+    if (ifd_offset + 2 + ndirs * sizeof(TIFFDirEntry) > std::size(buf)) {
+        return false;  // asking us to read beyond the buffer
+    }
     for (int d = 0; d < ndirs; ++d)
         read_exif_tag(spec,
                       (const TIFFDirEntry*)(ifd + 2 + d * sizeof(TIFFDirEntry)),
                       buf, swab, offset_adjustment, ifd_offsets_seen, tag_map);
+    return true;
 }
 
 
@@ -1140,12 +1153,12 @@ decode_exif(cspan<uint8_t> exif, ImageSpec& spec)
     if (swab)
         swap_endian(&head.tiff_diroff);
 
-    const unsigned char* ifd = ((const unsigned char*)exif.data()
-                                + head.tiff_diroff);
     // keep track of IFD offsets we've already seen to avoid infinite
     // recursion if there are circular references.
     std::set<size_t> ifd_offsets_seen;
-    decode_ifd(ifd, exif, spec, exif_tagmap_ref(), ifd_offsets_seen, swab);
+    if (!decode_ifd(exif, head.tiff_diroff, spec, exif_tagmap_ref(),
+                    ifd_offsets_seen, swab))
+        return false;
 
     // A few tidbits to look for
     ParamValue* p;
@@ -1172,9 +1185,10 @@ decode_exif(cspan<uint8_t> exif, ImageSpec& spec)
     int makernote_offset = spec.get_int_attribute("oiio:MakerNoteOffset");
     if (makernote_offset > 0) {
         if (Strutil::iequals(spec.get_string_attribute("Make"), "Canon")) {
-            decode_ifd((unsigned char*)exif.data() + makernote_offset, exif,
-                       spec, pvt::canon_maker_tagmap_ref(), ifd_offsets_seen,
-                       swab);
+            if (!decode_ifd(exif, makernote_offset, spec,
+                            pvt::canon_maker_tagmap_ref(), ifd_offsets_seen,
+                            swab))
+                return false;
         }
         // Now we can erase the attrib we used to pass the message about
         // the maker note offset.

src/libOpenImageIO/exif.h

@@ -31,12 +31,12 @@ namespace pvt {
 inline const void*
 dataptr(const TIFFDirEntry& td, cspan<uint8_t> data, int offset_adjustment)
 {
-    int len = tiff_data_size(td);
+    size_t len = tiff_data_size(td);
     if (len <= 4)
         return (const char*)&td.tdir_offset;
     else {
         int offset = td.tdir_offset + offset_adjustment;
-        if (offset < 0 || offset + len > (int)data.size())
+        if (offset < 0 || size_t(offset) + len > std::size(data))
             return nullptr;  // out of bounds!
         return (const char*)data.data() + offset;
     }
@@ -113,7 +113,7 @@ void append_tiff_dir_entry (std::vector<TIFFDirEntry> &dirs,
                             size_t offset_override = 0,
                             OIIO::endian endianreq = OIIO::endian::native);
 
-void decode_ifd (const unsigned char *ifd, cspan<uint8_t> buf,
+bool decode_ifd (cspan<uint8_t> buf, size_t ifd_offset,
                  ImageSpec &spec, const TagMap& tag_map,
                  std::set<size_t>& ifd_offsets_seen, bool swab=false,
                  int offset_adjustment=0);

src/psd.imageio/psdinput.cpp

@@ -309,8 +309,9 @@ class PSDInput final : public ImageInput {
     // Convert from photoshop native alpha to
     // associated/premultiplied
     template<class T>
-    void removeBackground(T* data, int size, int nchannels, int alpha_channel,
-                          double* background)
+    OIIO_NO_SANITIZE_UNDEFINED void
+    removeBackground(T* data, int size, int nchannels, int alpha_channel,
+                     double* background)
     {
         // RGB = CompRGB - (1 - alpha) * Background;
         double scale = std::numeric_limits<T>::is_integer

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

@@ -8,3 +8,13 @@ src/corrupt-exif.jpg :  256 x  256, 3 channel, uint8 jpeg
     YResolution: 300
     jpeg:subsampling: "4:2:0"
     oiio:ColorSpace: "sRGB"
+Reading src/corrupt-exif-1626.jpg
+src/corrupt-exif-1626.jpg :  256 x  256, 3 channel, uint8 jpeg
+    SHA-1: 6CBB7DB602A67DB300AB28842F20F6DCA02B82B1
+    channel list: R, G, B
+    Orientation: 1 (normal)
+    ResolutionUnit: "in"
+    XResolution: 300
+    YResolution: 300
+    jpeg:subsampling: "4:2:0"
+    oiio:ColorSpace: "sRGB"

testsuite/jpeg-corrupt/ref/out.txt

@@ -8,3 +8,13 @@ src/corrupt-exif.jpg :  256 x  256, 3 channel, uint8 jpeg
     YResolution: 300
     jpeg:subsampling: "4:2:0"
     oiio:ColorSpace: "sRGB"
+Reading src/corrupt-exif-1626.jpg
+src/corrupt-exif-1626.jpg :  256 x  256, 3 channel, uint8 jpeg
+    SHA-1: 6CBB7DB602A67DB300AB28842F20F6DCA02B82B1
+    channel list: R, G, B
+    Orientation: 1 (normal)
+    ResolutionUnit: "in"
+    XResolution: 300
+    YResolution: 300
+    jpeg:subsampling: "4:2:0"
+    oiio:ColorSpace: "sRGB"

testsuite/jpeg-corrupt/run.py

@@ -1,10 +1,14 @@
 #!/usr/bin/env python
 
+failureok = 1
+redirect = ' >> out.txt 2>&1 '
+
 # This file has a corrupted Exif block in the metadata. It used to
 # crash on some platforms, on others would be caught by address sanitizer.
 # Fixed by #1635. This test serves to guard against regressions.
 command += info_command ("src/corrupt-exif.jpg", safematch=True)
 
-# Checking the error output is important for this test
-outputs = [ "out.txt" ]
-failureok = 1
+# This file has a corrupted Exif block that makes it look like one item has a
+# nonsensical length, that before being fixed, caused a buffer overrun.
+command += info_command ("src/corrupt-exif-1626.jpg", safematch=True)
+

testsuite/psd/ref/out.txt

@@ -1064,3 +1064,7 @@ src/layer-mask.psd   :   10 x   10, 4 channel, uint8 psd
     stEvt:instanceID: "xmp.iid:a64763c8-be7b-ff48-b857-0f1ee8e5da2b; xmp.iid:9847e4c5-ca7e-fa42-9c7f-5fe6373d31de; xmp.iid:ddf40b95-b12c-744e-a1a9-5e7724fe4ca9"
     stEvt:softwareAgent: "Adobe Photoshop CC 2017 (Windows)"
     stEvt:when: "2017-07-13T10:26:10+09:00; 2017-07-13T10:32:41+09:00; 2017-07-13T11:42:54+09:00"
+oiiotool ERROR: read : Failed to decode Exif data
+failed to open "src/crash-psd-exif-1632.psd": failed load_resources
+Full command line was:
+> oiiotool --info -v -a --hash src/crash-psd-exif-1632.psd

testsuite/psd/run.py

@@ -1,5 +1,7 @@
 #!/usr/bin/env python
 
+redirect = ' >> out.txt 2>&1 '
+
 files = [ "psd_123.psd", "psd_123_nomaxcompat.psd", "psd_bitmap.psd",
           "psd_indexed_trans.psd", "psd_rgb_8.psd", "psd_rgb_16.psd",
           "psd_rgb_32.psd", "psd_rgba_8.psd" ]
@@ -8,3 +10,6 @@
 
 command += info_command ("src/different-mask-size.psd")
 command += info_command ("src/layer-mask.psd")
+
+# This file has a corrupted Exif block
+command += info_command ("src/crash-psd-exif-1632.psd", failureok = 1)

testsuite/runtest.py

@@ -215,7 +215,8 @@ def oiio_app (app):
 # the file "out.txt".  If 'safematch' is nonzero, it will exclude printing
 # of fields that tend to change from run to run or release to release.
 def info_command (file, extraargs="", safematch=False, hash=True,
-                  verbose=True, info_program="oiiotool") :
+                  verbose=True, silent=False, concat=True, failureok=False,
+                  info_program="oiiotool") :
     args = ""
     if info_program == "oiiotool" :
         args += " --info"
@@ -225,8 +226,15 @@ def info_command (file, extraargs="", safematch=False, hash=True,
         args += " --no-metamatch \"DateTime|Software|OriginatingProgram|ImageHistory\""
     if hash :
         args += " --hash"
-    return (oiio_app(info_program) + args + " " + extraargs
-            + " " + make_relpath(file,tmpdir) + redirect + ";\n")
+    cmd = (oiio_app(info_program) + args + " " + extraargs
+            + " " + make_relpath(file,tmpdir))
+    if not silent :
+        cmd += redirect
+    if failureok :
+        cmd += " || true "
+    if concat:
+        cmd += " ;\n"
+    return cmd
 
 
 # Construct a command that will compare two images, appending output to