Hi!
I've recently committed a number of fixes to 5.x branch. These fixes
mainly concern (un)serialization scenarios, you can see the full list in
5.4/5.5 NEWS. These changes are not merged yet to master/7.0 since due
to extensive differences between 5.x and 7 in zval handling, they
basically must be rewritten for 7. I don't want to commit completely
broken code to master, so I'll work on at least getting it to a state
where there is no new breakage and then porting the fixes properly to 7,
but that can take a couple of days. In the meantime, please be aware
that 5.x and master may not be in full sync and exercise caution if you
merge stuff from 5 to 7.
Stas Malyshev
smalyshev@gmail.com
Hi Stas,
There are many fixes regarding unserialize.
We also had many fixes regarding type mismatches.
I suppose many 3rd party modules have same issues.
How about have a doc for secure PHP internal coding?
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
I've recently committed a number of fixes to 5.x branch. These fixes
mainly concern (un)serialization scenarios, you can see the full list in
5.4/5.5 NEWS. These changes are not merged yet to master/7.0 since due
to extensive differences between 5.x and 7 in zval handling, they
basically must be rewritten for 7. I don't want to commit completely
broken code to master, so I'll work on at least getting it to a state
where there is no new breakage and then porting the fixes properly to 7,
but that can take a couple of days. In the meantime, please be aware
that 5.x and master may not be in full sync and exercise caution if you
merge stuff from 5 to 7.Stas Malyshev
smalyshev@gmail.com
Hi Stas,
There are many fixes regarding unserialize.
We also had many fixes regarding type mismatches.
I suppose many 3rd party modules have same issues.How about have a doc for secure PHP internal coding?
I'm writing the draft.
I see number of var_push_dtor() to fix unserialization.
var_push_dtor() or var_push_dtor_no_addref() is required always when
php_var_unserialize() is failed.
Am I correct?
It will cover
- Pointers to general secure programming resources
- Basic memory management and debugging (how to use run-tests.php)
- Unserialization
- Type confusion
- Typical overflows
If there is anything to add, please let me know.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
I see number of var_push_dtor() to fix unserialization.
var_push_dtor() or var_push_dtor_no_addref() is required always when
php_var_unserialize() is failed.
Am I correct?
Not necessarily. Basically, what happens is that when you do
php_var_unserialize() the value you unserialize gets a slot in the
reference table. So if you are destroying it and some serialization part
later tries to access it - boom! That's where var_push_* comes in. The
first one adds refcount so even if everybody drops references to it the
value still survives. The second is used when we know no code uses this
value, so no need to add refcount - only thing that should keep it alive
is that it's still in the refs table (except for the case where it is
taken from the table by r: or R: - then its refcount is bumped).
Now the failure is tricky case - generally after the failure we need to
bail out ASAP, but that failure may not be the final failure -
serializes can be nested. So we may want to keep the values around even
if our unserialize fails, because encompassing serialize may still
reference that variable, since we may have already put it in the refs
table. We could make serialize failure a fatal error, but that'd be
pretty big BC break (exception wouldn't work, we'd need full blown fatal
E_ERROR).
In general, the idea is that anything that was put in reference table
(namely, anything that passed through php_var_unserialize()) should be
kept alive until the end of unserialize() call. Which means it should be
put to dtor list via var_push_dtor() or var_push_dtor_no_addref().
Now, that all was true for PHP 5. For PHP 7, the matter are more
complicated as the lists now keep zvals and not zval *, and I'm not 100%
sure yet what that means. I need to look into it and figure out.
Stas Malyshev
smalyshev@gmail.com
Hi Stas,
I see number of var_push_dtor() to fix unserialization.
var_push_dtor() or var_push_dtor_no_addref() is required always when
php_var_unserialize() is failed.
Am I correct?Not necessarily. Basically, what happens is that when you do
php_var_unserialize() the value you unserialize gets a slot in the
reference table. So if you are destroying it and some serialization part
later tries to access it - boom! That's where var_push_* comes in. The
first one adds refcount so even if everybody drops references to it the
value still survives. The second is used when we know no code uses this
value, so no need to add refcount - only thing that should keep it alive
is that it's still in the refs table (except for the case where it is
taken from the table by r: or R: - then its refcount is bumped).Now the failure is tricky case - generally after the failure we need to
bail out ASAP, but that failure may not be the final failure -
serializes can be nested. So we may want to keep the values around even
if our unserialize fails, because encompassing serialize may still
reference that variable, since we may have already put it in the refs
table. We could make serialize failure a fatal error, but that'd be
pretty big BC break (exception wouldn't work, we'd need full blown fatal
E_ERROR).In general, the idea is that anything that was put in reference table
(namely, anything that passed through php_var_unserialize()) should be
kept alive until the end ofunserialize()call. Which means it should be
put to dtor list via var_push_dtor() or var_push_dtor_no_addref().Now, that all was true for PHP 5. For PHP 7, the matter are more
complicated as the lists now keep zvals and not zval *, and I'm not 100%
sure yet what that means. I need to look into it and figure out.
Thank you for the detailed explanation!
It's very helpful and it seems I'm better to wait for unserialize() patch.
I'll finish other parts first.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net