Everything

php

Home » Project » Newsgroups » php.internals » Dismantling the lies... »

Dismantling the lies...

php.internals

Stefan Esser

19 years ago

Hello,

it is no secret that I am really sick and tired of this constant stream
of nonsense and
lies comming out of the mouths of PHP developers when it comes to
security issues.

In the other thread, where Stanislav spreads the usual lie/propaganda that
there is no help comming from me to the PHP developers, he also claims
that the MOPB issues #1 and #2 cannot be fixed right now.

Xdebug, Suhosin, Hardening Patch have already demonstrated for years that
it is not true that #2 [2] cannot be fixed without breaking binary
compatibility.
There have also been patches on this mailinglist that calculated the maximum
depth automatically, therefore there is no need to dismantle this lie.

The other lie however that MOPB issue #1 [1]  is unfixable ends here...

First of all everyone into PHP development knows that the obvious "fix" for
this issue would be to just break binary compatibility and use a 32 bit
reference
counter. It does not fix the actual problem but it is enough so that it
cannot be
triggered anymore.

The reason for this fix not being applied is not it's impossibility, but
because the
closed source extension developers (everyone knows who they are) don't want
another binary compatibility break, because then their closed source
extensions
have to be shipped in yet another version.

However there exists another fix to the problem that deals with the
actual problem
of an overflowing reference counter. Therefore every refcount increase
in the Zend
Engine Source has to be protected. While this sounds much of work it
actually takes
less than half an hour to do it.

Here is the patch I created in approximately half an hour. A solution to
a problem
that is *NOT* fixable at the moment, according to Stanislav.

http://www.hardened-php.net/patches/php-4.4.7-refcount-overflow-fix.patch.gz
MD5: 0b558564d86b798651b69181920f9378

Stefan Esser


Reference:
[1] - reference counter overflow -
http://www.php-security.org/MOPB/MOPB-01-2007.html
[2] - deep recursion crash -
http://www.php-security.org/MOPB/MOPB-02-2007.html

Cristian Rodriguez

19 years ago

> Here is the patch I created in approximately half an hour. A solution to
> a problem

> that is *NOT* fixable at the moment, according to Stanislav.

at openSUSE, we also have a patch for this issue since a few weeks, as
a vendor unfortunately we have to take care of things that  people
here dont want to fix...

http://www.flyspray.org/patches/MOPB-01-abicompatible.patch.bz2

Stefan Esser

19 years ago

Cristian Rodriguez schrieb:

>> Here is the patch I created in approximately half an hour. A solution to
>> a problem
>
>> that is *NOT* fixable at the moment, according to Stanislav.
>
> at openSUSE, we also have a patch for this issue since a few weeks, as
> a vendor unfortunately we have to take care of things that  people
> here dont want to fix...
>
> http://www.flyspray.org/patches/MOPB-01-abicompatible.patch.bz2
>

I only see one major problem with this patch. You just call zend_error()
when a possible
integer overflow is detected. This is however not really safe. Refcount
increases etc...
are usually done deep in the engine and it is not ensured that in case
of a bailout
this cannot result in destruction of structures that were only partialy
initialised.

Stefan Esser

Stanislav Malyshev

19 years ago

> at openSUSE, we also have a patch for this issue since a few weeks, as
> a vendor unfortunately we have to take care of things that  people
> here dont want to fix...
>
> http://www.flyspray.org/patches/MOPB-01-abicompatible.patch.bz2

Did you perform the performance tests for this patch? What were the results?

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Cristian Rodriguez

19 years ago

2007/5/21, Stanislav Malyshev <stas@zend.com>:

> > at openSUSE, we also have a patch for this issue since a few weeks, as
> > a vendor unfortunately we have to take care of things that  people
> > here dont want to fix...
> >
> > http://www.flyspray.org/patches/MOPB-01-abicompatible.patch.bz2
>
> Did you perform the performance tests for this patch? What were the results?

no, no performance tests has been done over that patch, but as rasmus
said. it is probably negible or acceptable for real world.. I only
knows it works with real like code ( i tried ezpublish, phpmyadmin)
and passes the PHP test suite correctly ( or no worse than without the
patch)

