Merge pull request #1822 from Exiv2/mergify/bp/main/pr-1816

Add bounds-check to prevent out-of-bounds read in memcmp (backport #1816)
Exiv2 · Aug 1, 2021 · 0eacac6 · 0eacac6
2 parents c65941c + dd4659c
commit 0eacac6
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 8 deletions.
diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp
@@ -942,28 +942,35 @@ namespace Exiv2 {
                 assert(markerHasLength(marker));
                 assert(size >= 2); // Because this marker has a length field.
                 insertPos = count + 1;
-            } else if (skipApp1Exif == notfound && marker == app1_ && memcmp(buf.pData_ + 2, exifId_, 6) == 0) {
-                enforce(size >= 8, kerNoImageInInputData);
+            } else if (skipApp1Exif == notfound &&
+                       marker == app1_ &&
+                       size >= 8 && // prevent out-of-bounds read in memcmp on next line
+                       memcmp(buf.pData_ + 2, exifId_, 6) == 0) {
                 skipApp1Exif = count;
                 ++search;
                 rawExif.alloc(size - 8);
                 memcpy(rawExif.pData_, buf.pData_ + 8, size - 8);
-            } else if (skipApp1Xmp == notfound && marker == app1_ && memcmp(buf.pData_ + 2, xmpId_, 29) == 0) {
-                enforce(size >= 31, kerNoImageInInputData);
+            } else if (skipApp1Xmp == notfound &&
+                       marker == app1_ &&
+                       size >= 31 && // prevent out-of-bounds read in memcmp on next line
+                       memcmp(buf.pData_ + 2, xmpId_, 29) == 0) {
                 skipApp1Xmp = count;
                 ++search;
-            } else if (marker == app2_ && memcmp(buf.pData_ + 2, iccId_, 11) == 0) {
-                enforce(size >= 31, kerNoImageInInputData);
+            } else if (marker == app2_ &&
+                       size >= 13 && // prevent out-of-bounds read in memcmp on next line
+                       memcmp(buf.pData_ + 2, iccId_, 11) == 0) {
                 skipApp2Icc.push_back(count);
                 if (!foundIccData) {
                     ++search;
                     foundIccData = true;
                 }
-            } else if (!foundCompletePsData && marker == app13_ && memcmp(buf.pData_ + 2, Photoshop::ps3Id_, 14) == 0) {
+            } else if (!foundCompletePsData &&
+                       marker == app13_ &&
+                       size >= 16 && // prevent out-of-bounds read in memcmp on next line
+                       memcmp(buf.pData_ + 2, Photoshop::ps3Id_, 14) == 0) {
 #ifdef EXIV2_DEBUG_MESSAGES
                 std::cerr << "Found APP13 Photoshop PS3 segment\n";
 #endif
-                enforce(size >= 16, kerNoImageInInputData);
                 skipApp13Ps3.push_back(count);
                 // Append to psBlob
                 append(psBlob, buf.pData_ + 16, size - 16);

diff --git a/test/data/issue_1815_poc.jpg b/test/data/issue_1815_poc.jpg
diff --git a/tests/bugfixes/github/test_issue_1815.py b/tests/bugfixes/github/test_issue_1815.py
@@ -0,0 +1,17 @@
+# -*- coding: utf-8 -*-
+
+from system_tests import CaseMeta, CopyTmpFiles, path
+@CopyTmpFiles("$data_path/issue_1815_poc.jpg")
+
+class JpgImageDoWriteMetadataOutOfBoundsRead(metaclass=CaseMeta):
+    """
+    Regression test for the bug described in:
+    https://github.com/Exiv2/exiv2/issues/1815
+    """
+    url = "https://github.com/Exiv2/exiv2/issues/1815"
+
+    filename = path("$tmp_path/issue_1815_poc.jpg")
+    commands = ["$exiv2 rm $filename"]
+    stdout = [""]
+    stderr = [""]
+    retval = [0]