RE: [PEAR-DEV] On the use of eval() in XML_Query2XML

Hi, Justin Patrin wrote: > On 10/30/06, Lukas Feiler <lukas@empoweredmail.com> wrote: >> Hi, >> >> recently I stumbled across the EvalForbiddance RFC >> (http://pear.php.net/pepr/pepr-proposal-show.php?id=288). It turns out that >> XML_Query2XML (I'm the lead developer for that package) uses eval() in a >> couple of places: >> >> - >> http://query2xml.sourceforge.net/docs/html/XML_Query2XML/tutorial_XML_Query2 >> XML.pkg.html#query2xml_getxml.options_sql.simplequeryspecification >> - >> http://query2xml.sourceforge.net/docs/html/XML_Query2XML/tutorial_XML_Query2 >> XML.pkg.html#query2xml_getxml.options_condition >> - >> http://query2xml.sourceforge.net/docs/html/XML_Query2XML/tutorial_XML_Query2 >> XML.pkg.html#query2xml_getxml.options_value >> >> Especially in the last two cases the use of eval() really simplifies things >> (IMHO). >> I would not like to make BC breaks but if they are really necessary I would >> prefer this issue being settled before going stable some time in the near >> future. >> > > As was pointed out, this RFC was not accepted, but you need to be very > careful about eval use. It is usually best not to use it as it's a > huge possible vulnerability (PHP injection is a much bigger problem > than SQL injection). You need to put large and easily findable > warnings about which code may be eval'ed so that developers don't let > users enter any text for these options unless absolutely trusted. > > I would also suggest adding an option to turn off eval use entirely > (if you don't already have one) so that developers can turn it off > entirely. > > After taking a look at your code it looks like this package is code > injection just waiting to happen. I would suggest that either you > refactor so as not to need eval or that people never use this package > so that users can enter *any* data in the options affected by eval. > Seriously, it would be far too easy for a code injecton vilnerability > to happen. Some of the options defined by the programmer are directly passed to eval() (as documented). So yes, if the programmer creates an injection vulnerability there, shit's hitting the fan :-) > > Perhaps you should consider allowing callbacks rather than using eval. > It would allow a coder to use their own code for these things without > opening it all up to injection. > > -- > Justin Patrin > Thanks for your input. Currently it is indeed very easy for a developer to create a code injection vulnerability. I'll use callbacks and will get completely rid of eval(). Cheers, Lukas