From e735d2bc3b41c40cea45fdaefabd3cd48f100e0a Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Sat, 15 Feb 2025 12:38:55 +0100
Subject: [PATCH] Fix GH-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 GH-17811.
---
 NEWS                        |  3 +++
 ext/phar/phar_object.c      | 15 +++++++++++++--
 ext/phar/tests/gh17808.phpt | 21 +++++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)
 create mode 100644 ext/phar/tests/gh17808.phpt

diff --git a/NEWS b/NEWS
index 42a0ec193d9..c71825e7304 100644
--- a/NEWS
+++ b/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)
diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c
index 39526ce4b23..bd724b10369 100644
--- a/ext/phar/phar_object.c
+++ b/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;
 }
 /* }}} */
 
diff --git a/ext/phar/tests/gh17808.phpt b/ext/phar/tests/gh17808.phpt
new file mode 100644
index 00000000000..28cb9a2e227
--- /dev/null
+++ b/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)