Fix GH-20582: Heap Buffer Overflow in iptcembed

If you can extend the file between the file size gathering (resulting in a buffer allocation), and reading / writing to the file you can trigger a TOC-TOU where you write out of bounds. To solve this, add extra bound checks and make sure that write actions always fail when going out of bounds. The easiest way to trigger this is via a pipe, which is used in the test, but it should be possible with a regular file and a quick race condition as well. Closes GH-20591.
2026-03-24 08:12:21 +01:00 · 2025-11-25 22:58:09 +01:00
parent 494dd97521
commit 99ed66b49f
3 changed files with 105 additions and 28 deletions
--- a/1
+++ b/1
@@ -52,6 +52,7 @@ PHP                                                                        NEWS

 - Standard:
  . Fix error check for proc_open() command. (ndossche)
+  . Fixed bug GH-20582 (Heap Buffer Overflow in iptcembed). (ndossche)

 18 Dec 2025, PHP 8.3.29

--- a/ext/standard/iptc.c
+++ b/ext/standard/iptc.c
@@ -73,19 +73,24 @@
 #define M_APP15 0xef

 /* {{{ php_iptc_put1 */
-static int php_iptc_put1(FILE *fp, int spool, unsigned char c, unsigned char **spoolbuf)
+static int php_iptc_put1(FILE *fp, int spool, unsigned char c, unsigned char **spoolbuf, const unsigned char *spoolbuf_end)
 {
 	if (spool > 0)
 		PUTC(c);

-	if (spoolbuf) *(*spoolbuf)++ = c;
+	if (spoolbuf) {
+		if (UNEXPECTED(*spoolbuf >= spoolbuf_end)) {
+			return EOF;
+		}
+		*(*spoolbuf)++ = c;
+	}

 	return c;
 }
 /* }}} */

 /* {{{ php_iptc_get1 */
-static int php_iptc_get1(FILE *fp, int spool, unsigned char **spoolbuf)
+static int php_iptc_get1(FILE *fp, int spool, unsigned char **spoolbuf, const unsigned char *spoolbuf_end)
 {
 	int c;
 	char cc;
@@ -99,66 +104,71 @@ static int php_iptc_get1(FILE *fp, int spool, unsigned char **spoolbuf)
 		PUTC(cc);
 	}

-	if (spoolbuf) *(*spoolbuf)++ = c;
+	if (spoolbuf) {
+		if (UNEXPECTED(*spoolbuf >= spoolbuf_end)) {
+			return EOF;
+		}
+		*(*spoolbuf)++ = c;
+	}

 	return c;
 }
 /* }}} */

 /* {{{ php_iptc_read_remaining */
-static int php_iptc_read_remaining(FILE *fp, int spool, unsigned char **spoolbuf)
+static int php_iptc_read_remaining(FILE *fp, int spool, unsigned char **spoolbuf, const unsigned char *spoolbuf_end)
 {
-	while (php_iptc_get1(fp, spool, spoolbuf) != EOF) continue;
+	while (php_iptc_get1(fp, spool, spoolbuf, spoolbuf_end) != EOF) continue;

 	return M_EOI;
 }
 /* }}} */

 /* {{{ php_iptc_skip_variable */
-static int php_iptc_skip_variable(FILE *fp, int spool, unsigned char **spoolbuf)
+static int php_iptc_skip_variable(FILE *fp, int spool, unsigned char **spoolbuf, const unsigned char *spoolbuf_end)
 {
 	unsigned int  length;
 	int c1, c2;

-	if ((c1 = php_iptc_get1(fp, spool, spoolbuf)) == EOF) return M_EOI;
+	if ((c1 = php_iptc_get1(fp, spool, spoolbuf, spoolbuf_end)) == EOF) return M_EOI;

-	if ((c2 = php_iptc_get1(fp, spool, spoolbuf)) == EOF) return M_EOI;
+	if ((c2 = php_iptc_get1(fp, spool, spoolbuf, spoolbuf_end)) == EOF) return M_EOI;

 	length = (((unsigned char) c1) << 8) + ((unsigned char) c2);

 	length -= 2;

 	while (length--)
-		if (php_iptc_get1(fp, spool, spoolbuf) == EOF) return M_EOI;
+		if (php_iptc_get1(fp, spool, spoolbuf, spoolbuf_end) == EOF) return M_EOI;

 	return 0;
 }
 /* }}} */

 /* {{{ php_iptc_next_marker */
