Archive for January 23, 2007

After getting a little impatient looking at PHP code from however many projects and seeing the typical approach of making input filtering, sql and output escaping the responsibility of the human error-prone developer, I’m now making it standard practice on any of the projects I run to dump this sorry mess. And it is usually a mess.

The fact is you cannot trust a developer to manually secure source code – it’s like God relaying the Ten Commandments to Moses. No matter how many burning bushes, prophets, unnatural disasters, signs and miracles you use to emphasis those ten simple rules, you can still wager there will be a bunch of folk breaking them (including myself!) left, right and centre. It’s human nature to err. It’s my nature to make the verb “err” redundant.

Since the developer is not to be trusted with security (at least not on a detailed manual basis), I have over the months integrated a few simple practices into some of my code, including Zend Framework subclasses where appropriate. I say simple, because when implemented they become automatic and require no developer decision making (i.e. no stupid errors).

Here’s one example.

For HTML output with templates, escape all variable data assigned to the template engine. Why? Because expecting the template author to call that hanging escape() method where requires allows for that author to make a mistake. Mistakes that can introduce security vulnerabilities nobody else might catch before an application is out the door and some scriptkiddie is doing the samba at their PC…

That may sound a bit over the top and strange (so the emails go), but is relatively simple to implement. You just find the template engine’s setter method, for example, the Smarty or Template Lite assign() method, and modify/subclass it to escape all assigned data by default. The default bit is important – force the developer to manually disable it if absolutely required.

If you use the Zend Framework, this type of modification requires subclassing Zend_View with a class containing the following method:

[geshi lang=php]public function __set($key, $val)
{
require_once ‘Zend/View/Exception.php’;
if (‘_’ = substr($key, 0, 1)) {
throw new Zend_View_Exception(‘Setting private or protected class members is not allowed’);
}
if(is_array($val))
{
$clean = array();
foreach ($val as $index => $value)
{
if(is_object($val))
{
throw new Zend_View_Exception(‘Objects may not be directly set as template variables, only arrays of, or standalone, values.’);
}
$clean[$index] = htmlentities($value, ENT_QUOTES, ‘utf-8′);
}
}
else
{
if(is_object($val))
{
throw new Zend_View_Exception(‘Objects may not be directly set as template variables, only arrays of, or standalone, values.’);
}
$clean = htmlentities($val, ENT_QUOTES, ‘utf-8′);
}
$this->$key = $clean;
}[/geshi]

I completely ignored the Zend_View ability to define an escape function as well as the character encoding to use – so lump that in if you wish. The main limitation of the approach is of course that values are escaped under the assumption they are strings, so assigning objects has been made an Exception though after a single dimension that detection will falter – unavoidable with the code above without implementing a recursive check. Personally I see no harm in this – I won’t always have total control over who is writing templates so why should I give them additional rope to hang themselves by handing over objects instead of simple value arrays?

The Exception handling isn’t pretty either, but if subclassing it’s likely the subclass would have its own separate Exception class anyway. It would be nice if the ZF put includes in a predictable placement but the __autoload() debate on the mailing list and other preferences are sometimes pushing Exception file includes to differing places depending on the component.

If special treatment is needed, i.e. some object absolutely must be passed, then a second custom method may be added to handle such special cases. The point of course being to force a template author/developer to deliberately override the default behaviour as needed which presumably means they will think about why they are doing this first.

As for the entry title, Wikipedia has this definition of Poka-Yoke:

Poka-yoke (ポカヨケ – pronounced “POH-kah YOH-keh” means “fail-safing” or “mistake-proofing” — avoiding (yokeru) inadvertent errors (poka)) is a behavior-shaping constraint, or a method of preventing errors by putting limits on how an operation can be performed in order to force the correct completion of the operation. The concept was originated by Shigeo Shingo as part of the Toyota Production System. Originally described as Baka-yoke, but as this means “idiot-proofing” the name was changed to the milder Poka-yoke. One example is the inability to remove a car key from the ignition switch of an automobile if the automatic transmission is not first put in the “Park” position, so that the driver cannot leave the car in an unsafe parking condition where the wheels are not locked against movement. Another example can be found in a normal 3.5″ floppy disk: the top-right corner is shaped in a certain way so that the disk cannot be inserted upside-down.

Edit: Altered code to reflect Zend_View_Abstract no longer extending ArrayObject (I need to update my source after the January bustle!). Thanks to Waldemar Schott for pointing this out. Took a time out to elaborate a bit more as a result too.