Stanislav Malyshev

19 years ago

> no, no performance tests has been done over that patch, but as rasmus
> said. it is probably negible or acceptable for real world.. I only
> knows it works with real like code ( i tried ezpublish, phpmyadmin)
> and passes the PHP test suite correctly ( or no worse than without the
> patch)

Well, php test suite is a functional test, so if the patch is done
correctly it'd pass of course. I'm moe worried about the performance -
my experience shows that the guesses are often wrong when talked about
performance effects in complicated code. If you would have time to run a
benchmark using something like bench.php and maybe some more complicated
code, it'd be nice to know the result.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Cristian Rodriguez

19 years ago

2007/5/21, Stanislav Malyshev <stas@zend.com>:

>I'm moe worried about the performance -

the vast mayority of users will be happy with a more secure system and
will  like accept a 0.0.0.0.0.0.0.0.0.1% slowdown in case it is slow..
I sometimes wonder what it is real target of PHP, the masses or a
selected neglible group of users that requires extreme craze
performance..

If you would have time to run a

> benchmark using something like bench.php and maybe some more complicated
> code, it'd be nice to know the result.

maybe I can check that tomorrow.. but you can also run it by yourself :P

However , is in your hands to end with this strange policy, you can
either make the 32 bit reference counter, apply this hackaround or
state publicitely in the php homepage that PHP4 will not get
proper/complete security fixes anymore because you dont want to break
binary compatibility to protect corporate interests or simple because
it is dead. that would be clearer for users and for all the
people/companies that distributes/uses your software.

Stanislav Malyshev

19 years ago

> the vast mayority of users will be happy with a more secure system and
> will  like accept a 0.0.0.0.0.0.0.0.0.1% slowdown in case it is slow..

Sure, if only we knew that's not 10% slowdown.

> I sometimes wonder what it is real target of PHP, the masses or a
> selected neglible group of users that requires extreme craze
> performance..

Who is "the masses"? Almost everybody I know isn't the masses and is
concerned about performance in production. And performance hits are
cumulative - ignore 1% performance hit 10 times and you have 10% at
least...

> maybe I can check that tomorrow.. but you can also run it by yourself :P

I can. Independent verification could be useful though.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Stefan Esser

19 years ago

>
> Well, php test suite is a functional test, so if the patch is done
> correctly it'd pass of course. I'm moe worried about the performance -
> my experience shows that the guesses are often wrong when talked about
> performance effects in complicated code. If you would have time to run
> a benchmark using something like bench.php and maybe some more
> complicated code, it'd be nice to know the result.

bench.php is a totally unrealistic benchmark that has nothing todo with
real life PHP applications. Of course when you call a function several
million times the performance penalty will be big if you add code that
executed with every function call. But in reality PHP applications don't
call millions of functions. Most of the waiting time in PHP applications
is waiting for disk/DB I/O operations.

Stefan Esser

Stanislav Malyshev

19 years ago

> bench.php is a totally unrealistic benchmark that has nothing todo with
> real life PHP applications. Of course when you call a function several

It tests the engine speed. Of course, in life of PHP script there are
other things, that's why I said "and" - to put it in context.

> million times the performance penalty will be big if you add code that
> executed with every function call. But in reality PHP applications don't
> call millions of functions. Most of the waiting time in PHP applications

Maybe not millions for typical application, but thousands and tens of
thousands - they do. In any case, the patch in question added function
call (or at the very best branch) with every variable access - which
could be millions.

> is waiting for disk/DB I/O operations.

That's why people do caching.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

David

19 years ago

Hi,

I'm David Wang and I'm one of the students that is participating in Google Summer of Code for this summer. My project addresses detecting reference cycles in PHP, and I'm following this discussion because it seems very pertinent to what I will be working with this summer. I'm currently planning on implementing Bacon and Rajan's synchronous cycle collection algorithm based on reference counting. The algorithm requires performing a simple additional task for each increment or decrement of the reference counter. There is a branch in the decrement operation but there is no branch in the increment operation. I wasn't aware that a simple branch such as the one in the proposed patch would incur significant performance penalties on modern superscalar processors, since both execution paths would be followed simultaneously. However, I would also be very grateful for any performance data anyone can provide on this subject (and really, any other advice anyone has!).

