Skip to content
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.

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

unserialize: Strictly check for :{ at object start #10214

Merged
merged 5 commits into from Jan 12, 2023

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Jan 3, 2023

This change adds validation for the serialization format during object serialization: Checks where missing to verify that objects actually start with :{, because the object format is not actually part of the grammar, likely because of the classname that is not necessary for arrays.

It also fixes a case where object_custom moved *p past the string bounds. While the contents at the pointer are not read afterwards, even the creation of such a pointer is technically UB.


It's not clear to me what the appropriate target branch would be. I think it should go into PHP 8.2 at least.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

This change looks reasonable but I would like another person's opinion.

Copy link
Contributor

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you! The stricter checking makes sense to me, but there should be a note in UPGRADING.

@TimWolla
Copy link
Member Author

@saundefined @adoy @ramsey Is this change acceptable as-is for PHP 8.2? This PR:

  • Fixes an actual bug: The previous logic might invoke undefined behavior (in the C language sense), because it creates an out-of-bounds pointer (no dereference needed to invoke UB). This fix is highlighted by the change to the ext/standard/tests/serialize/bug74111.phpt test.
  • Increases strictness of the unserialization parser. This is highlighted by the newly added tests. Specifically for serialized objects the leading :{ may be replaced by any byte without changing the result.

It does not introduce a behavioral change to valid serialization data, as emitted by serialize().

Personally I would prefer to apply this as-is to PHP 8.2 which still is a fairly young branch. If including the full PR is undesired for compat purposes, then I would like to apply the first bullet point to PHP 8.2 at least and the stricter checking to master only.

@saundefined
Copy link
Member

@TimWolla thanks!

I see no reason not to include as is in PHP-8.2, perhaps Ben and Pierrick would disagree with me.

@adoy
Copy link
Member

adoy commented Jan 12, 2023

@TimWolla Same as @saundefined it looks good to me. You can go ahead to merge this on 8.2

@TimWolla
Copy link
Member Author

Thank you, I'll rebase this later onto PHP-8.2 to run CI again, add NEWS and UPGRADING notes and then merge.

@TimWolla TimWolla changed the base branch from master to PHP-8.2 January 12, 2023 16:47
It's unlikely that the object syntax error contributed to the actual CVE. The
CVE is rather caused by the incorrect object serialization data of the `C`
format. Add a second string without such a syntax error to ensure that path is
still executed as well to ensure the CVE is absent.
No changes to the input required, because the test actually is intended to
verify the behavior for a missing `}`, it's just that the report position changed.
@TimWolla TimWolla merged commit f2e8c5d into php:PHP-8.2 Jan 12, 2023
@TimWolla TimWolla deleted the unserialize-strict branch January 12, 2023 18:57
TimWolla added a commit that referenced this pull request Jan 12, 2023
* PHP-8.2:
  unserialize: Strictly check for `:{` at object start (#10214)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants