Heap Buffer Overflow in iptcembed #20582

Activity

1. 012git012

   added

   Bug

   Status: Needs Triage

   on Nov 25, 2025
2. ndossche

   self-assigned this

   on Nov 25, 2025
3. ndossche

   added

   Extension: standard

   Status: Verified

   and removed

   Status: Needs Triage

   on Nov 25, 2025
4. ndossche commented on Nov 25, 2025

   

   ndossche

   on Nov 25, 2025

   Last edited by ndossche

   Member

   More actions

   It's a form of TOC-TOU: the buffer is allocated based on the initial size. However, writing more data to the file (or pipe in this case) will make sure that getc() will not return EOF before going over the initial size.
   An extra bounds check is probably the right way to go.
5. ndossche commented on Nov 25, 2025

   

   ndossche

   on Nov 25, 2025

   Member

   More actions

   I propose this, but will need to port the test code to PHP:

   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 {
   
6. ndossche

   added a commit that references this issue on Nov 25, 2025

   Fix phpGH-20582: Heap Buffer Overflow in iptcembed

   ...

   This issue will close once commit e410e68 is merged into the 'master' branch.

   e410e68
7. ndossche

   linked a pull request that will close this issueFix GH-20582: Heap Buffer Overflow in iptcembed #20591on Nov 25, 2025
8. 012git012 commented on Dec 23, 2025

   

   012git012

   on Dec 23, 2025

   Author

   More actions

   Hello @ndossche
   Could you please let us know whether you plan to resolve this issue by the end of December? If not, we would appreciate it if you could provide an estimated timeline.
9. ndossche

   added a commit that references this issue on Dec 26, 2025

   Fix GH-20582: Heap Buffer Overflow in iptcembed

   ...

   This issue will close once commit 99ed66b is merged into the 'master' branch.

   99ed66b
10. ndossche

    closed this as completedin 99ed66bon Dec 26, 2025
11. ndossche

    added 3 commits that reference this issue on Dec 26, 2025

    Merge branch 'PHP-8.3' into PHP-8.4

    ...

    This issue will close once commit 48b4289 is merged into the 'master' branch.

    48b4289

    Merge branch 'PHP-8.4' into PHP-8.5

    ...

    This issue will close once commit 4ed8fce is merged into the 'master' branch.

    4ed8fce

    Merge branch 'PHP-8.5'

    ...

    This issue will close once commit 69166c3 is merged into the 'master' branch.

    69166c3
12. ndossche commented on Dec 26, 2025

    

    ndossche

    on Dec 26, 2025

    Member

    More actions

    @012git012 the fix is merged and will be part of the next release.