From d5cebe289721083ae37fa47e649a8e77ed02f2b2 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.
---
 ext/phar/phar_object.c      | 15 +++++++++++++--
 ext/phar/tests/gh17808.phpt | 21 +++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)
 create mode 100644 ext/phar/tests/gh17808.phpt

diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c
index 39526ce4b235b..bd724b1036949 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 0000000000000..75a674b96efd9
--- /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)