Skip to content

Commit cb3da7d

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

10 files changed

+210
-0
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: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3214,6 +3214,9 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
32143214
zval *entry; /* Hash entry */
32153215
uint32_t iter_pos = zend_hash_iterators_lower_pos(in_hash, 0);
32163216

3217+
GC_ADDREF(in_hash);
3218+
HT_ALLOW_COW_VIOLATION(in_hash); /* Will be reset when setting the flags for in_hash */
3219+
32173220
/* Get number of entries in the input hash */
32183221
num_in = zend_hash_num_elements(in_hash);
32193222

@@ -3372,6 +3375,15 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
33723375
HT_SET_ITERATORS_COUNT(&out_hash, HT_ITERATORS_COUNT(in_hash));
33733376
HT_SET_ITERATORS_COUNT(in_hash, 0);
33743377
in_hash->pDestructor = NULL;
3378+
3379+
if (UNEXPECTED(GC_DELREF(in_hash) == 0)) {
3380+
/* Array was completely deallocated during the operation */
3381+
zend_array_destroy(in_hash);
3382+
zend_hash_destroy(&out_hash);
3383+
zend_throw_error(NULL, "Array was modified during array_splice operation");
3384+
return;
3385+
}
3386+
33753387
zend_hash_destroy(in_hash);
33763388

33773389
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 modified 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 modified 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 modified 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 modified 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 modified 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 modified 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 modified during array_splice operation

0 commit comments

Comments
 (0)