The function call does strike me as completely unnecessary and the additional stack operations required are expensive. I'm willing to be that if the function was converted to a macro, the proposed solution would be a workable fix. Just my two cents; I'm not very experienced with the PHP internals yet.

Thanks all!

David Wang

-----Original Message-----
From: Stanislav Malyshev [mailto:stas@zend.com]
Sent: Sunday, May 20, 2007 11:08 PM
To: Cristian Rodriguez
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Dismantling the lies...

> no, no performance tests has been done over that patch, but as rasmus
> said. it is probably negible or acceptable for real world.. I only
> knows it works with real like code ( i tried ezpublish, phpmyadmin)
> and passes the PHP test suite correctly ( or no worse than without the
> patch)

Well, php test suite is a functional test, so if the patch is done
correctly it'd pass of course. I'm moe worried about the performance -
my experience shows that the guesses are often wrong when talked about
performance effects in complicated code. If you would have time to run a
benchmark using something like bench.php and maybe some more complicated
code, it'd be nice to know the result.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Derick Rethans

19 years ago

On Sun, 20 May 2007, David wrote:

> The function call does strike me as completely unnecessary and the
> additional stack operations required are expensive. I'm willing to be
> that if the function was converted to a macro, the proposed solution
> would be a workable fix. Just my two cents; I'm not very experienced
> with the PHP internals yet.

I actually have a patch for that somewhere, let's see if I can find that
if I get back to the office.

regards,
Derick

Stefan Esser

19 years ago

> The function call does strike me as completely unnecessary and the additional stack operations required are expensive. I'm willing to be that if the function was converted to a macro, the proposed solution would be a workable fix. Just my two cents; I'm not very experienced with the PHP internals yet.
>

Like I said the patch was hacked in 30 minutes, to proove it is
possible. I am well aware that a function call is additional overhead.

Don't forget that the MOPB-1 issue was disclosed on the 1st March. This
was 2.5 months ago. 2.5 months vs. 30 minutes.
Additionally the PHP dev team knows this problem for YEARS and it was
the only possible solution to expose the problem during
the MOPB to get it ever fixed.

I am fully aware that it can be made faster. But a slow solution is
better than no solution at all.

Stefan Esser

Stanislav Malyshev

19 years ago

> I am fully aware that it can be made faster. But a slow solution is
> better than no solution at all.

Actually in many situations it isn't. Since as far as I can see the
problem can lead to real harm only in rather limited set of situations,
making the engine always considerably slower just to fix it does not
seem a very good solution to me.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Stefan Esser

19 years ago

Stanislav Malyshev schrieb:

>> I am fully aware that it can be made faster. But a slow solution is
>> better than no solution at all.
>
> Actually in many situations it isn't. Since as far as I can see the
> problem can lead to real harm only in rather limited set of
> situations, making the engine always considerably slower just to fix
> it does not seem a very good solution to me.

Well yes. I think to solve this "once and for all" a public statement by
the PHP group would be nice that says:

We think that local vulnerabilities that allow people who managed to
execute PHP code on the server
through a PHP script vulnerability or those on shared hosting to launch
further attacks, like stealing data
from apache memory or takeover the webserver socket (when mod_php is
used) or to launch direct
kernel exploits (which would not be possible if PHP would be secure), or
a bunch of other attacks that
are not possible from PHP code, are not important. We therefore won't
fix them.

This statement would be honest and would be a good warning sign for
people to choose another language.

Stefan Esser

Stanislav Malyshev

19 years ago

> Well yes. I think to solve this "once and for all" a public statement by
> the PHP group would be nice that says:

I don't think they are "not important", just that they are not important
enough to want them fixed no matter the cost. Running shared hosted
server in a mode that relies on restricted code IMO is wrong anyway, and
for non-shared environment these problems could be exploited only if
specifically enabled by very badly written code. So when there's a
trade-off between having the language work better for 100% of cases or
protect those who run broken code on their servers - the choice would be
to make language run better. Again, that doesn't mean bugs shouldn't be
fixed - just the fix shouldn't make the situation worse.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Lester Caine

