Skip to content

random: Validate that the arrays do not contain extra elements when unserializing #9458

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 5, 2022
Merged

random: Validate that the arrays do not contain extra elements when unserializing #9458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd098de
Apply `var_dump()` in 02_engine/all_serialize_error.phpt
TimWolla Aug 31, 2022
28fa3f0
random: Validate that the arrays do not contain extra elements when u…
TimWolla Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ext/random/engine_mt19937.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,11 @@ static bool unserialize(php_random_status *status, HashTable *data)
php_random_status_state_mt19937 *s = status->state;
zval *t;

/* Verify the expected number of elements, this implicitly ensures that no additional elements are present. */
if (zend_hash_num_elements(data) != (MT_N + 2)) {
Copy link
Member

Choose a reason for hiding this comment

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

A lenient validation logic was a life-saver in case of SplFixedArray, otherwise my changes would have caused breaking changes between PHP versions. If ext/random engines will have new state information in a future PHP version, then unserializing these classes will be unnecessary in a platform-agnostic way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, but on the contrary I believe that strict validation makes this kind of upgrade easier:

  1. I strongly doubt that the engines are ever going to change, as they are “tied” to some external specification. Mt19937 is Mt19937, the state is well-defined.
  2. If some additional fields are in fact necessary in a future version, then I can be fairly sure that any existing serialized data will not yet include those fields with garbage values, because it was rejected previously.
  3. Serialization compatibility would not really exist even when ignoring unknown fields: Older PHP versions are not able to make sense of those out of the box, thus possible interpreting the unserialized instance differently, introducing a behavioral change. This is especially important for the random engiens whose whole selling point is generating repeatable sequences

Instead I would treat incompatible serialization output between PHP x.y and PHP x.(y+1) as a regular bug. The solution would then be adding a patch to the older PHP version to accept and ignore the newly added fields, but only once it is clear what the semantics of the newly added fields are!

return false;
}

for (uint32_t i = 0; i < MT_N; i++) {
t = zend_hash_index_find(data, i);
if (!t || Z_TYPE_P(t) != IS_STRING || Z_STRLEN_P(t) != (2 * sizeof(uint32_t))) {
Expand Down Expand Up @@ -358,6 +363,12 @@ PHP_METHOD(Random_Engine_Mt19937, __unserialize)
Z_PARAM_ARRAY_HT(d);
ZEND_PARSE_PARAMETERS_END();

/* Verify the expected number of elements, this implicitly ensures that no additional elements are present. */
if (zend_hash_num_elements(d) != 2) {
zend_throw_exception_ex(NULL, 0, "Invalid serialization data for %s object", ZSTR_VAL(engine->std.ce->name));
RETURN_THROWS();
}

/* members */
t = zend_hash_index_find(d, 0);
if (!t || Z_TYPE_P(t) != IS_ARRAY) {
Expand Down
5 changes: 5 additions & 0 deletions ext/random/engine_pcgoneseq128xslrr64.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ static bool unserialize(php_random_status *status, HashTable *data)
uint64_t u[2];
zval *t;

/* Verify the expected number of elements, this implicitly ensures that no additional elements are present. */
if (zend_hash_num_elements(data) != 2) {
return false;
}

for (uint32_t i = 0; i < 2; i++) {
t = zend_hash_index_find(data, i);
if (!t || Z_TYPE_P(t) != IS_STRING || Z_STRLEN_P(t) != (2 * sizeof(uint64_t))) {
Expand Down
5 changes: 5 additions & 0 deletions ext/random/engine_xoshiro256starstar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,11 @@ static bool unserialize(php_random_status *status, HashTable *data)
php_random_status_state_xoshiro256starstar *s = status->state;
zval *t;

/* Verify the expected number of elements, this implicitly ensures that no additional elements are present. */
if (zend_hash_num_elements(data) != 4) {
return false;
}

for (uint32_t i = 0; i < 4; i++) {
t = zend_hash_index_find(data, i);
if (!t || Z_TYPE_P(t) != IS_STRING || Z_STRLEN_P(t) != (2 * sizeof(uint64_t))) {
Expand Down
6 changes: 6 additions & 0 deletions ext/random/randomizer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,12 @@ PHP_METHOD(Random_Randomizer, __unserialize)
Z_PARAM_ARRAY_HT(d);
ZEND_PARSE_PARAMETERS_END();

/* Verify the expected number of elements, this implicitly ensures that no additional elements are present. */
if (zend_hash_num_elements(d) != 1) {
zend_throw_exception(NULL, "Invalid serialization data for Random\\Randomizer object", 0);
RETURN_THROWS();
}

members_zv = zend_hash_index_find(d, 0);
if (!members_zv || Z_TYPE_P(members_zv) != IS_ARRAY) {
zend_throw_exception(NULL, "Invalid serialization data for Random\\Randomizer object", 0);
Expand Down
32 changes: 24 additions & 8 deletions ext/random/tests/02_engine/all_serialize_error.phpt
View file
Open in desktop

Large diffs are not rendered by default.