Fix out-of-bounds read in exif tag reading

This issue was recently introduced in c739023a50876e2a90588f915803b0140a95638e,
when the restriction that components>0 has been relaxed. We now need
to make sure that any tags that expect at least one component check
that this is the case.

4 files changed, 37 insertions, 0 deletions

diff --git a/ext/exif/exif.c b/ext/exif/exif.c
index 25cec40df8..f6eb26a997 100644
--- a/ext/exif/exif.c
+++ b/ext/exif/exif.c
@@ -3256,6 +3256,14 @@ static int exif_process_IFD_in_MAKERNOTE(image_info_type *ImageInfo, char * valu
 }
 /* }}} */
 
+#define REQUIRE_NON_EMPTY() do { \
+	if (byte_count == 0) { \
+		exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Process tag(x%04X=%s): Cannot be empty", tag, exif_get_tagname(tag, tagname, -12, tag_table)); \
+		return FALSE; \
+	} \
+} while (0)
+
+
 /* {{{ exif_process_IFD_TAG
  * Process one of the nested IFDs directories. */
 static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, char *offset_base, size_t IFDlength, size_t displacement, int section_index, int ReadNextIFD, tag_table_type tag_table)
@@ -3373,8 +3381,12 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
 	}
 #endif
 
+	/* NB: The following code may not assume that there is at least one component!
+	 * byte_count may be zero! */
+
 	if (section_index==SECTION_THUMBNAIL) {
 		if (!ImageInfo->Thumbnail.data) {
+			REQUIRE_NON_EMPTY();
 			switch(tag) {
 				case TAG_IMAGEWIDTH:
 				case TAG_COMP_IMAGE_WIDTH:
@@ -3457,6 +3469,7 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
 			case TAG_FNUMBER:
 				/* Simplest way of expressing aperture, so I trust it the most.
 				   (overwrite previously computed value if there is one) */
+				REQUIRE_NON_EMPTY();
 				ImageInfo->ApertureFNumber = (float)exif_convert_any_format(value_ptr, format, ImageInfo->motorola_intel);
 				break;
 
@@ -3465,6 +3478,7 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
 				/* More relevant info always comes earlier, so only use this field if we don't
 				   have appropriate aperture information yet. */
 				if (ImageInfo->ApertureFNumber == 0) {
+					REQUIRE_NON_EMPTY();
 					ImageInfo->ApertureFNumber
 						= (float)exp(exif_convert_any_format(value_ptr, format, ImageInfo->motorola_intel)*log(2)*0.5);
 				}
@@ -3476,6 +3490,7 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
 				   SHUTTERSPEED comes after EXPOSURE TIME
 				  */
 				if (ImageInfo->ExposureTime == 0) {
+					REQUIRE_NON_EMPTY();
 					ImageInfo->ExposureTime
 						= (float)(1/exp(exif_convert_any_format(value_ptr, format, ImageInfo->motorola_intel)*log(2)));
 				}
@@ -3485,20 +3500,24 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
 				break;
 
 			case TAG_COMP_IMAGE_WIDTH:
+				REQUIRE_NON_EMPTY();
 				ImageInfo->ExifImageWidth = exif_convert_any_to_int(value_ptr, exif_rewrite_tag_format_to_unsigned(format), ImageInfo->motorola_intel);
 				break;
 
 			case TAG_FOCALPLANE_X_RES:
+				REQUIRE_NON_EMPTY();
 				ImageInfo->FocalplaneXRes = exif_convert_any_format(value_ptr, format, ImageInfo->motorola_intel);
 				break;
 
 			case TAG_SUBJECT_DISTANCE:
 				/* Inidcates the distacne the autofocus camera is focused to.
 				   Tends to be less accurate as distance increases. */
+				REQUIRE_NON_EMPTY();
 				ImageInfo->Distance = (float)exif_convert_any_format(value_ptr, format, ImageInfo->motorola_intel);
 				break;
 
 			case TAG_FOCALPLANE_RESOLUTION_UNIT:
+				REQUIRE_NON_EMPTY();
 				switch((int)exif_convert_any_format(value_ptr, format, ImageInfo->motorola_intel)) {
 					case 1: ImageInfo->FocalplaneUnits = 25.4; break; /* inch */
 					case 2:
@@ -3541,6 +3560,7 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
 			case TAG_GPS_IFD_POINTER:
 			case TAG_INTEROP_IFD_POINTER:
 				if (ReadNextIFD) {
+					REQUIRE_NON_EMPTY();
 					char *Subdir_start;
 					int sub_section_index = 0;
 					switch(tag) {
diff --git a/ext/exif/tests/bug73737.phpt b/ext/exif/tests/bug73737.phpt
index 21eaf80585..f97f54c387 100644
--- a/ext/exif/tests/bug73737.phpt
+++ b/ext/exif/tests/bug73737.phpt
@@ -8,5 +8,7 @@ Bug #73737 (Crash when parsing a tag format)
 	var_dump($exif);
 ?>
 --EXPECTF--
+Warning: exif_thumbnail(bug73737.tiff): Process tag(x0100=ImageWidth ): Cannot be empty in %s on line %d
+
 Warning: exif_thumbnail(bug73737.tiff): Error in TIFF: filesize(x0030) less than start of IFD dir(x10102) in %s line %d
 bool(false)
diff --git a/ext/exif/tests/tag_with_illegal_zero_components.jpeg b/ext/exif/tests/tag_with_illegal_zero_components.jpeg
new file mode 100644
index 0000000000..c000b938df
--- /dev/null
+++ b/ext/exif/tests/tag_with_illegal_zero_components.jpeg
diff --git a/ext/exif/tests/tag_with_illegal_zero_components.phpt b/ext/exif/tests/tag_with_illegal_zero_components.phpt
new file mode 100644
index 0000000000..26422db7ca
--- /dev/null
+++ b/ext/exif/tests/tag_with_illegal_zero_components.phpt
@@ -0,0 +1,15 @@
+--TEST--
+OSS-Fuzz #17163: Out-of-bounds read due to tag with zero components
+--FILE--
+<?php
+
+var_dump(exif_read_data(__DIR__ . '/tag_with_illegal_zero_components.jpeg'));
+
+?>
+--EXPECTF--
+Warning: exif_read_data(tag_with_illegal_zero_components.jpeg): Process tag(x0202=JPEGInterch): Cannot be empty in %s on line %d
+
+Warning: exif_read_data(tag_with_illegal_zero_components.jpeg): File structure corrupted in %s on line %d
+
+Warning: exif_read_data(tag_with_illegal_zero_components.jpeg): Invalid JPEG file in %s on line %d
+bool(false)