ext/pdo: Fix a UAF when changing default fetch class ctor args #17579
Conversation
ext/pdo/pdo_stmt.c
Outdated
| PDO_STMT_CLEAR_ERR(); | ||
| /* Increase refcount for ctor_args as those might be removed during individual fetches */ | ||
| bool increase_refcount_ctor = Z_TYPE(stmt->fetch.cls.ctor_args) == IS_ARRAY; | ||
| if (increase_refcount_ctor) { |
There was a problem hiding this comment.
I wonder if it's better to combine this with the ZVAL_COPY_VALUE, i.e. make that ZVAL_COPY. However, this is not hugely important (it also depends on how this would complicate the resolution of my second comment).
ext/pdo/pdo_stmt.c
Outdated
| } | ||
|
|
||
| do_fetch_opt_finish(stmt, 0); | ||
| if (increase_refcount_ctor) { |
There was a problem hiding this comment.
I suppose it's theoretically possible that the ctor_args is changed by user code during the do_fetch operation.
In that case we may be destroying the wrong instance.
So I think you should keep a pointer to the HashTable that you initially refcount-incremented.
There was a problem hiding this comment.
Actually this seems like a more sensible approach, and more general let me try some new test cases and this idea.
Girgias
force-pushed
the
pdo-uaf-set-fetch-mode-stuffing
branch
from
ffa55b3 to
e124848
Compare
|
|
||
| do_fetch_opt_finish(stmt, 0); | ||
| if (current_ctor) { | ||
| // TODO: can current_ctor contain cycles? If yes, then this should be added as possible root (or be handled via a zval*) |
There was a problem hiding this comment.
This TODO should be removed upon squash-merging.
|
Merged as 3027600 |
2 participants
No description provided.