19 years ago

Stefan Esser wrote:

> Stanislav Malyshev schrieb:
>>> I am fully aware that it can be made faster. But a slow solution is
>>> better than no solution at all.
>> Actually in many situations it isn't. Since as far as I can see the
>> problem can lead to real harm only in rather limited set of
>> situations, making the engine always considerably slower just to fix
>> it does not seem a very good solution to me.
> Well yes. I think to solve this "once and for all" a public statement by
> the PHP group would be nice that says:

"We are no longer wasting time fixing security problems in PHP4 that have
already been addressed in the current version of PHP - PHP5 - So no further
development time will be wasted on PHP4" :)
This decision has been made to allow developers to move forward to get PHP6
out of the door ;)

--
Lester Caine - G8HFL
-----------------------------
Contact - http://home.lsces.co.uk/lsces/wiki/?page=contact
L.S.Caine Electronic Services - http://home.lsces.co.uk
MEDW - http://home.lsces.co.uk/ModelEngineersDigitalWorkshop/
Firebird Foundation Inc. - http://www.firebirdsql.org/index.php

Stanislav Malyshev

19 years ago

> at openSUSE, we also have a patch for this issue since a few weeks, as
> a vendor unfortunately we have to take care of things that  people
> here dont want to fix...
>
> http://www.flyspray.org/patches/MOPB-01-abicompatible.patch.bz2

I tested this patch on bench.php - surprisingly, no substantial
performance difference. So I guess you could submit it to Derick for
inclusion into 4.x tree, if there's no other objections to it. Only
thing is that I don't see macros ZVAL_SAFE_REF_ADD and ZVAL_SAFE_REF_SET
used anywhere, so probably there's no need for them.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Stanislav Malyshev

19 years ago

> thing is that I don't see macros ZVAL_SAFE_REF_ADD and ZVAL_SAFE_REF_SET
> used anywhere, so probably there's no need for them.

My bad - they are used of course, I just looked into the wrong place.
I would however rename ZVAL_SAFE_REF to something like ZVAL_SAFE_ADDREF.
Also, comparing values to 0xffff might be not effective if they are
ushort...

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Rasmus Lerdorf

19 years ago

Stefan Esser wrote:

> The reason for this fix not being applied is not it's impossibility, but
> because the
> closed source extension developers (everyone knows who they are) don't want
> another binary compatibility break, because then their closed source
> extensions
> have to be shipped in yet another version.

I don't have any commercial closed source extensions and am not
affiliated in any way with anyone who has some, and I'd rather not see a
PHP 4.5 just for this fix either.

> However there exists another fix to the problem that deals with the
> actual problem
> of an overflowing reference counter. Therefore every refcount increase
> in the Zend
> Engine Source has to be protected. While this sounds much of work it
> actually takes
> less than half an hour to do it.
>
> Here is the patch I created in approximately half an hour. A solution to
> a problem
> that is *NOT* fixable at the moment, according to Stanislav.

You make things sound very black and white when they are usually grey.
In this case the main objection to this rather obvious fix has always
been performance.  Adding a check on every refcount increase is a bit
scary for the performance folks.  It may be that in most realworld cases
this is an acceptable performance tradeoff.  We have to balance the
seriousness of the vulnerability against the performance cost of the
fix.  The thinking on this one has been that the actual vulnerability is
rather obscure, which I know you don't agree with, but that's been the
driving factor which makes it hard to justify any performance tradeoff
to fix it.

As you said, moving to a 32-bit refcount is a better fix which is why we
chose to go that route and encourage people who feel this is not an
obscure vulnerability in their environment to migrate to PHP 5.

-Rasmus

Stefan Esser

19 years ago

> You make things sound very black and white when they are usually grey.
>

You only don't realise how black things are.

> this is an acceptable performance tradeoff.  We have to balance the
> seriousness of the vulnerability against the performance cost of the
>

