Skip to content

Commit a6bff41

Browse files
alexandre-dauboisalexandre-daubois
committed
Fix GH-16649: Avoid UAF when using array_splice
1 parent bd2766c commit a6bff41

10 files changed

+227
-5
lines changed

‎NEWS‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ PHP NEWS
7373
. Fix theoretical issues with hrtime() not being available. (nielsdos)
7474
. Fixed bug GH-19300 (Nested array_multisort invocation with error breaks).
7575
(nielsdos)
76+
. Fixed bug GH-16649 (UAF during array_splice). (alexandre-daubois)
7677

7778
- Windows:
7879
. Free opened_path when opened_path_len >= MAXPATHLEN. (dixyes)

‎ext/standard/array.c‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3213,6 +3213,11 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
32133213
uint32_t idx;
32143214
zval *entry; /* Hash entry */
32153215
uint32_t iter_pos = zend_hash_iterators_lower_pos(in_hash, 0);
3216+
void *original_arData; /* Store original arData to detect CoW */
3217+
3218+
GC_ADDREF(in_hash);
3219+
HT_ALLOW_COW_VIOLATION(in_hash); /* Will be reset when setting the flags for in_hash */
3220+
original_arData = in_hash->arData;
32163221

32173222
/* Get number of entries in the input hash */
32183223
num_in = zend_hash_num_elements(in_hash);
@@ -3252,7 +3257,8 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
32523257

