From e410e68aaa0d5d737cb06bc41755dd5ae87609b6 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+ndossche@users.noreply.github.com> Date: Tue, 25 Nov 2025 22:58:09 +0100 Subject: [PATCH] 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. --- ext/standard/iptc.c | 80 +++++++++++++++++---------- ext/standard/tests/image/gh20582.phpt | 52 +++++++++++++++++ 2 files changed, 104 insertions(+), 28 deletions(-) create mode 100644 ext/standard/tests/image/gh20582.phpt diff --git a/ext/standard/iptc.c b/ext/standard/iptc.c index 44dd33bab10ac..0f46ecd19bc7c 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 0000000000000..63561534b2fd0 --- /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-- + +--FILE-- + 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-- + +--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"