Yeah well. Luckily since Suhosin-Patch exists, people have a choice. And
they choose Security
(Suhosin) over performance. And sorry a tradeofff between performance
and security is never
a choice, when we are speaking about a simple compare operation.
You also did not even implement a simple compare per variable ptr dtor
that checks if the
refcount was already 0 on entry. This is not a 100% solution but it
catches a bunch of attacks
and it even catches the demonstration exploit for MOPB-1.
The choice of the PHP development team is to do NOTHING. And I am just
reporting about
this to make people aware. I know this is taken as attack from the PHP
development team, but
I am just reporting FACTS.

> fix.  The thinking on this one has been that the actual vulnerability is
> rather obscure, which I know you don't agree with, but that's been the
> driving factor which makes it hard to justify any performance tradeoff
> to fix it.
>

Considering the fact that the reference counter overflow was for example
triggerable through
unserialize() which made for example every phpBB Forum and every
serendipity vulnerable
to remote compromise. (An exploit exists in metasploit), makes your
statemt look rather
DUMB. Unfortunately I had to fix the symptom instead of the cause
because I was forbidden
to fix it correctly in PHP. The refcount overflow is therefore still a
ticking timebomb until someone
finds another way to trigger it remotely. This behaviour of PHP.net is
by the way the real reason
why Suhosin/HPHP exists: You only think in short terms, you don't plan
in the long run and
realise that fixing only symptoms solves no problems.

And well, I am obviously not the only one that believes the thing has to
be fixed, as the patch
by SuSE prooves. But well it is YOUR choice to ignore my warnings. But
then just ignore it
and don't resort to attacks like Stanislav and his php-shrink. Stop
complaining that I would not
help, because this is simply a lie. As a security researcher my job is
to point out bugs, not to fix
them. And if I do so (like I do so often f.e. in Suhosin), it is my
choice to do this outside of PHP.
And PHP.net has absolutely NO RIGHT to demand that I fix something. I
have no problem with
this anyway because your lack of compliance just drives more people into
the arms of Suhosin.
Which seems to be something you don't understand.

> As you said, moving to a 32-bit refcount is a better fix which is why we
> chose to go that route and encourage people who feel this is not an
> obscure vulnerability in their environment to migrate to PHP 5.
>

Uhm... I pretty much said the opposite. Moving to a 32-bit refcount does
not fix anything. It just makes
the bug not triggerable on 32 big platforms, because the memory required
would exceed the 32 bit
addresspace. On 64 bit systems things would be different again... (Again
a fixing symptoms approach)

PS: You should also think about stopping to claim that current PHP is
free of security bugs, when you
mean CVS. Current PHP is not CVS but the latest release. (And don't tell
me that it is not you who
claim it, but Stanislav. If you think otherwise you can correct him
anytime.)

Stefan

Mathias Bank

19 years ago

Rasmus Lerdorf schrieb:

> Adding a check on every refcount increase is a bit
> scary for the performance folks.  It may be that in most realworld cases
> this is an acceptable performance tradeoff.  We have to balance the
> seriousness of the vulnerability against the performance cost of the
> fix.

Sorry, but I don't agree with you. You have to think about people, who
are concerned in performance. Performance is relevant in big web
applications. And I think, that in such big applications security is one
of the most important things. I think, no responsible person would
decide to use php for a performance critical application when he/she
knows, that there is a security leak.

In this way, I'm sure, that security is more important.

Mathias

Stanislav Malyshev

19 years ago

> In the other thread, where Stanislav spreads the usual lie/propaganda
> that there is no help comming from me to the PHP developers, he also
> claims

For the record - I never said that. I said that in that thread, as
in some of its predecessors, when it came from general discussion to the
substance Stefan somehow lost interest to the substance part. Of course,
he's not in any obligation to contribute to PHP and his substantial help
- when it indeed happens - is always welcome. I just noted in that
particular case it didn't happen, regretfully. Now it happens, which I
can only be glad about.

> that the MOPB issues #1 and #2 cannot be fixed right now. Xdebug,
> Suhosin, Hardening Patch have already demonstrated for years that it
> is not true that #2 [2] cannot be fixed without breaking binary
> compatibility.

Actually #1 can't be, as I see it. #2 can, but that requires arbitrarily
limiting stack depth as I far as I can see.