32533258
/* If hash for removed entries exists, go until offset+length and copy the entries to it */
32543259
if (removed != NULL) {
3255-
for ( ; pos - offset < length && idx < in_hash->nNumUsed; idx++, entry++) {
3260+
for ( ; pos - offset < length && idx < in_hash->nNumUsed; idx++) {
3261+
entry = in_hash->arPacked + idx;
32563262
if (Z_TYPE_P(entry) == IS_UNDEF) continue;
32573263
pos++;
32583264
Z_TRY_ADDREF_P(entry);
@@ -3262,7 +3268,8 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
32623268
} else { /* otherwise just skip those entries */
32633269
zend_long pos2 = pos;
32643270

3265-
for ( ; pos2 - offset < length && idx < in_hash->nNumUsed; idx++, entry++) {
3271+
for ( ; pos2 - offset < length && idx < in_hash->nNumUsed; idx++) {
3272+
entry = in_hash->arPacked + idx;
32663273
if (Z_TYPE_P(entry) == IS_UNDEF) continue;
32673274
pos2++;
32683275
zend_hash_packed_del_val(in_hash, entry);
@@ -3317,7 +3324,8 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
33173324

33183325
/* If hash for removed entries exists, go until offset+length and copy the entries to it */
33193326
if (removed != NULL) {
3320-
for ( ; pos - offset < length && idx < in_hash->nNumUsed; idx++, p++) {
3327+
for ( ; pos - offset < length && idx < in_hash->nNumUsed; idx++) {
3328+
p = in_hash->arData + idx;
33213329
if (Z_TYPE(p->val) == IS_UNDEF) continue;
33223330
pos++;
33233331
entry = &p->val;
@@ -3332,7 +3340,8 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
33323340
} else { /* otherwise just skip those entries */
33333341
zend_long pos2 = pos;
33343342

3335-
for ( ; pos2 - offset < length && idx < in_hash->nNumUsed; idx++, p++) {
3343+
for ( ; pos2 - offset < length && idx < in_hash->nNumUsed; idx++) {
3344+
p = in_hash->arData + idx;
33363345
if (Z_TYPE(p->val) == IS_UNDEF) continue;
33373346
pos2++;
33383347
zend_hash_del_bucket(in_hash, p);
@@ -3350,7 +3359,8 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
33503359
}
33513360

33523361
/* Copy the remaining input hash entries to the output hash */
3353-
for ( ; idx < in_hash->nNumUsed ; idx++, p++) {
3362+
for ( ; idx < in_hash->nNumUsed ; idx++) {
3363+
p = in_hash->arData + idx;
33543364
if (Z_TYPE(p->val) == IS_UNDEF) continue;
33553365
entry = &p->val;
33563366
if (p->key == NULL) {
@@ -3372,6 +3382,20 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
33723382
HT_SET_ITERATORS_COUNT(&out_hash, HT_ITERATORS_COUNT(in_hash));
33733383
HT_SET_ITERATORS_COUNT(in_hash, 0);
33743384
in_hash->pDestructor = NULL;
3385+
3386+
if (UNEXPECTED(GC_DELREF(in_hash) == 0)) {
3387+
/* Array was completely deallocated by a destructor */
3388+
zend_array_destroy(in_hash);
3389+
zend_hash_destroy(&out_hash);
3390+
zend_throw_error(NULL, "Array was deallocated by destructors during array_splice operation");
3391+
return;
3392+
} else if (UNEXPECTED(in_hash->arData != original_arData)) {
3393+
/* Array data changed */
3394+
zend_hash_destroy(&out_hash);
3395+
zend_throw_error(NULL, "Array was modified by destructors during array_splice operation");
3396+
return;
3397+
}
3398+
33753399
zend_hash_destroy(in_hash);
33763400

33773401
HT_FLAGS(in_hash) = HT_FLAGS(&out_hash);

‎ext/standard/tests/array/gh16649/array_splice_normal_destructor.phpt‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
--TEST--
2+
GH-16649: array_splice with normal destructor should work fine
3+
--FILE--
4+
<?php
5+
class C {
6+
function __destruct() {
7+
echo "Destructor called\n";
8+
}
9+
}
10+
11+
$arr = ["1", new C, "2"];
12+
array_splice($arr, 1, 2);
13+
var_dump($arr);
14+
?>
15+
--EXPECT--
16+
Destructor called
17+
array(1) {
18+
[0]=>
19+
string(1) "1"
20+
}

‎ext/standard/tests/array/gh16649/array_splice_uaf_add_elements.phpt‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
--TEST--
2+
GH-16649: array_splice UAF when destructor adds elements to array
3+
--FILE--
4+
<?php
5+
class C {
6+
function __destruct() {
7+
global $arr;
8+
$arr[] = 0;
9+
}
10+
}
11+
12+
$arr = ["1", new C, "2"];
13+
14+
try {
15+
array_splice($arr, 1, 2);
16+
echo "ERROR: Should have thrown exception\n";
17+
} catch (Error $e) {
18+
echo "Exception caught: " . $e->getMessage() . "\n";
19+
}
20+
?>
21+
--EXPECT--
22+
Exception caught: Array was deallocated by destructors during array_splice operation

‎ext/standard/tests/array/gh16649/array_splice_uaf_array_deallocated.phpt‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
--TEST--
2+
GH-16649: array_splice UAF when array is released entirely
3+
--FILE--
4+
<?php
5+
class C {
6+
function __destruct() {
7+
global $arr;
8+
$arr = null;
9+
}
10+
}
11+
12+
$arr = ["1", new C, "2"];
13+
14+
try {
15+
array_splice($arr, 1, 2);
16+
echo "ERROR: Should have thrown exception\n";
17+
} catch (Error $e) {
18+
echo "Exception caught: " . $e->getMessage() . "\n";
19+
}
20+
?>
21+
--EXPECT--
22+
Exception caught: Array was deallocated by destructors during array_splice operation

‎ext/standard/tests/array/gh16649/array_splice_uaf_complex_modification.phpt‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
--TEST--
2+
GH-16649: array_splice UAF with complex array modification
3+
--FILE--
4+
<?php
5+
class ComplexModifier {
6+
function __destruct() {
7+
global $arr;
8+
// complex modification that causes cow
9+
unset($arr[0]);
10+
$arr["new_key"] = "new_value";
11+
$arr[100] = "another_value";
12+
}
13+
}
14+
15+
$arr = ["first", new ComplexModifier, "last"];
16+
17+
try {
18+
array_splice($arr, 1, 1, ["replacement"]);
19+
echo "ERROR: Should have thrown exception\n";
20+
} catch (Error $e) {
21+
echo "Exception caught: " . $e->getMessage() . "\n";
22+
}
23+
?>
24+
--EXPECT--
25+
Exception caught: Array was deallocated by destructors during array_splice operation

‎ext/standard/tests/array/gh16649/array_splice_uaf_multiple_destructors.phpt‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
--TEST--
2+
GH-16649: array_splice UAF with multiple destructors
3+
--FILE--
4+
<?php
5+
class MultiDestructor {
6+
public $id;
7+
8+
function __construct($id) {
9+
$this->id = $id;
10+
}
11+
12+
function __destruct() {
13+
global $arr;
14+
echo "Destructor {$this->id} called\n";
15+
if ($this->id == 2) {
16+
$arr = null;
17+
}
18+
}
19+
}
20+
21+
$arr = ["start", new MultiDestructor(1), new MultiDestructor(2), "end"];
22+
23+
try {
24+
array_splice($arr, 1, 2);
25+
echo "ERROR: Should have thrown exception\n";
26+
} catch (Error $e) {
27+
echo "Exception caught: " . $e->getMessage() . "\n";
28+
}
29+
?>
30+
--EXPECT--
31+
Destructor 1 called
32+
Destructor 2 called
33+
Exception caught: Array was deallocated by destructors during array_splice operation

‎ext/standard/tests/array/gh16649/array_splice_uaf_original_case.phpt‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
--TEST--
2+
GH-16649: array_splice UAF with destructor modifying array (original case)
3+
--FILE--
4+
<?php
5+
function resize_arr() {
6+
global $arr;
7+
for ($i = 0; $i < 10; $i++) {
8+
$arr[$i] = $i;
9+
}
10+
}
11+
12+
class C {
13+
function __destruct() {
14+
resize_arr();
15+
return "3";
16+
}
17+
}
18+
19+
$arr = ["a" => "1", "3" => new C, "2" => "2"];
20+
21+
try {
22+
array_splice($arr, 1, 2);
23+
echo "ERROR: Should have thrown exception\n";
24+
} catch (Error $e) {
25+
echo "Exception caught: " . $e->getMessage() . "\n";
26+
}
27+
?>
28+
--EXPECT--
29+
Exception caught: Array was deallocated by destructors during array_splice operation

‎ext/standard/tests/array/gh16649/array_splice_uaf_packed_to_hash.phpt‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
--TEST--
2+
GH-16649: array_splice UAF when array is converted from packed to hash
3+
--FILE--
4+
<?php
5+
class C {
6+
function __destruct() {
7+
global $arr;
8+
// array is converted from packed to hash
9+
$arr["str"] = 0;
10+
}
11+
}
12+
13+
$arr = ["1", new C, "2"];
14+
15+
try {
16+
array_splice($arr, 1, 2);
17+
echo "ERROR: Should have thrown exception\n";
18+
} catch (Error $e) {
19+
echo "Exception caught: " . $e->getMessage() . "\n";
20+
}
21+
?>
22+
--EXPECT--
23+
Exception caught: Array was deallocated by destructors during array_splice operation

‎ext/standard/tests/array/gh16649/array_splice_with_replacement.phpt‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
--TEST--
2+
GH-16649: array_splice with replacement array when destructor modifies array
3+
--FILE--
4+
<?php
5+
class C {
6+
function __destruct() {
7+
global $arr;
8+
$arr["modified"] = "by_destructor";
9+
}
10+
}
11+
12+
$arr = ["a", new C, "b"];
13+
$replacement = ["replacement1", "replacement2"];
14+
15+
try {
16+
array_splice($arr, 1, 1, $replacement);
17+
echo "ERROR: Should have thrown exception\n";
18+
} catch (Error $e) {
19+
echo "Exception caught: " . $e->getMessage() . "\n";
20+
}
21+
?>
22+
--EXPECT--
23+
Exception caught: Array was deallocated by destructors during array_splice operation

0 commit comments

Comments
 (0)