From 934438ed545ba8c6d42c4048a3396acd906d4b93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Wed, 28 Sep 2022 18:12:24 +0200 Subject: [PATCH 1/3] Unserialize: Warn if extra data is appended to the serialized string --- .../typed_property_ref_overwrite.phpt | 2 +- .../serialize/unserialize_extra_data.phpt | 26 +++++++++++++++++++ ext/standard/var.c | 5 ++++ 3 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 ext/standard/tests/serialize/unserialize_extra_data.phpt diff --git a/ext/standard/tests/serialize/typed_property_ref_overwrite.phpt b/ext/standard/tests/serialize/typed_property_ref_overwrite.phpt index 148c66b76c741..9b68e50783795 100644 --- a/ext/standard/tests/serialize/typed_property_ref_overwrite.phpt +++ b/ext/standard/tests/serialize/typed_property_ref_overwrite.phpt @@ -7,7 +7,7 @@ class Test { public ?object $prop; } $s = <<<'STR' -O:4:"Test":2:{s:4:"prop";R:1;s:4:"prop";N;}} +O:4:"Test":2:{s:4:"prop";R:1;s:4:"prop";N;} STR; var_dump(unserialize($s)); diff --git a/ext/standard/tests/serialize/unserialize_extra_data.phpt b/ext/standard/tests/serialize/unserialize_extra_data.phpt new file mode 100644 index 0000000000000..02f49b3477b02 --- /dev/null +++ b/ext/standard/tests/serialize/unserialize_extra_data.phpt @@ -0,0 +1,26 @@ +--TEST-- +Extra data at the end of a valid value +--FILE-- + +--EXPECTF-- +Warning: unserialize(): Extra data starting at offset 4 of 8 bytes in %s on line %d +int(5) + +Warning: unserialize(): Extra data starting at offset 2 of 6 bytes in %s on line %d +NULL + +Warning: unserialize(): Extra data starting at offset 4 of 8 bytes in %s on line %d +bool(true) + +Warning: unserialize(): Extra data starting at offset 20 of 24 bytes in %s on line %d +array(1) { + ["foo"]=> + bool(true) +} diff --git a/ext/standard/var.c b/ext/standard/var.c index e8145ab227141..7d32aa8b579be 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -1406,6 +1406,11 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co zval_ptr_dtor(return_value); } RETVAL_FALSE; + } else if ((char*)p < buf + buf_len) { + if (!EG(exception)) { + php_error_docref(NULL, E_WARNING, "Extra data starting at offset " ZEND_LONG_FMT " of %zd bytes", + (zend_long)((char*)p - buf), buf_len); + } } else if (BG(unserialize).level > 1) { ZVAL_COPY(return_value, retval); } else if (Z_REFCOUNTED_P(return_value)) { From 2b7489b0dc644f6525980b94b61e13146d574b92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Sat, 8 Oct 2022 13:22:38 +0200 Subject: [PATCH 2/3] Fix extra data detection for nested unserialize in Serializable interface --- ...a.phpt => unserialize_extra_data_001.phpt} | 2 +- .../serialize/unserialize_extra_data_002.phpt | 42 +++++++++++++++++++ .../serialize/unserialize_extra_data_003.phpt | 42 +++++++++++++++++++ ext/standard/var.c | 21 ++++++---- 4 files changed, 97 insertions(+), 10 deletions(-) rename ext/standard/tests/serialize/{unserialize_extra_data.phpt => unserialize_extra_data_001.phpt} (90%) create mode 100644 ext/standard/tests/serialize/unserialize_extra_data_002.phpt create mode 100644 ext/standard/tests/serialize/unserialize_extra_data_003.phpt diff --git a/ext/standard/tests/serialize/unserialize_extra_data.phpt b/ext/standard/tests/serialize/unserialize_extra_data_001.phpt similarity index 90% rename from ext/standard/tests/serialize/unserialize_extra_data.phpt rename to ext/standard/tests/serialize/unserialize_extra_data_001.phpt index 02f49b3477b02..421fb3428b3a7 100644 --- a/ext/standard/tests/serialize/unserialize_extra_data.phpt +++ b/ext/standard/tests/serialize/unserialize_extra_data_001.phpt @@ -1,5 +1,5 @@ --TEST-- -Extra data at the end of a valid value +Test unserialize() with extra data at the end of a valid value --FILE-- foo = unserialize($foo['bar']); + } + + public function __serialize(): array + { + return [ + 'bar' => serialize($this->foo) . 'garbage', + ]; + } +} + +$f = new Foo; +$f->foo = ['a', 'b', 'c']; + +var_dump(unserialize(serialize($f) . 'garbage')); + +?> +--EXPECTF-- +Warning: unserialize(): Extra data starting at offset 81 of 88 bytes in %s on line %d + +Warning: unserialize(): Extra data starting at offset 42 of 49 bytes in %s on line %d +object(Foo)#2 (1) { + ["foo"]=> + array(3) { + [0]=> + string(1) "a" + [1]=> + string(1) "b" + [2]=> + string(1) "c" + } +} diff --git a/ext/standard/tests/serialize/unserialize_extra_data_003.phpt b/ext/standard/tests/serialize/unserialize_extra_data_003.phpt new file mode 100644 index 0000000000000..93f2c49a56a0c --- /dev/null +++ b/ext/standard/tests/serialize/unserialize_extra_data_003.phpt @@ -0,0 +1,42 @@ +--TEST-- +Test unserialize() with extra data at the end of a valid value with Serializable +--FILE-- +foo = unserialize($foo); + } + + public function serialize(): string + { + return serialize($this->foo) . 'garbage'; + } +} + +$f = new Foo; +$f->foo = ['a', 'b', 'c']; + +var_dump(unserialize(serialize($f) . 'garbage')); + +?> +--EXPECTF-- +Deprecated: Foo implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary) in %s on line %d + +Warning: unserialize(): Extra data starting at offset 42 of 49 bytes in %s on line %d + +Warning: unserialize(): Extra data starting at offset 64 of 71 bytes in %s on line %d +object(Foo)#2 (1) { + ["foo"]=> + array(3) { + [0]=> + string(1) "a" + [1]=> + string(1) "b" + [2]=> + string(1) "c" + } +} diff --git a/ext/standard/var.c b/ext/standard/var.c index 7d32aa8b579be..784903616724a 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -1406,16 +1406,19 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co zval_ptr_dtor(return_value); } RETVAL_FALSE; - } else if ((char*)p < buf + buf_len) { - if (!EG(exception)) { - php_error_docref(NULL, E_WARNING, "Extra data starting at offset " ZEND_LONG_FMT " of %zd bytes", - (zend_long)((char*)p - buf), buf_len); + } else { + if ((char*)p < buf + buf_len) { + if (!EG(exception)) { + php_error_docref(NULL, E_WARNING, "Extra data starting at offset " ZEND_LONG_FMT " of %zd bytes", + (zend_long)((char*)p - buf), buf_len); + } + } + if (BG(unserialize).level > 1) { + ZVAL_COPY(return_value, retval); + } else if (Z_REFCOUNTED_P(return_value)) { + zend_refcounted *ref = Z_COUNTED_P(return_value); + gc_check_possible_root(ref); } - } else if (BG(unserialize).level > 1) { - ZVAL_COPY(return_value, retval); - } else if (Z_REFCOUNTED_P(return_value)) { - zend_refcounted *ref = Z_COUNTED_P(return_value); - gc_check_possible_root(ref); } cleanup: From d859cae4660f1db725c8b2acbbdaa1b6e4690691 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Mon, 1 May 2023 19:05:58 +0200 Subject: [PATCH 3/3] [ci skip] NEWS --- NEWS | 2 ++ UPGRADING | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index a988b783f6ee2..8b74b47abbee9 100644 --- a/NEWS +++ b/NEWS @@ -124,6 +124,8 @@ PHP NEWS - Standard: . E_NOTICEs emitted by unserialize() have been promoted to E_WARNING. (timwolla) + . unserialize() now emits a new E_WARNING if the input contains unconsumed + bytes. (timwolla) . Make array_pad's $length warning less confusing. (nielsdos) . E_WARNING emitted by strtok in the caase both arguments are not provided when starting tokenisation. (David Carlier) diff --git a/UPGRADING b/UPGRADING index cdada9e9b3273..4cbf5133b8c01 100644 --- a/UPGRADING +++ b/UPGRADING @@ -87,8 +87,11 @@ PHP 8.3 UPGRADE NOTES (Alex Dowad) - Standard: - . E_NOTICEs emitted by unserialized() have been promoted to E_WARNING. + . E_NOTICEs emitted by unserialize() have been promoted to E_WARNING. RFC: https://wiki.php.net/rfc/improve_unserialize_error_handling + . unserialize() now emits a new E_WARNING if the input contains unconsumed + bytes. + RFC: https://wiki.php.net/rfc/unserialize_warn_on_trailing_data . array_pad() is now only limited by the maximum number of elements an array can have. Before, it was only possible to add at most 1048576 elements at a time.