-static int php_iptc_next_marker(FILE *fp, int spool, unsigned char **spoolbuf)
+static int php_iptc_next_marker(FILE *fp, int spool, unsigned char **spoolbuf, const unsigned char *spoolbuf_end)
 {
 	int c;

 	/* skip unimportant stuff */

-	c = php_iptc_get1(fp, spool, spoolbuf);
+	c = php_iptc_get1(fp, spool, spoolbuf, spoolbuf_end);

 	if (c == EOF) return M_EOI;

 	while (c != 0xff) {
-		if ((c = php_iptc_get1(fp, spool, spoolbuf)) == EOF)
+		if ((c = php_iptc_get1(fp, spool, spoolbuf, spoolbuf_end)) == EOF)
 			return M_EOI; /* we hit EOF */
 	}

 	/* get marker byte, swallowing possible padding */
 	do {
-		c = php_iptc_get1(fp, 0, 0);
+		c = php_iptc_get1(fp, 0, 0, NULL);
 		if (c == EOF)
 			return M_EOI;       /* we hit EOF */
 		else
 		if (c == 0xff)
-			php_iptc_put1(fp, spool, (unsigned char)c, spoolbuf);
+			php_iptc_put1(fp, spool, (unsigned char)c, spoolbuf, spoolbuf_end);
 	} while (c == 0xff);

 	return (unsigned int) c;
@@ -178,6 +188,7 @@ PHP_FUNCTION(iptcembed)
 	size_t inx;
 	zend_string *spoolbuf = NULL;
 	unsigned char *poi = NULL;
+	unsigned char *spoolbuf_end = NULL;
 	zend_stat_t sb = {0};
 	bool written = 0;

@@ -210,10 +221,11 @@ PHP_FUNCTION(iptcembed)

 		spoolbuf = zend_string_safe_alloc(1, iptcdata_len + sizeof(psheader) + 1024 + 1, sb.st_size, 0);
 		poi = (unsigned char*)ZSTR_VAL(spoolbuf);
+		spoolbuf_end = poi + ZSTR_LEN(spoolbuf);
 		memset(poi, 0, iptcdata_len + sizeof(psheader) + sb.st_size + 1024 + 1);
 	}

