[RFC] Deprecate and remove /e modifier from preg_replace

php.internals

Nikita Popov

14 years ago
Hi internals! I have written an RFC that proposes to *deprecate* and *remove* the /e modifier: https://wiki.php.net/rfc/remove_preg_replace_eval_modifier Comments welcome!

Tom Boutell

14 years ago
I think this is a good idea, since nobody's keeping you from calling eval() in a callback if you really really really need to, there's no loss of functionality, just the discouragement of an unhealthy "worst practice." (: On Sun, Feb 5, 2012 at 9:59 AM, Nikita Popov <nikita.ppv@googlemail.com> wrote:
> Hi internals! > > I have written an RFC that proposes to *deprecate* and *remove* the /e modifier: > > https://wiki.php.net/rfc/remove_preg_replace_eval_modifier > > Comments welcome! > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php >
-- Tom Boutell P'unk Avenue 215 755 1330 punkave.com window.punkave.com

Pierre Joye

14 years ago
hi, I think we should remove eval at the same time then. As the risk is exactly the same in both situations. Eval is just as evil and can be avoided as well (or any other similar features, not sure if other exts allow that). Cheers, On Sun, Feb 5, 2012 at 3:59 PM, Nikita Popov <nikita.ppv@googlemail.com> wrote:
> Hi internals! > > I have written an RFC that proposes to *deprecate* and *remove* the /e modifier: > > https://wiki.php.net/rfc/remove_preg_replace_eval_modifier > > Comments welcome! > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php >
-- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org

Tom Boutell

14 years ago
I know a straw man argument when I see one (: /e suggests that eval is a casual thing to be sprinkled into typical search and replace operations. It also suggests that putting PHP code in a quoted string is probably the easy way to solve your problem. It's often discovered before the developer has even heard of preg_replace_callback. eval() does not have these PR problems (: It's there for those who are truly looking for it. On Sun, Feb 5, 2012 at 11:21 AM, Pierre Joye <pierre.php@gmail.com> wrote:
> hi, > > I think we should remove eval at the same time then. As the risk is > exactly the same in both situations. Eval is just as evil and can be > avoided as well (or any other similar features, not sure if other exts > allow that). > > Cheers, > > On Sun, Feb 5, 2012 at 3:59 PM, Nikita Popov <nikita.ppv@googlemail.com> wrote: >> Hi internals! >> >> I have written an RFC that proposes to *deprecate* and *remove* the /e modifier: >> >> https://wiki.php.net/rfc/remove_preg_replace_eval_modifier >> >> Comments welcome! >> >> -- >> PHP Internals - PHP Runtime Development Mailing List >> To unsubscribe, visit: http://www.php.net/unsub.php >> > > > > -- > Pierre > > @pierrejoye | http://blog.thepimp.net | http://www.libgd.org > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php >
-- Tom Boutell P'unk Avenue 215 755 1330 punkave.com window.punkave.com

Reindl Harald

14 years ago
what he hell - if you kill eval you would kill the whole work of my life and yes i know that eval is evil and it is only used at one place which is a central and real important to include modules and set parameters dynamically the /e modifier is a total other dimension because it can be used by people not knowing what they are doing exactly by C&P any code snippet eval() is a documentated function Am 05.02.2012 17:21, schrieb Pierre Joye:

Tom Boutell

14 years ago
A sense of humor is important when reading mailing lists frequented by extremely clever people (: On Sun, Feb 5, 2012 at 11:34 AM, Reindl Harald <h.reindl@thelounge.net> wrote:
> what he hell - if you kill eval you would kill the whole > work of my life and yes i know that eval is evil and > it is only used at one place which is a central and > real important to include modules and set parameters > dynamically > > the /e modifier is a total other dimension because it can > be used by people not knowing what they are doing exactly > by C&P any code snippet > > eval() is a documentated function > > Am 05.02.2012 17:21, schrieb Pierre Joye: >> I think we should remove eval at the same time then. As the risk is >> exactly the same in both situations. Eval is just as evil and can be >> avoided as well (or any other similar features, not sure if other exts >> allow that). >> >> Cheers, >> >> On Sun, Feb 5, 2012 at 3:59 PM, Nikita Popov <nikita.ppv@googlemail.com> wrote: >>> Hi internals! >>> >>> I have written an RFC that proposes to *deprecate* and *remove* the /e modifier: >>> >>> https://wiki.php.net/rfc/remove_preg_replace_eval_modifier >>> >>> Comments welcome! >
-- Tom Boutell P'unk Avenue 215 755 1330 punkave.com window.punkave.com

Reindl Harald

14 years ago
i did not see any smiley and without it is hard to smell remove /e makes sense over the long because it is really dangerous to get wrong used with user-input by pepole C&P reg-expressions from somewehre without exactly understand what they are doing and that they can trigger remote-code execution form anonymous requests the places where i use eval will never see any user-input these are different worlds Am 05.02.2012 17:37, schrieb Tom Boutell:

Mike Stowe

14 years ago
Perhaps rather than killing it we should just emphasize it more in the documentation (ie on preg_replace and not only in pattern modifiers). But I have found the /e modifier to be very useful in the past. Unfortunately, just having woken up I can't remember exactly where and how I used it, lol. Perhaps another option, if it's a security concern is the ability to turn off the /e modifier, and have it off by default. This way we can protect our less experienced programmers, while keeping it available for more advanced use cases. Just my two cents + inflation - Mike Sent from my iPhone On Feb 5, 2012, at 10:37 AM, Tom Boutell <tom@punkave.com> wrote:

Reindl Harald

14 years ago
Am 05.02.2012 17:45, schrieb Michael Stowe:
> Perhaps another option, if it's a security concern is the ability > to turn off the /e modifier, and have it off by default. This way > we can protect our less experienced programmers, while keeping it > available for more advanced use cases. > > Just my two cents + inflation
+1 for secure/sane defaults! it is possible with suhosin since years suhosin.executor.disable_emodifier = Off

Nikita Popov

14 years ago
On Sun, Feb 5, 2012 at 5:45 PM, Michael Stowe <mikegstowe@gmail.com> wrote: [snip]
> Perhaps another option, if it's a security concern is the ability to turn off the /e modifier, and have it off by default. This way we can protect our less experienced programmers, while keeping it available for more advanced use cases.
I think introducing an option for this will only create problems. Code using /e will be non-portable as it depends on the ini option being enabled. Also this way shared hosting will never disabled the modifier because it doesn't want to break apps. And I think disabling it is especially important for people on shared hosting, who usually are less educated about security than people on dedicated servers. Also: If you really want to use /e you can still call eval() inside preg_replace_callback. This additionally has the benefit of making the code evaluation more explicit. Nikita

Reindl Harald

14 years ago
Am 05.02.2012 18:09, schrieb Nikita Popov:
> On Sun, Feb 5, 2012 at 5:45 PM, Michael Stowe <mikegstowe@gmail.com> wrote: > [snip] >> Perhaps another option, if it's a security concern is the ability to turn off the /e modifier, and have it off by default. This way we can protect our less experienced programmers, while keeping it available for more advanced use cases. > > I think introducing an option for this will only create problems. Code > using /e will be non-portable as it depends on the ini option being > enabled.
yes, and security problematic things hsould only be enbaled active
> Also this way shared hosting will never disabled the modifier > because it doesn't want to break apps.
the one who cares security will do it
> And I think disabling it is especially important for people on shared hosting, > who usually are less educated about security than people on dedicated servers.
but the one on dedicated servers currently have no option to make their setup secure without suhosin
> Also: If you really want to use /e you can still call eval() inside > preg_replace_callback. This additionally has the benefit of making the > code evaluation more explicit.
the problem is "you can" if it is default off you should do it this way if you like portable apps

Pierre Joye

14 years ago
hi, On Sun, Feb 5, 2012 at 5:45 PM, Michael Stowe <mikegstowe@gmail.com> wrote:
> Perhaps another option, if it's a security concern is the ability to turn off the /e modifier, and have it off by default. This way we can protect our less experienced programmers, while keeping it available for more advanced use cases.
That sounds like a nicer approach and it is actually one of the RFC I like to see to bring some of the features of Suhosin in PHP (disable eval and the e modifier). Cheers,
-- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org

Stas Malyshev

14 years ago
Hi!
> That sounds like a nicer approach and it is actually one of the RFC I > like to see to bring some of the features of Suhosin in PHP (disable > eval and the e modifier).
Disbaling eval() makes little sense to me - nobody accidentally writes an eval() and if you execute third-party code there's dozens of ways to do the same thing as eval() does. The /e case though seems much stronger, as one could legitimately write preg_replace() which uses /e and securing it is a non-trivial task since you basically inject third-party code into your context (like SQL injection only worse since SQL doesn't have vars in strings :). So given we have preg_replace_callback, phasing out /e starting 5.5 would probably make sense.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227

Derick Rethans

14 years ago
On Sun, 5 Feb 2012, Tom Boutell wrote:
> A sense of humor is important when reading mailing lists frequented by > extremely clever people (:
Sarcasm doesn't work on mailinglists, so don't use it. Derick
-- http://derickrethans.nl | http://xdebug.org Like Xdebug? Consider a donation: http://xdebug.org/donate.php twitter: @derickr and @xdebug

Ferenc Kovacs

14 years ago
On Sun, Feb 5, 2012 at 5:21 PM, Pierre Joye <pierre.php@gmail.com> wrote:
> hi, > > I think we should remove eval at the same time then. As the risk is > exactly the same in both situations. Eval is just as evil and can be > avoided as well (or any other similar features, not sure if other exts > allow that). > > Cheers, > >
https://wiki.php.net/rfc/remove_preg_replace_eval_modifier#but_we_don_t_remove_eval_either_do_we I think it would be very unwise to start bringing up that here, when the RFC author explicitly stated that his RFC is not about removing eval. From my POV, the problem with /e is that it is easy miss the fact that using unfiltered user input in your preg_replace call can lead to arbitrary code execution. With eval() you explicitly state that you want this, and if you miss the security implications of your actions, you can blame nobody but yourself. Another difference is that as the RFC states, the /e modifier can be replaced easily with a callback, which would make the code even more clear, so it is an even better alternative. On the other hand, eval doesn't have such an easy alternative, you would have to generate and save the code and include it, which has the same security implications but it is much more tendersome, and it would also open up potential security implications (another process overwriting your temp file would cause arbitrary code execution for example, so one would need to use hard-to-guess filenames and/or exclusive locking, or using custom streams, etc.). To summarize /e has less use cases than eval and it has an alternative which provides better code, which isn't true for eval.
-- Ferenc Kovács @Tyr43l - http://tyrael.hu

Derick Rethans

14 years ago
On Sun, 5 Feb 2012, Nikita Popov wrote:
> I have written an RFC that proposes to *deprecate* and *remove* the /e modifier: > > https://wiki.php.net/rfc/remove_preg_replace_eval_modifier > > Comments welcome!
This RFC makes no sense. It says: For example the above example can be used to execute arbitrary PHP code by passing the string <h1>{${eval($_GET[php_code])}}</h1>. The evaluted code in this case would be "<h1>" . strtoupper("{${eval($_GET[php_code])}}") . "</h1>" and as such execute any PHP code passed in the php_code GET variable. If you don't sanitize your imput than all sorts of intesting things can't happen. You're going to inconvenience a lot of people by removing it. So, definitely against removing features from a language with no real win. cheers, Derick
-- http://derickrethans.nl | http://xdebug.org Like Xdebug? Consider a donation: http://xdebug.org/donate.php twitter: @derickr and @xdebug

John Crenshaw

14 years ago
> -----Original Message----- > From: Derick Rethans [mailto:derick@php.net] > Sent: Sunday, February 05, 2012 11:46 AM > To: Nikita Popov > Cc: PHP internals > Subject: Re: [PHP-DEV] [RFC] Deprecate and remove /e modifier from preg_replace > > On Sun, 5 Feb 2012, Nikita Popov wrote: > > > I have written an RFC that proposes to *deprecate* and *remove* the /e modifier: > > > > https://wiki.php.net/rfc/remove_preg_replace_eval_modifier > > > > Comments welcome! > > This RFC makes no sense. It says: > > For example the above example can be used to execute arbitrary PHP code by passing the string <h1>{${eval($_GET[php_code])}}</h1>. The evaluted code in this case would be "<h1>" .
strtoupper("{${eval($_GET[php_code])}}") . "</h1>" and as such execute any PHP code passed in the php_code GET variable.
> > If you don't sanitize your imput than all sorts of intesting things can't happen. You're going to inconvenience a lot of people by removing it. > > So, definitely against removing features from a language with no real win. > > cheers, > Derick
Normally I'd totally agree with not removing stuff, but in this case the RFC makes a critical error which serves to demonstrate exactly how bad the problem is. The author incorrectly used double quotes in their replace string, when the only safe solution is to use single quotes. This is a super common mistake with /e, and even many veterans won't notice it (although they'll probably notice the use of /e). Removing this would obviously be an inconvenience for some people, but getting your server hacked is also an inconvenience, and hackers don't give you nice warnings with file and line number. I like the idea of doing _something_ here. Deprecate now and remove later sounds fair. John Crenshaw Priacta, Inc.

Stas Malyshev

14 years ago
Hi!
> Removing this would obviously be an inconvenience for some people, > but getting your server hacked is also an inconvenience, and hackers > don't give you nice warnings with file and line number. I like the > idea of doing _something_ here. Deprecate now and remove later sounds > fair.
Inconvenience is fairly minimal - you can easily rewrite it with preg_replace_callback, which given we have now anon functions is no more verbose, much easier to understand and has no security problems whatsoever, seems to be a win. The problem with /e is not people that do not sanitize, the problem is that sanitizing properly is not easy and people regularly mess it up and don't even know it. Yes, that means that some code will have to be patched - but it can be easily done, even in backwards-compatible way since preg_replace_callback is available since forever. And the resulting code will also be faster (though probably it wouldn't matter :)
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227

Tom Boutell

14 years ago
Deprecate and then remove, or just leave it in. "Make it optional forever" just generates more nonportable PHP code. Ick. On Sun, Feb 5, 2012 at 3:28 PM, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> Hi! > > >> Removing this would obviously be an inconvenience for some people, >> but getting your server hacked is also an inconvenience, and hackers >> don't give you nice warnings with file and line number. I like the >> idea of doing _something_ here. Deprecate now and remove later sounds >> fair. > > > Inconvenience is fairly minimal - you can easily rewrite it with > preg_replace_callback, which given we have now anon functions is no more > verbose, much easier to understand and has no security problems whatsoever, > seems to be a win. The problem with /e is not people that do not sanitize, > the problem is that sanitizing properly is not easy and people regularly > mess it up and don't even know it. > Yes, that means that some code will have to be patched - but it can be > easily done, even in backwards-compatible way since preg_replace_callback is > available since forever. And the resulting code will also be faster (though > probably it wouldn't matter :) > > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php >
-- Tom Boutell P'unk Avenue 215 755 1330 punkave.com window.punkave.com

Adam Harvey

14 years ago
On 6 February 2012 04:37, Tom Boutell <tom@punkave.com> wrote:
> Deprecate and then remove, or just leave it in. "Make it optional > forever" just generates more nonportable PHP code. Ick.
Huge +1 to that. Given the existence of preg_replace_callback() (as Stas noted above), my preference is for deprecation and then removal, but I think the overriding goal should be not introducing any more configuration dependent behaviour — I'd rather be actively trying to kill some of that than ushering more in. Adam

Gwynne Raskind

14 years ago
On Sun, Feb 5, 2012 at 19:32, Adam Harvey <aharvey@php.net> wrote:
> On 6 February 2012 04:37, Tom Boutell <tom@punkave.com> wrote: >> Deprecate and then remove, or just leave it in. "Make it optional >> forever" just generates more nonportable PHP code. Ick. > Huge +1 to that. > Given the existence of preg_replace_callback() (as Stas noted above), > my preference is for deprecation and then removal, but I think the > overriding goal should be not introducing any more configuration > dependent behaviour — I'd rather be actively trying to kill some of > that than ushering more in.
Strong +1 from me as well. Deprecate, remove. Do NOT get Suhoshin involved in this. It's not. -- Gwynne

Patrick ALLAERT

14 years ago
+1: It does make sense to me for the same reason as Stas mentioned.

Richard Lynch

14 years ago
A couple people I respect a heck of a lot have voted against, but I've heard no technical explanation of "why" from them... I voted "Yes" because I've never found a need for /e at all, personally. Not sure my vote even counts, so feel free to nuke it. :-) Or maybe I'm just not smart enough to employ it, but un-wind the text and use more PHP code to get the job done... Anyway, I'd like to hear reasoned thoughts on "No" votes.
-- brain cancer update: http://richardlynch.blogspot.com/search/label/brain%20tumor Donate: https://www.paypal.com/cgi-bin/webscr?cmd=_s-xclick&hosted_button_id=FS9NLTNEEKWBE