Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GH-17442: Engine UAF with reference assign and dtor #17443

Closed
wants to merge 3 commits into from

Conversation

nielsdos
Copy link
Member

Need to protect the object zval in this case by using the garbage zval trick.
I don't really like doing this, but at least the code in the test isn't completely pathological and the assign_ref operations aren't super common on hot code I would think; so I can live a bit with that.

@nielsdos nielsdos linked an issue Jan 11, 2025 that may be closed by this pull request
@nielsdos nielsdos marked this pull request as ready for review January 11, 2025 11:26
@nielsdos nielsdos requested a review from dstogov as a code owner January 11, 2025 11:26
Zend/zend_API.h Outdated
Comment on lines 1103 to 1106
zval_ptr_dtor(_zv); \
zval garbage; \
ZVAL_COPY_VALUE(&garbage, _zv); \
ZVAL_NULL(_zv); \
zval_ptr_dtor(&garbage); \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, you should use "safe" zval destructor (or safe assign).
Safe arguments destructors were removed in PHP-8.0 (see 2b279b4).

Probably it makes sense to introduce zval_ptr_dtor_safe() in master to reduce code bloat.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this part of the macro, we can indeed use either the safe destructor code, or safe assign code.
I found that zend_try_assign_typed_ref_ex has the same problem, see following reproducer (which is a variant of the attached phpt test):

<?php
$map = new WeakMap;

class Test {
	public stdClass|string $obj;
}

$test = new Test;
$test->obj = new stdClass;

$map[$test->obj] = new class {
    function __destruct() {
		global $test;
		var_dump($test->obj);
        throw new Exception("Test");
    }
};

headers_sent($test->obj);

We can't use the safe destructor approach here, because it would set $test->obj to NULL and that is observable in the var_dump($test->obj); line in the example code. This is not allowed because that violates the type of the typed property. To solve this case we can use a safe assign like so:

diff --git a/Zend/zend_API.c b/Zend/zend_API.c
index 13c640a64dd..4879b2744eb 100644
--- a/Zend/zend_API.c
+++ b/Zend/zend_API.c
@@ -4461,8 +4461,17 @@ ZEND_API zend_result zend_try_assign_typed_ref_ex(zend_reference *ref, zval *val
 		zval_ptr_dtor(val);
 		return FAILURE;
 	} else {
-		zval_ptr_dtor(&ref->val);
-		ZVAL_COPY_VALUE(&ref->val, val);
+		if (Z_REFCOUNTED_P(&ref->val)) {
+			zend_refcounted *rc = Z_COUNTED_P(&ref->val);
+			ZVAL_COPY_VALUE(&ref->val, val);
+			if (!GC_DELREF(rc)) {
+				rc_dtor_func(rc);
+			} else {
+				gc_check_possible_root(rc);
+			}
+		} else {
+			ZVAL_COPY_VALUE(&ref->val, val);
+		}
 		return SUCCESS;
 	}
 }

The safe destructor zval_ptr_dtor_safe can easily be introduced via a new function. The safe assign can also be done via a function but that may have overhead in comparison to the old code. Perhaps a safe assign function should be a macro or always-inline function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safe assign looks good.

@nielsdos nielsdos changed the base branch from PHP-8.3 to master January 26, 2025 16:51
@nielsdos nielsdos force-pushed the fix-17442 branch 2 times, most recently from 6852055 to 65ae09b Compare January 26, 2025 17:31
Zend/zend_API.h Outdated
@@ -1097,6 +1097,22 @@ ZEND_API zend_result zend_try_assign_typed_ref_res(zend_reference *ref, zend_res
ZEND_API zend_result zend_try_assign_typed_ref_zval(zend_reference *ref, zval *zv);
ZEND_API zend_result zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval *zv, bool strict);

#define ZEND_SAFE_ASSIGN_VALUE(zv, value) do { \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have very similar zend_assign_to_variable() in zend_execute.h.
This one does almost the same except for reference support.
I think this sholud be named somehow more consistently and may be moved to zend_execute.h

The rest looks fine.

if (!GC_DELREF(ref)) {
rc_dtor_func(ref);
} else {
gc_check_possible_root(ref);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use gc_check_possible_root_no_ref() or GC_DTOR_NO_REF()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I don't know why I forgot this.

@nielsdos nielsdos closed this in b068c2f Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Engine UAF with reference assign and dtor
2 participants