by Daniel Peck, research scientist
Coders get a bum rap about code quality with regard to security. Some of the berating is deserved, like when they try to roll their own crypto algorithms (these people should get the 21st century equivalent of stocks in the public square and rotten fruit pelted at them), but other times it is much more subtle and things that an “end user” coder shouldn’t have to worry about at all.
Success in increasing code quality comes from making it very difficult for a developer to do the wrong thing, making sure that the path of least resistance is also the most correct path. Unfortunately as some programming languages have come to be used as much by designers and artists than the more mathematically included coder of old, a mindset of working around the coder and giving them results that they expect rather than what they’ve asked for has become common. This leads the developers to think they’re doing the right thing, while actually shooting themselves in the foot. A friend of mine (hat tip to @suburbsec) pointed me to a very good example of this the other day on one of spotthevuln.com’s latest entries.
if ( (int) $_REQUEST['w'] && (int) $_REQUEST['h'] ) {
$choice = array(
'type' => "Custom size ({$_REQUEST['w']}x{$_REQUEST['h']})",
'width' => $_REQUEST['w'],
'height' => $_REQUEST['h']
);
}
...
<iframe src="../../../wp-login.php"
width="<?php echo $choice['width']; ?>"
height="<?php echo $choice['height']; ?>"
>your browser does not support iframes.</iframe>
Anyone with a bit of programming knowledge can see that the developer is writing this bit of code with security in mind, testing to make sure that the parameters w and h are indeed string representations of integers before displaying them. Otherwise they wouldn’t cast to an int, right? Wrong. Perfectly valid assumption, but it doesn’t hold true in the land of PHP (a place where black is white, cats and dogs live together, and notions of computational science have no place).
$_REQUEST['w'] and $_REQUEST['h'] still retain the same values as before the int cast, as they should, but if they contain values like “11<bad juju here>” the cast would still return an integer value, 11, and the script is now a funhouse for, admittedly lame, reflected xss. Another interesting side effect of this function is that either of the variables is “0″ – which last time I checked is still an integer. The test fails as a side effect of the test being done in a boolean context and not a type context. In this case, the result is a trivial xss bug but similar snippets can be pulled from many codebases that lead to all sorts of problems with the developers honestly believing they’ve performed all the reasonable steps to ensure input validation. For this particular problem, a much better approach would be to use the is_numeric function for testing or to assign the value of the cast to the variable you’ll be using later, but you’d have a tough time figuring that out by searching for “php string to int”.
It needs to be difficult for the coder to deliver a working product while holding onto false assumptions, and it is up to the languages, frameworks, and development tools to make that more of a reality. Less “more than one way to do it” and more “this is the right way to do it” would go a long way to towards making web security less of a trainwreck than it currently is.
