Re: On the use of eval() in XML_Query2XML

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. 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