> There have also been patches on this mailinglist that calculated the
> maximum depth automatically, therefore there is no need to dismantle
> this lie.

I don't imagine how one really could calculate maximum depth without
solving the halting problem, so I must be missing something. I ask
somebody who knows what these patches are to send me a link - if there
were patches that do that automatically for any code I would very much
like to see them.
Adding the arbitrary - even configurable - limit doesn't seem to me the
correct solution, since it has high potential to break application code
- with the same effect as the actual bug would. In the context of the
debugger it might be OK since the developer would get immediate feedback
and thus could know what and why crashed, in the context of runtime
application that could be as bad as original bug. Also, since stack size
differs for different OSes, it would be rather hard to find good default
for it. BTW, as far as I can understand, Hardened PHP patch has this
protection disabled by default.

> First of all everyone into PHP development knows that the obvious
> "fix" for this issue would be to just break binary compatibility and
> use a 32 bit reference counter. It does not fix the actual problem
> but it is enough so that it cannot be triggered anymore.

Yes, this is correct. However, that would require releasing
binary-incompatible PHP 4 version, which is not supported by the group
as I understand.

> However there exists another fix to the problem that deals with the
> actual problem of an overflowing reference counter. Therefore every
> refcount increase in the Zend Engine Source has to be protected.
> While this sounds much of work it actually takes less than half an
> hour to do it.

That would also incur considerable performance penalty. As I see in your
patch, you convert a number of increments (which would normally generate
single increment instruction) into function calls. Also, it doesn't
cover extensions accessing refcounts directly (and they do that) - which
is less of a problem, those could be converted too. But performance loss
makes me worry. Did you test the performance difference for the patch?
Even if function calls are converted to a macro, it still might have
some performance lost since refcount change is a very frequent operation
in the engine and inserting a branch for it could hurt performance.
However, if I see somebody showing the patch with near-zero effect it
can be a good case for including such patch.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Stefan Esser

19 years ago

> I don't imagine how one really could calculate maximum depth without
> solving the halting problem, so I must be missing something. I ask
> somebody who knows what these patches are to send me a link - if there
> were patches that do that automatically for any code I would very much
> like to see them.
> Adding the arbitrary - even configurable - limit doesn't seem to me the
> correct solution, since it has high potential to break application code

Maybe I was a bit unspecific. If I recall correctly Nuno had some patch
(or was it someone else) that was keeping track of depth and maximum
stack size
and was deciding on the fly if another step deeper could crash. Of
course you need some sane detection.
Other languages like Python or Perl (I actually don't remember which one
of the two is protected) have a stack depth protection, and live happily
with it.

And the arbitrary limit argument. Well if you write portable PHP code
you have an arbitrary limit anyway. IIRC on some Solaris Sparc systems
the crash limit was at 800.

> understand, Hardened PHP patch has this protection disabled by default.

Hardened-PHP is no more. As a thankyou for my security work I was
forbidden to carry the PHP tag in the name.
But this is history and now Suhosin exists. And the only reason it is
deactivated by default is that without knowing
the system and code it is hard to detect a sane limit. I personally
believe that 256 is enough for everyone.

And even if you require a deeper limit Suhosin has a nice simulation
mode that will not block deeper recursion, so
that you can run the simulation mode on your development server for
weeks and see if you ever violate the 256
depth.

Stefan Esser

Stanislav Malyshev

19 years ago

> Maybe I was a bit unspecific. If I recall correctly Nuno had some patch
> (or was it someone else) that was keeping track of depth and maximum
> stack size
> and was deciding on the fly if another step deeper could crash. Of

I would very much like to know how he does that. If the solution does
not involve arbitrary stack limits then I think it's acceptable.

> course you need some sane detection.
> Other languages like Python or Perl (I actually don't remember which one
> of the two is protected) have a stack depth protection, and live happily
> with it.

As far as I can see on my tests, Perl just runs out of memory.
Apparently PHP uses more stack than Perl. The result more or less the
same anyway - script just dies.

