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

Fix GH-16590: UAF in session_encode() #16640

Closed
wants to merge 1 commit into from
Closed

Conversation

nielsdos
Copy link
Member

The PS_ENCODE_LOOP does not protect the session hash table that it iterates over. Change it by temporarily creating a copy.

Only applied on 8.4 because of the ABI break. That should be fine as no one should be writing malicious code like this in the first place.
Asking RMs for permission: @SakiTakamachi @NattyNarwhal @ericmann

The `PS_ENCODE_LOOP` does not protect the session hash table that it
iterates over. Change it by temporarily creating a copy.
@nielsdos nielsdos requested a review from Girgias as a code owner October 29, 2024 18:45
@nielsdos nielsdos linked an issue Oct 29, 2024 that may be closed by this pull request
@nielsdos nielsdos changed the title GH-16590 (UAF in session_encode()) Fix GH-16590: UAF in session_encode() Oct 29, 2024
@ericmann
Copy link
Contributor

I'm always a fan of plugging UAF issues, so I have no problem with this unless @SakiTakamachi or @NattyNarwhal see something here that I don't.

@cmb69
Copy link
Member

cmb69 commented Oct 29, 2024

I don't think there is a practical ABI break (okay, the _zv variable may be used in the code, and there's a memory leak in case that code returns). The more relevant problem I see is that extensions which will not be rebuilt after this patch, will not profit from it. But even that doesn't appear to be a real issue ("malicious code").

@NattyNarwhal
Copy link
Member

NattyNarwhal commented Oct 29, 2024

I think this is good to merge to earlier releases too. I don't think it's an ABI break for the reasons Christoph outlined.

@nielsdos
Copy link
Member Author

Maybe not per se ABI break, but it is an API break at least. A third party extension that uses PS_ENCODE_LOOP and uses return in the loop will suddenly have to change their code to not leak memory.

@NattyNarwhal
Copy link
Member

Maybe not per se ABI break, but it is an API break at least. A third party extension that uses PS_ENCODE_LOOP and uses return in the loop will suddenly have to change their code to not leak memory.

I tried to look for users of PS_ENCODE_LOOP on GitHub (Not a perfect strategy, not everything is on GitHub). The only external user I could find was Susohin, which did do an early return. That's also PHP 5, so... (Well, WDDX is also a user, but it uses it very trivially and it's a former member of ext/.)

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.

MSTM, will let RM decide where it can land.

@nielsdos
Copy link
Member Author

nielsdos commented Nov 4, 2024

Seems like there's not really a problem for the RMs to go lower than 8.4, but I will only push it to 8.4 because of the theoretical break and the fact that users are unlikely to encounter this anyway. If really necessary, it can always be backported.

@nielsdos nielsdos closed this in cc39bc2 Nov 4, 2024
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.

UAF in session_encode()
5 participants