Skip to content

Commit 3059ada

Browse files
GirgiasGirgias
authored
ext/standard: Improve checking of allowed_classes option (#15267)
* ext/standard: Add some unserializing tests * ext/standard: Add proper type checking for values of the allowed_classes option array * ext/standard: Check that class names are somewhat sensible for the allowed_classes option array * Indicate type of value * Add test for Stringable objects
1 parent efe0e73 commit 3059ada

5 files changed

+161
-11
lines changed

‎ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
--TEST--
2+
Test unserialize() with array allowed_classes and nonsensical values
3+
--FILE--
4+
<?php
5+
class foo {
6+
public $x = "bar";
7+
}
8+
$z = array(new foo(), 2, "3");
9+
$s = serialize($z);
10+
11+
try {
12+
unserialize($s, ["allowed_classes" => [null]]);
13+
} catch (Throwable $e) {
14+
echo $e::class, ': ', $e->getMessage(), "\n";
15+
}
16+
try {
17+
unserialize($s, ["allowed_classes" => [false]]);
18+
} catch (Throwable $e) {
19+
echo $e::class, ': ', $e->getMessage(), "\n";
20+
}
21+
try {
22+
unserialize($s, ["allowed_classes" => [true]]);
23+
} catch (Throwable $e) {
24+
echo $e::class, ': ', $e->getMessage(), "\n";
25+
}
26+
try {
27+
unserialize($s, ["allowed_classes" => [42]]);
28+
} catch (Throwable $e) {
29+
echo $e::class, ': ', $e->getMessage(), "\n";
30+
}
31+
try {
32+
unserialize($s, ["allowed_classes" => [15.2]]);
33+
} catch (Throwable $e) {
34+
echo $e::class, ': ', $e->getMessage(), "\n";
35+
}
36+
try {
37+
unserialize($s, ["allowed_classes" => [[]]]);
38+
} catch (Throwable $e) {
39+
echo $e::class, ': ', $e->getMessage(), "\n";
40+
}
41+
try {
42+
unserialize($s, ["allowed_classes" => [STDERR]]);
43+
} catch (Throwable $e) {
44+
echo $e::class, ': ', $e->getMessage(), "\n";
45+
}
46+
try {
47+
unserialize($s, ["allowed_classes" => [new stdClass]]);
48+
} catch (Throwable $e) {
49+
echo $e::class, ': ', $e->getMessage(), "\n";
50+
}
51+
52+
?>
53+
--EXPECT--
54+
TypeError: unserialize(): Option "allowed_classes" must be an array of class names, null given
55+
TypeError: unserialize(): Option "allowed_classes" must be an array of class names, false given
56+
TypeError: unserialize(): Option "allowed_classes" must be an array of class names, true given
57+
TypeError: unserialize(): Option "allowed_classes" must be an array of class names, int given
58+
TypeError: unserialize(): Option "allowed_classes" must be an array of class names, float given
59+
TypeError: unserialize(): Option "allowed_classes" must be an array of class names, array given
60+
TypeError: unserialize(): Option "allowed_classes" must be an array of class names, resource given
61+
Error: Object of class stdClass could not be converted to string

‎ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_class_names.phpt

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
--TEST--
2+
Test unserialize() with array allowed_classes and nonsensical class names
3+
--FILE--
4+
<?php
5+
class foo {
6+
public $x = "bar";
7+
}
8+
$z = array(new foo(), 2, "3");
9+
$s = serialize($z);
10+
11+
try {
12+
unserialize($s, ["allowed_classes" => [""]]);
13+
} catch (Throwable $e) {
14+
echo $e::class, ': ', $e->getMessage(), "\n";
15+
}
16+
try {
17+
unserialize($s, ["allowed_classes" => ["245blerg"]]);
18+
} catch (Throwable $e) {
19+
echo $e::class, ': ', $e->getMessage(), "\n";
20+
}
21+
try {
22+
unserialize($s, ["allowed_classes" => [" whitespace "]]);
23+
} catch (Throwable $e) {
24+
echo $e::class, ': ', $e->getMessage(), "\n";
25+
}
26+
try {
27+
unserialize($s, ["allowed_classes" => ["name\nwith whitespace"]]);
28+
} catch (Throwable $e) {
29+
echo $e::class, ': ', $e->getMessage(), "\n";
30+
}
31+
try {
32+
unserialize($s, ["allowed_classes" => ['$dollars']]);
33+
} catch (Throwable $e) {
34+
echo $e::class, ': ', $e->getMessage(), "\n";
35+
}
36+
try {
37+
unserialize($s, ["allowed_classes" => ["have\0nul_byte"]]);
38+
} catch (Throwable $e) {
39+
echo $e::class, ': ', $e->getMessage(), "\n";
40+
}
41+
42+
?>
43+
--EXPECT--
44+
ValueError: unserialize(): Option "allowed_classes" must be an array of class names, " whitespace " given
45+
ValueError: unserialize(): Option "allowed_classes" must be an array of class names, "name
46+
with whitespace" given
47+
ValueError: unserialize(): Option "allowed_classes" must be an array of class names, "$dollars" given
48+
ValueError: unserialize(): Option "allowed_classes" must be an array of class names, "have" given

‎ext/standard/tests/serialize/unserialize_error_001.phpt renamed to ‎ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_value.phpt

File renamed without changes.

‎ext/standard/tests/serialize/unserialize_allowed_classes_option_stringable_value.phpt

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
--TEST--
2+
Test unserialize() with Stringable object in allowed_classes
3+
--FILE--
4+
<?php
5+
class foo {
6+
public $x = "bar";
7+
}
8+
$z = array(new foo(), 2, "3");
9+
$s = serialize($z);
10+
11+
class Name {
12+
public function __toString(): string {
13+
return 'Foo';
14+
}
15+
}
16+
17+
$o = new Name();
18+
19+
var_dump(unserialize($s, ["allowed_classes" => [$o]]));
20+
?>
21+
--EXPECT--
22+
array(3) {
23+
[0]=>
24+
object(foo)#3 (1) {
25+
["x"]=>
26+
string(3) "bar"
27+
}
28+
[1]=>
29+
int(2)
30+
[2]=>
31+
string(1) "3"
32+
}

‎ext/standard/var.c

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1370,25 +1370,34 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co
13701370
goto cleanup;
13711371
}
13721372

1373-
if(classes && (Z_TYPE_P(classes) == IS_ARRAY || !zend_is_true(classes))) {
1373+
if (classes && (Z_TYPE_P(classes) == IS_ARRAY || !zend_is_true(classes))) {
13741374
ALLOC_HASHTABLE(class_hash);
13751375
zend_hash_init(class_hash, (Z_TYPE_P(classes) == IS_ARRAY)?zend_hash_num_elements(Z_ARRVAL_P(classes)):0, NULL, NULL, 0);
13761376
}
1377-
if(class_hash && Z_TYPE_P(classes) == IS_ARRAY) {
1377+
if (class_hash && Z_TYPE_P(classes) == IS_ARRAY) {
13781378
zval *entry;
1379-
zend_string *lcname;
13801379

13811380
ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(classes), entry) {
1382-
convert_to_string(entry);
1383-
lcname = zend_string_tolower(Z_STR_P(entry));
1381+
ZVAL_DEREF(entry);
1382+
if (UNEXPECTED(Z_TYPE_P(entry) != IS_STRING && Z_TYPE_P(entry) != IS_OBJECT)) {
1383+
zend_type_error("%s(): Option \"allowed_classes\" must be an array of class names, %s given",
1384+
function_name, zend_zval_value_name(entry));
1385+
goto cleanup;
1386+
}
1387+
zend_string *name = zval_try_get_string(entry);
1388+
if (UNEXPECTED(name == NULL)) {
1389+
goto cleanup;
1390+
}
1391+
if (UNEXPECTED(!zend_is_valid_class_name(name))) {
1392+
zend_value_error("%s(): Option \"allowed_classes\" must be an array of class names, \"%s\" given", function_name, ZSTR_VAL(name));
1393+
zend_string_release_ex(name, false);
1394+
goto cleanup;
1395+
}
1396+
zend_string *lcname = zend_string_tolower(name);
13841397
zend_hash_add_empty_element(class_hash, lcname);
1385-
zend_string_release_ex(lcname, 0);
1398+
zend_string_release_ex(name, false);
1399+
zend_string_release_ex(lcname, false);
13861400
} ZEND_HASH_FOREACH_END();
1387-
1388-
/* Exception during string conversion. */
1389-
if (EG(exception)) {
1390-
goto cleanup;
1391-
}
13921401
}
13931402
php_var_unserialize_set_allowed_classes(var_hash, class_hash);
13941403

0 commit comments

Comments
 (0)