> And the arbitrary limit argument. Well if you write portable PHP code
> you have an arbitrary limit anyway. IIRC on some Solaris Sparc systems
> the crash limit was at 800.

The problem is I see no generic way to know that "legitimate"
applications would run with the limit and illegitimate won't. I.e.
nothing prevents one from setting the limit to 1000 on solaris and get
the same crashes, or from having legitimate application which goes more
than 256 levels deep. If we could make the limit not arbitrary but based
on capabilities of concrete system - then it would be very useful.

> deactivated by default is that without knowing
> the system and code it is hard to detect a sane limit. I personally

exactly. And if it's disable by default 99% of installations would have
it run with default and thus such protection would be not much of a use.
It's not only without knowing - even having the system it might be not
that easy to figure it out - different calls consume different amount of
stack, especially if there are internal functions in between.

> believe that 256 is enough for everyone.

I'm not sure it is. If there's an app which, for example, recursively
parses data structures, I don't see why it can't be more than 256 levels
deep. Good solution would be rely to real limits.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Nuno Lopes

19 years ago

>> I don't imagine how one really could calculate maximum depth without
>> solving the halting problem, so I must be missing something. I ask
>> somebody who knows what these patches are to send me a link - if there
>> were patches that do that automatically for any code I would very much
>> like to see them.
>> Adding the arbitrary - even configurable - limit doesn't seem to me the
>> correct solution, since it has high potential to break application code
>
> Maybe I was a bit unspecific. If I recall correctly Nuno had some patch
> (or was it someone else) that was keeping track of depth and maximum
> stack size
> and was deciding on the fly if another step deeper could crash. Of
> course you need some sane detection.

For the record the patch I had is this:
http://mega.ist.utl.pt/~ncpl/zend_stack_protection.txt (it shouldn't apply
cleanly due to some changes in zend_try some time ago).
The approach of this patch is not to know in advance if there's still
available stack, but rather catch the SIGSEGV and bailout. I used this
approach in order to catch problems in external libraries, too. By that time
I was thinking in pcre, which was pretty easy to segfault with user-provided
data (now this is not the case because its internal recursion depth can be
limited - although with a PHP_INI_ALL config).
This approach is not very portable though, and by the time I was told that
probably it wouldn't play well with Apache signal handlers.

Knowing in advance if you can recurse or not doesn't sound much difficult in
theory.. You can get the limit (e.g. with getrlimit) then you can know how
much stack does a function call take, and then you can use a heuristic to
make the decision. This isn't 100% secure though (the limited depth approach
isn't too), but it's an option. I would love to ear how other VMs handle the
problem, like the JVM, anyone?

Nuno

Stanislav Malyshev

19 years ago

> For the record the patch I had is this:
> http://mega.ist.utl.pt/~ncpl/zend_stack_protection.txt (it shouldn't
> apply cleanly due to some changes in zend_try some time ago).

Ah, this is different think - it's more like a nice error message, not
preventing stack depletion.

> Knowing in advance if you can recurse or not doesn't sound much
> difficult in theory.. You can get the limit (e.g. with getrlimit) then
> you can know how much stack does a function call take, and then you can

PHP function calls are not the only things that take stack space.

> use a heuristic to make the decision. This isn't 100% secure though (the
> limited depth approach isn't too), but it's an option. I would love to
> ear how other VMs handle the problem, like the JVM, anyone?

Probably they use recursion less (which may be done for Zend engine too,
but that would require some work). Perl in same situation just runs out
of memory.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/

Oliver Block

19 years ago

Am Sonntag, 20. Mai 2007 12:28 schrieb Stefan Esser:

> it is no secret that I am really sick and tired of this constant stream
> of nonsense and
> lies comming out of the mouths of PHP developers when it comes to
> security issues.

What I do not understand than is, why are you doing all this?

You are searching the source code for unsecure usage of C to uncover it.
What drives you?

Another thing: I remember that Zev wrote that it is not a goal yet of the php
group to be safe against local attackers. That is a clear statement.

Let me also say that your "style of writing" does not make it pleasant to read
your postings.

> Xdebug, Suhosin, Hardening Patch

How is the acceptance of the Hardening Patch?

Regards,

Oliver