-	if (php_iptc_get1(fp, spool, poi?&poi:0) != 0xFF) {
+	if (php_iptc_get1(fp, spool, poi?&poi:0, spoolbuf_end) != 0xFF) {
 		fclose(fp);
 		if (spoolbuf) {
 			zend_string_efree(spoolbuf);
@@ -221,7 +233,8 @@ PHP_FUNCTION(iptcembed)
 		RETURN_FALSE;
 	}

-	if (php_iptc_get1(fp, spool, poi?&poi:0) != 0xD8) {
+	if (php_iptc_get1(fp, spool, poi?&poi:0, spoolbuf_end) != 0xD8) {
+err:
 		fclose(fp);
 		if (spoolbuf) {
 			zend_string_efree(spoolbuf);
@@ -230,20 +243,22 @@ PHP_FUNCTION(iptcembed)
 	}

 	while (!done) {
-		marker = php_iptc_next_marker(fp, spool, poi?&poi:0);
+		marker = php_iptc_next_marker(fp, spool, poi?&poi:0, spoolbuf_end);

 		if (marker == M_EOI) { /* EOF */
 			break;
 		} else if (marker != M_APP13) {
-			php_iptc_put1(fp, spool, (unsigned char)marker, poi?&poi:0);
+			if (php_iptc_put1(fp, spool, (unsigned char)marker, poi?&poi:0, spoolbuf_end) < 0) {
+				goto err;
+			}
 		}

 		switch (marker) {
 			case M_APP13:
 				/* we are going to write a new APP13 marker, so don't output the old one */
-				php_iptc_skip_variable(fp, 0, 0);
+				php_iptc_skip_variable(fp, 0, 0, spoolbuf_end);
 				fgetc(fp); /* skip already copied 0xFF byte */
-				php_iptc_read_remaining(fp, spool, poi?&poi:0);
+				php_iptc_read_remaining(fp, spool, poi?&poi:0, spoolbuf_end);
 				done = 1;
 				break;

@@ -256,7 +271,7 @@ PHP_FUNCTION(iptcembed)
 				}
 				written = 1;

-				php_iptc_skip_variable(fp, spool, poi?&poi:0);
+				php_iptc_skip_variable(fp, spool, poi?&poi:0, spoolbuf_end);

 				if (iptcdata_len & 1) {
 					iptcdata_len++; /* make the length even */
@@ -266,25 +281,33 @@ PHP_FUNCTION(iptcembed)
 				psheader[ 3 ] = (iptcdata_len+28)&0xff;

 				for (inx = 0; inx < 28; inx++) {
-					php_iptc_put1(fp, spool, psheader[inx], poi?&poi:0);
+					if (php_iptc_put1(fp, spool, psheader[inx], poi?&poi:0, spoolbuf_end) < 0) {
+						goto err;
+					}
 				}

-				php_iptc_put1(fp, spool, (unsigned char)(iptcdata_len>>8), poi?&poi:0);
-				php_iptc_put1(fp, spool, (unsigned char)(iptcdata_len&0xff), poi?&poi:0);
+				if (php_iptc_put1(fp, spool, (unsigned char)(iptcdata_len>>8), poi?&poi:0, spoolbuf_end) < 0) {
+					goto err;
+				}
+				if (php_iptc_put1(fp, spool, (unsigned char)(iptcdata_len&0xff), poi?&poi:0, spoolbuf_end) < 0) {
+					goto err;
+				}

 				for (inx = 0; inx < iptcdata_len; inx++) {
-					php_iptc_put1(fp, spool, iptcdata[inx], poi?&poi:0);
+					if (php_iptc_put1(fp, spool, iptcdata[inx], poi?&poi:0, spoolbuf_end) < 0) {
+						goto err;
+					}
 				}
 				break;

 			case M_SOS:
 				/* we hit data, no more marker-inserting can be done! */
-				php_iptc_read_remaining(fp, spool, poi?&poi:0);
+				php_iptc_read_remaining(fp, spool, poi?&poi:0, spoolbuf_end);
 				done = 1;
 				break;

 			default:
-				php_iptc_skip_variable(fp, spool, poi?&poi:0);
+				php_iptc_skip_variable(fp, spool, poi?&poi:0, spoolbuf_end);
 				break;
 		}
 	}
@@ -292,6 +315,7 @@ PHP_FUNCTION(iptcembed)
 	fclose(fp);

 	if (spool < 2) {
+		*poi = '\0';
 		spoolbuf = zend_string_truncate(spoolbuf, poi - (unsigned char*)ZSTR_VAL(spoolbuf), 0);
 		RETURN_NEW_STR(spoolbuf);
 	} else {
--- a/ext/standard/tests/image/gh20582.phpt
+++ b/ext/standard/tests/image/gh20582.phpt
@@ -0,0 +1,52 @@
+--TEST--
+GH-20582 (Heap Buffer Overflow in iptcembed)
+--CREDITS--
+Nikita Sveshnikov (Positive Technologies)
+ndossche
+--SKIPIF--
+<?php
+if (PHP_OS_FAMILY === "Windows") die("skip Only for platforms with FIFO pipes");
+?>
+--FILE--
+<?php
+
+$pipe = __DIR__.'/gh20582.pipe.jpg';
+
+// Create named pipe (FIFO)
+if (!file_exists($pipe)) {
+    if (!posix_mkfifo($pipe, 0666)) {
+        die("Failed to create FIFO\n");
+    }
+}
+
+$descriptorspec = array(
+   0 => STDIN,
+   1 => STDOUT,
+   2 => STDOUT,
+);
+$pipes = [];
+$proc = proc_open([PHP_BINARY, '-n', '-r', "var_dump(iptcembed('A', '$pipe'));"], $descriptorspec, $pipes);
+
+// Blocks until a reader opens it
+$fp = fopen($pipe, 'wb') or die("Failed to open FIFO");
+
+// Write header
+$data  = "\xFF\xD8";                    // SOI marker
+$data .= "\xFF\xE0\x00\x10";            // APP0 marker (JFIF)
+$data .= "JFIF" . str_repeat("\x00", 9);
+$data .= "\xFF\xDA\x00\x08";            // SOS marker
+$data .= str_repeat("\x00", 6);
+fwrite($fp, $data);
+
+// Write garbage
+fwrite($fp, str_repeat("A", 5120));
+
+fclose($fp);
+
+?>
+--CLEAN--
+<?php
+@unlink(__DIR__.'/gh20582.pipe.jpg');
+?>
+--EXPECTF--
+string(1055) "ÿØÿà%0JFIF%0%0%0%0%0%0%0%0%0ÿÿí%0Photoshop 3.0%08BIM%0%0%0%0%0A%0Ú%0%0%0%0%0%0%0AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"