archive/php-src
1
0
Fork 0
mirror of https://github.com/php/php-src.git synced 2025-08-16 05:58:45 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Fix GH-17808: PharFileInfo refcount bug

Browse source 
PharFileInfo just takes a pointer from the manifest without refcounting
anything. If the entry is then removed from the manifest while the
PharFileInfo object still exists, we get a UAF.
We fix this by using the fp_refcount field. This is technically a
behaviour change as the unlinking is now blocked, and potentially file
modifications can be blocked as well. The alternative would be to have a
field that indicates whether deletion is blocked, but similar corruption
bugs may occur as well with file overwrites, so we increment fp_refcount
instead.
This also fixes an issue where a destructor called multiple times
resulted in a UAF as well, by moving the NULL'ing of the entry field out
of the if.

Closes GH-17811.
This commit is contained in:
Niels Dossche 2025-02-15 12:38:55 +01:00
parent 0f63bee3e9
commit e735d2bc3b
No known key found for this signature in database
GPG key ID: B8A8AD166DF0E2E5
3 changed files with 37 additions and 2 deletions
Download patch file Download diff file Expand all files Collapse all files

3
NEWS
View file

@ -37,6 +37,9 @@ PHP NEWS
JIT crash). (nielsdos)
. Fixed bug GH-17577 (JIT packed type guard crash). (nielsdos, Dmitry)
- Phar:
. Fixed bug GH-17808: PharFileInfo refcount bug. (nielsdos)
- PHPDBG:
. Partially fixed bug GH-17387 (Trivial crash in phpdbg lexer). (nielsdos)
. Fix memory leak in phpdbg calling registered function. (nielsdos)

15
ext/phar/phar_object.c
View file

@ -4483,6 +4483,9 @@ PHP_METHOD(PharFileInfo, __construct)
efree(entry);
entry_obj->entry = entry_info;
if (!entry_info->is_persistent && !entry_info->is_temp_dir) {
++entry_info->fp_refcount;
}
ZVAL_STRINGL(&arg1, fname, fname_len);
@ -4512,15 +4515,23 @@ PHP_METHOD(PharFileInfo, __destruct)
RETURN_THROWS();
}
if (entry_obj->entry && entry_obj->entry->is_temp_dir) {
if (!entry_obj->entry) {
return;
}
if (entry_obj->entry->is_temp_dir) {
if (entry_obj->entry->filename) {
efree(entry_obj->entry->filename);
entry_obj->entry->filename = NULL;
}
efree(entry_obj->entry);
entry_obj->entry = NULL;
} else if (!entry_obj->entry->is_persistent) {
--entry_obj->entry->fp_refcount;
/* It is necessarily still in the manifest, which will ultimately free this. */
}
entry_obj->entry = NULL;
}
/* }}} */

21
ext/phar/tests/gh17808.phpt Normal file
View file

@ -0,0 +1,21 @@
--TEST--
GH-17808 (PharFileInfo refcount bug)
--EXTENSIONS--
phar
--FILE--
<?php
$fname = __DIR__.'/tar/files/Structures_Graph-1.0.3.tgz';
$tar = new PharData($fname);
foreach (new RecursiveIteratorIterator($tar) as $file) {
}
var_dump("$file");
var_dump(strlen($file->getContent()));
unlink("$file");
var_dump($file->getATime());
?>
--EXPECTF--
string(%d) "phar://%spackage.xml"
int(6747)
Warning: unlink(): phar error: "package.xml" in phar %s, has open file pointers, cannot unlink in %s on line %d
int(33188)