jpegrutils: check EXIF offsets before adding them in getExifAppleHeadroom

lib/src/jpegrutils.cpp

@@ -509,6 +509,21 @@ static bool readS32(const uint8_t* data, size_t size, int32_t* value, size_t* of
   return true;
 }
 
+// Resolves an offset stored in the EXIF data, relative to base, to an offset in
+// the buffer. Fails if the result lies past the end of the buffer. Every offset
+// read from the EXIF data must go through this, so that the sum never wraps.
+static bool resolveOffset(size_t base, uint32_t relOffset, size_t size, size_t* offset) {
+  if (base > size || relOffset > size - base) return false;
+  *offset = base + relOffset;
+  return true;
+}
+
+static bool matchBytes(const uint8_t* data, size_t size, size_t offset, const uint8_t* expected,
+                       size_t expectedSize) {
+  if (offset > size || size - offset < expectedSize) return false;
+  return !memcmp(&data[offset], expected, expectedSize);
+}
+
 static bool getExifAppleHeadroom(const uint8_t* exif, size_t size, float* altHeadroom) {
   *altHeadroom = 0.0f;
   size_t offset = 0;
@@ -554,7 +569,9 @@ static bool getExifAppleHeadroom(const uint8_t* exif, size_t size, float* altHea
   const size_t tiffHeaderOffset = offset - 8;
 
   while (offsetToIfd != 0 && numIfds++ < maxIfds) {
-    offset = tiffHeaderOffset + offsetToIfd;
+    size_t ifdOffset;
+    if (!resolveOffset(tiffHeaderOffset, offsetToIfd, size, &ifdOffset)) return false;
+    offset = ifdOffset;
     bool offsetToNextIfdAlreadySet = false;
 
     uint16_t fieldCount;
@@ -575,30 +592,26 @@ static bool getExifAppleHeadroom(const uint8_t* exif, size_t size, float* altHea
         offsetToNextIfdAlreadySet = true;
         break;
       } else if (tagId == 0x927c) {  // Maker Notes
-        uint32_t makerNotesOffset = tagData;
-        if (tiffHeaderOffset + makerNotesOffset + appleMakerNotesHeaderSize <= size &&
-            !memcmp(&exif[tiffHeaderOffset + makerNotesOffset], appleMakerNotesHeader,
-                    appleMakerNotesHeaderSize)) {
-          offsetToIfd = makerNotesOffset + (uint32_t)appleMakerNotesHeaderSize;
+        size_t makerNotesOffset;
+        // The first IFD follows the header; its offset must fit in 32 bits too.
+        if (tagData <= UINT32_MAX - appleMakerNotesHeaderSize &&
+            resolveOffset(tiffHeaderOffset, tagData, size, &makerNotesOffset) &&
+            matchBytes(exif, size, makerNotesOffset, appleMakerNotesHeader,
+                       appleMakerNotesHeaderSize)) {
+          offsetToIfd = tagData + (uint32_t)appleMakerNotesHeaderSize;
           inAppleMakerNotes = true;
           offsetToNextIfdAlreadySet = true;
           isBigEndian = true;  // Apple Maker Notes are always big endian.
           break;
         }
       } else if (inAppleMakerNotes && (tagId == 33 || tagId == 48) && dataFormat == 10) {
-        // Guard against size_t underflow before the subtraction. If
-        // (tiffHeaderOffset + offsetToIfd) is smaller than the Apple Maker
-        // Notes header size, the unsigned arithmetic would wrap around to a
-        // value close to SIZE_MAX, and the subsequent readU32/readS32 bounds
-        // check (*offset + 4 > size) would also overflow and silently pass,
-        // resulting in an out-of-bounds read.
-        if (tiffHeaderOffset + offsetToIfd < appleMakerNotesHeaderSize) return false;
-        size_t tmpOffset =
-            tiffHeaderOffset + offsetToIfd - appleMakerNotesHeaderSize;
-        // Guard against size_t overflow when adding the attacker-controlled
-        // tagData to the computed base offset.
-        if (tmpOffset > SIZE_MAX - (size_t)tagData) return false;
-        tmpOffset += (size_t)tagData;
+        // Offsets in the Apple Maker Notes are relative to the Maker Notes
+        // header, which precedes their first IFD.
+        if (ifdOffset < appleMakerNotesHeaderSize) return false;
+        size_t tmpOffset;
+        if (!resolveOffset(ifdOffset - appleMakerNotesHeaderSize, tagData, size, &tmpOffset)) {
+          return false;
+        }
         int32_t numerator;
         uint32_t denominator;
         if (!readS32(exif, size, &numerator, &tmpOffset, isBigEndian)) return false;

tests/jpegr_test.cpp

@@ -1712,6 +1712,121 @@ TEST(JpegRTest, writeXmpThenRead) {
   EXPECT_TRUE(metadata_read.use_base_cg);
 }
 
+// Apple XMP without HDRGainMapHeadroom, so that the headroom is read from EXIF.
+static std::vector<uint8_t> appleXmpWithoutHeadroom() {
+  const std::string nameSpace = "http://ns.adobe.com/xap/1.0/\0";
+  const size_t nameSpaceLength = nameSpace.size() + 1;
+  const std::string xmp =
+      "<x:xmpmeta xmlns:x=\"adobe:ns:meta/\"><rdf:RDF "
+      "xmlns:rdf=\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\"><rdf:Description "
+      "xmlns:HDRGainMap=\"http://ns.apple.com/HDRGainMap/1.0/\">"
+      "<HDRGainMap:HDRGainMapVersion>65536</HDRGainMap:HDRGainMapVersion>"
+      "</rdf:Description></rdf:RDF></x:xmpmeta>";
+  std::vector<uint8_t> xmpData;
+  xmpData.insert(xmpData.end(), reinterpret_cast<const uint8_t*>(nameSpace.c_str()),
+                 reinterpret_cast<const uint8_t*>(nameSpace.c_str()) + nameSpaceLength);
+  xmpData.insert(xmpData.end(), xmp.begin(), xmp.end());
+  return xmpData;
+}
+
+static void putU16(std::vector<uint8_t>& v, size_t pos, uint16_t x, bool bigEndian) {
+  v[pos + (bigEndian ? 0 : 1)] = x >> 8;
+  v[pos + (bigEndian ? 1 : 0)] = x & 0xff;
+}
+
+static void putU32(std::vector<uint8_t>& v, size_t pos, uint32_t x, bool bigEndian) {
+  putU16(v, pos + (bigEndian ? 0 : 2), x >> 16, bigEndian);
+  putU16(v, pos + (bigEndian ? 2 : 0), x & 0xffff, bigEndian);
+}
+
+// EXIF with little endian TIFF data: IFD0 holds an Exif sub-IFD pointer, the sub-IFD
+// holds the Maker Notes, which are Apple Maker Notes with tags 33 and 48 (rationals).
+// Offsets of the IFDs and rationals are parameters, relative to the TIFF header and the
+// Maker Notes respectively.
+struct AppleExifOffsets {
+  uint32_t ifd0 = 8;
+  uint32_t exifIfd = 26;
+  uint32_t makerNotes = 44;
+  uint32_t tag33 = 46;
+  uint32_t tag48 = 54;
+};
+
+static std::vector<uint8_t> appleExif(const AppleExifOffsets& o) {
+  const size_t kTiff = 6;
+  std::vector<uint8_t> exif(kTiff + 44 + 62, 0);
+  memcpy(exif.data(), "Exif\0\0II*\0", 10);
+  putU32(exif, kTiff + 4, o.ifd0, false);
+  // IFD0 at TIFF + 8: one entry, the Exif sub-IFD pointer.
+  putU16(exif, kTiff + 8, 1, false);
+  putU16(exif, kTiff + 10, 0x8769, false);
+  putU16(exif, kTiff + 12, 4, false);
+  putU32(exif, kTiff + 14, 1, false);
+  putU32(exif, kTiff + 18, o.exifIfd, false);
+  // Exif sub-IFD at TIFF + 26: one entry, the Maker Notes.
+  putU16(exif, kTiff + 26, 1, false);
+  putU16(exif, kTiff + 28, 0x927c, false);
+  putU16(exif, kTiff + 30, 7, false);
+  putU32(exif, kTiff + 32, 62, false);
+  putU32(exif, kTiff + 36, o.makerNotes, false);
+  // Apple Maker Notes at TIFF + 44, big endian, offsets relative to their start.
+  const size_t kMaker = kTiff + 44;
+  const uint8_t kHeader[] = {'A', 'p', 'p', 'l', 'e', ' ', 'i', 'O', 'S', 0, 0, 1, 'M', 'M'};
+  memcpy(&exif[kMaker], kHeader, sizeof(kHeader));
+  putU16(exif, kMaker + 14, 2, true);
+  putU16(exif, kMaker + 16, 33, true);
+  putU16(exif, kMaker + 18, 10, true);
+  putU32(exif, kMaker + 20, 1, true);
+  putU32(exif, kMaker + 24, o.tag33, true);
+  putU16(exif, kMaker + 28, 48, true);
+  putU16(exif, kMaker + 30, 10, true);
+  putU32(exif, kMaker + 32, 1, true);
+  putU32(exif, kMaker + 36, o.tag48, true);
+  // Rationals: tag 33 = 1/2, tag 48 = 5/1000.
+  putU32(exif, kMaker + 46, 1, true);
+  putU32(exif, kMaker + 50, 2, true);
+  putU32(exif, kMaker + 54, 5, true);
+  putU32(exif, kMaker + 58, 1000, true);
+  return exif;
+}
+
+TEST(JpegRTest, readAppleHeadroomFromExif) {
+  std::vector<uint8_t> xmpData = appleXmpWithoutHeadroom();
+  std::vector<uint8_t> exif = appleExif(AppleExifOffsets());
+  uhdr_gainmap_metadata_ext_t metadata;
+  ASSERT_EQ(getMetadataFromXMP(xmpData.data(), xmpData.size(), exif.data(), exif.size(), &metadata)
+                .error_code,
+            UHDR_CODEC_OK);
+  // maker33 < 1 and maker48 <= 0.01: stops = -20 * maker48 + 1.8.
+  EXPECT_FLOAT_EQ(metadata.hdr_capacity_max, (float)pow(2.0, 1.7));
+  EXPECT_FLOAT_EQ(metadata.max_content_boost[0], (float)pow(2.0, 1.7));
+}
+
+// Every offset read from the EXIF data must be rejected when it points past the end of the
+// data, including values that would wrap around when added to the position of the TIFF
+// header or of the Maker Notes.
+TEST(JpegRTest, rejectAppleExifOffsetsPastEnd) {
+  std::vector<uint8_t> xmpData = appleXmpWithoutHeadroom();
+  const size_t exifSize = appleExif(AppleExifOffsets()).size();
+  const uint32_t kBadOffsets[] = {(uint32_t)exifSize, (uint32_t)exifSize - 1,
+                                  0xffffffffu,        0xfffffffau,
+                                  0xfffffff2u,        0xffffffe0u,
+                                  0xffffffd2u,        0x80000000u};
+  for (uint32_t bad : kBadOffsets) {
+    for (int field = 0; field < 5; ++field) {
+      AppleExifOffsets o;
+      uint32_t* fields[] = {&o.ifd0, &o.exifIfd, &o.makerNotes, &o.tag33, &o.tag48};
+      *fields[field] = bad;
+      std::vector<uint8_t> exif = appleExif(o);
+      uhdr_gainmap_metadata_ext_t metadata;
+      EXPECT_EQ(
+          getMetadataFromXMP(xmpData.data(), xmpData.size(), exif.data(), exif.size(), &metadata)
+              .error_code,
+          UHDR_CODEC_ERROR)
+          << "field " << field << " offset " << bad;
+    }
+  }
+}
+
 TEST(JpegRTest, decodeApple) {
   JpegR decoder;
   uhdr_compressed_image_t uhdrCompressedImg;