Fix EXIF bugs where corrupted exif blocks could overrun memory (Acade…

…mySoftwareFoundation#3627)

In one case, we actually had a check for this, but an assignment to an
int made the nonsensical offset appear negative, and we only tested
whether the necessary offset it was bigger than the buffer size.
Keeping it as (unsigned) size_t makes the test work as intended.

In another case, there were several places where we never checked that
we were staying within the exif block, and here we address this by
changing the utility decode_ifd so instead of passing it a pointer to
the ifd, it passes the offset (the pointer turned out to always be
inside the buffer) so it can check the extent for subsequent accesses.

Also some fixes related to squashing undefined behavior sanitizer
cases.

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