Fix GH-22480: phpdbg use-after-free re-watching a watched variable#22493
Fix GH-22480: phpdbg use-after-free re-watching a watched variable#22493iliaal wants to merge 1 commit into
Conversation
|
I don't know PHPDBG nor have the time to learn how it works rn. |
|
This will have to wait until the weekend, I don't have time right now. |
| GH-22480 (Use-after-free when re-watching an already-watched variable) | ||
| --SKIPIF-- | ||
| <?php | ||
| if (getenv('SKIP_ASAN')) { |
There was a problem hiding this comment.
Leftover from when the test demonstrated the crash. It's ASAN-clean with the fix, so I dropped the skip and let it run under ASAN.
There was a problem hiding this comment.
Judging by the last force push, they are re-added now
There was a problem hiding this comment.
Ugh, the drop didn't make it into the last push. Removed now.
| GH-22480 (Use-after-free re-watching a sub-path of a recursive watchpoint) | ||
| --SKIPIF-- | ||
| <?php | ||
| if (getenv('SKIP_ASAN')) { |
There was a problem hiding this comment.
Same, a stale crash-marker. Dropped it so this one runs under ASAN too.
|
|
||
| phpdbg_watch_element *phpdbg_add_bucket_watch_element(Bucket *bucket, phpdbg_watch_element *element) { | ||
| phpdbg_watchpoint_t watch; | ||
| phpdbg_watch_element *added; |
There was a problem hiding this comment.
Merge declaration and assignment
| phpdbg_watch_parent_ht(element); | ||
| return element; | ||
| added = phpdbg_add_watch_element(&watch, element); | ||
| if (added == element) { |
There was a problem hiding this comment.
Doesn't this risk ABA-like issues?
There was a problem hiding this comment.
You're right, phpdbg_add_watch_element() frees element on a duplicate and returns the pre-existing one, so added == element read a freed pointer. Threaded a bool out-param through the add functions so callers test that instead, and did the same for the sibling checks at the recursion and array-watch sites.
iliaal
force-pushed
the
fix/gh-22480-phpdbg-watch-uaf
branch
2 times, most recently
from
cd00f35 to
6d9d07d
Compare
phpdbg_add_watch_element() frees the passed element and returns the existing one when the variable is already watched, but several callers kept using the freed pointer, so re-watching a variable was a use-after-free in more places than the two reported. Report the duplicate back through the parse callback and register nothing, and reject a recursive watch's hashtable element that collides with an existing watch, at creation and recreation, rather than freeing a still-referenced one. A recursive watch over an already-array-watched variable now watches only the root and skips the shared hashtable. Fixes phpGH-22480
iliaal
force-pushed
the
fix/gh-22480-phpdbg-watch-uaf
branch
from
6d9d07d to
796b0fd
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Complex / intricate code, but okay.
Please note the following:
- I put a cleanup remark about using enum returns, this is non-blocking
- watch_006 is marked as XFAIL, but likely no longer should. Please double check.
| static int phpdbg_create_simple_watchpoint(zval *zv, phpdbg_watch_element *element) { | ||
| element->flags = PHPDBG_WATCH_SIMPLE; | ||
| phpdbg_add_bucket_watch_element((Bucket *) zv, element); | ||
| #define PHPDBG_WATCH_DUPLICATE 1 |
There was a problem hiding this comment.
Instead of mixing SUCCESS / this define; it would be cleaner if we had an enum and used that enum as a return type.
|
Interesting I didn't see UAFs locally or before in CI/CD let me investigate, this needs more checking |
3 participants
Fixes #22480
phpdbg_add_watch_element()frees the passed element and returns the existing one when the variable is already watched, but several callers kept using the freed pointer, so re-watching a variable was a use-after-free. The two reported derefs are not the whole bug: with only the proposed two-line capture,w a $atyped twice still crashes, relocated to a later interned-string read. This reports the duplicate back through the parse callback so the wrapper registers nothing, and rejects a recursive watch's hashtable element that collides with an existing watch, at creation and recreation, instead of freeing a still-referenced one.