On Oct 17, 2008, at 1:58 PM, Lamp Lists wrote:
I'm reading "Essential PHP Security" by Chris Shiflett.
on the very beginning, page 5 & 6, if I got it correct, he said this
is not good:
$search = isset($_GET['search']) ? $_GET['search'] : '';
and this is good:
$search = '';
if (isset($_GET['search']))
{
$search = $_GET['search'];
}
what's the difference? I really can't see?
I believe I was trying to emphasize how simple, obvious code can be a
boon to security. I'm sure I could have picked a better example, but
let me show you a line of code I noticed in a security audit just
yesterday (only the variable name has been changed to be generic):
$host = strlen($host) > 0 ? $host : htmlentities($host);
We have developed tools to help us find things like this, but imagine
you're manually reviewing a colleague's code, and you're looking
through a few thousand lines to try to help identify security problems.
In this particular example, my first thought was to suggest specifying
the character encoding when using htmlentities(), and making sure this
matches the Content-Type header, to avoid things like this:
http://shiflett.org/blog/2005/dec/google-xss-example
You might also be distracted by the comparison of strlen() to 0, since
it seems like you could simply rely on a boolean evaluation of
strlen() instead.
Can you spot the bigger problem?
The order is reversed, so if $host has a non-zero length, it is not
escaped.
When spending mere seconds per line, on average, reviewing a lot of
code, this is exactly the sort of thing that's not that hard to miss.
The real question is whether it would be slightly harder to miss if
expanded:
if (strlen($host) > 0) {
$host = $host;
} else {
$host = htmlentities($host);
}
I think it's much less likely to be overlooked when written like this,
and this is the sort of decision that many developers take for
granted. If you're too proud to admit that the ternary is less
obvious, or too proud to admit that you could ever make a mistake like
this, maybe you can at least convince yourself that not everyone is as
clever as you, and code that is easier to review is ultimately going
to be better code.
Hope that helps,
Chris
--
Chris Shiflett
http://shiflett.org/
--
PHP General Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
