Commit 99ed66b49fa for php.net
commit 99ed66b49fa59fcb5a7a4f2b460d4155027d44d0
Author: Niels Dossche <7771979+ndossche@users.noreply.github.com>
Date: Tue Nov 25 22:58:09 2025 +0100
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.
diff --git a/NEWS b/NEWS
index a2690fa84d3..77119e29fcf 100644
--- a/NEWS
+++ b/NEWS
@@ -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
diff --git a/ext/standard/iptc.c b/ext/standard/iptc.c
index 44dd33bab10..0f46ecd19bc 100644
--- 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 {
diff --git a/ext/standard/tests/image/gh20582.phpt b/ext/standard/tests/image/gh20582.phpt
new file mode 100644
index 00000000000..63561534b2f
--- /dev/null
+++ 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--