Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 2.5.0b5

Bug #18826 Crash and security problem with is_a() in combination with value escaping
Submitted: 2011-09-12 18:55 UTC
From: ogmueller Assigned: doconnor
Status: Closed Package: MDB2 (version 2.5.0b3)
PHP Version: 5.3.7 OS:
Roadmaps: 2.5.0b4    
Subscription  


 [2011-09-12 18:55 UTC] ogmueller (Oliver Mueller)
Description: ------------ Because PHP has changed autoloading in combination with is_a() function, it happens that database values are going to be autoloaded now! This is a very bad behavior. Test script: --------------- In file MDB2/Driver/Datatype/Common.php in method _quoteText (line 1229) $db->escape is called. It returns an escaped DB value (e.g. "Database"). The problem occurs in the line below, where this return value is checked with PEAR::isError(). At the beginning of isError() this value is checked with is_a(), which asks the autoloader to load a class "Database". Expected result: ---------------- Never ask the autoloader to load any DB text as a class. There are basically 2 ways on fixing this. 1. replace is_a() with instanceof, which also makes sense in terms of performance!!! (all is_a() should be replace in whole PEAR!) 2. Do not call return value against isError if it is not an object.

Comments

 [2011-09-12 19:09 UTC] ogmueller (Oliver Mueller)
 [2011-09-18 13:20 UTC] doconnor (Daniel O'Connor)
Areas to look at: MDB2/Driver/Datatype/Common.php 1389: if (isset($db->function) && is_a($db->function, 'MDB2_Driver_Function_Common')) { 1418: if (isset($db->function) && is_a($db->function, 'MDB2_Driver_Function_Common')) { 1447: if (isset($db->function) && is_a($db->function, 'MDB2_Driver_Function_Common')) { MDB2/Driver/Datatype/oci8.php 76: if (is_object($value) && is_a($value, 'OCI-Lob')) { MDB2/Driver/oci8.php 1211: if (is_a($buffer[$key], 'oci-lob')) { 1473: } elseif (is_a($this->values[$parameter], 'OCI-Lob')) { 1487: if (!is_a($this->values[$parameter], 'OCI-Lob')) { 1504: if (!is_a($this->values[$parameter], "OCI-Lob")) { 1520: if (!is_a($this->values[$parameter], "OCI-Lob")) { tests/MDB2_internals_testcase.php 87: $this->assertTrue(is_a($result, 'pear_error'), 'loadClass'); tests/MDB2_usage_testcase.php 1426: if (is_a($lob, 'oci-lob')) {
 [2011-09-18 13:29 UTC] doconnor (Daniel O'Connor)
 [2011-09-18 13:30 UTC] doconnor (Daniel O'Connor)
-Status: Open +Status: Analyzed -Assigned To: +Assigned To: quipo
Here's a patch; untested; but it should work if Alan's thinking is right. Let me know if you just want me to commit it.
 [2011-09-20 15:46 UTC] ogmueller (Oliver Mueller)
Just a note: This patch will work, in my opinion, but it is going to decrease performance even more. "is_a" is very slow compared to instanceof, if you are going to add a "is_object", it will make it worse. It is important to know, that in combination with a class like MDB2, these function might get called easily more than 150.000 times per request! If you do quite a lot of DB transactions or fetching or whatsoever, the performance impact is very noticeable.
 [2011-11-21 17:55 UTC] doconnor (Daniel O'Connor)
-Status: Analyzed +Status: Closed
This bug has been fixed in SVN. If this was a documentation problem, the fix will appear on pear.php.net by the end of next Sunday (CET). If this was a problem with the pear.php.net website, the change should be live shortly. Otherwise, the fix will appear in the package's next release. Thank you for the report and for helping us make PEAR better. Give the version from SVN ago; if the performance regression is severe, feel free to open a new ticket around optimization.
 [2011-11-21 17:56 UTC] doconnor (Daniel O'Connor)
-Assigned To: quipo +Assigned To: doconnor
Committed revision 319612.