Fix phpGH-17808: PharFileInfo refcount bug

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 phpGH-17811.

Author: ndossche

NEWS

@@ -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)

ext/phar/phar_object.c

@@ -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;
 }
 /* }}} */
 

ext/phar/tests/gh17808.phpt

@@ -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)