XSS/SQL inject classics - all in one
-
The contract I'm on at the moment is taking a turn for the worst, security-wise. I've been asked to look over someone else's code (32k lines). I opened the landing page code, what do I find?
$userName = mysql_escape_string($_POST['userName']);
$password = mysql_escape_string($_POST['password']);
if(checkUserLogin($userName, $password)){One would expect sanitization to occur in the function. Right?
Well, wrong. Here's the function:
function checkUserLogin($username, $pass){
//userID, username, password, class, status, frags, online, lastLogin, ...
$SQL = "SELECT * FROM users WHERE username = '$username' AND password = SHA('$pass') AND class !=8 AND class !=9";And believe me, it gets a lot better from here. All of the SQL is more or less of that standard (i.e. no sanitization whatsoever). But when it gets really interesting is the array manipulation.
$trshVar = explode(",", $trashes);
for($x=0; $x < sizeof($trshVar)-1; $x++){Believe it or not, no-one noticed in beta that the last element of the array was not acted upon. All this shit will be fixed (by me), but in the meantime, all you can hope is for the programmer who wrote that last idiocy to go and take basic maths classes (for those of you who watched Divided yesterday, we all know what I'm talking about. For those who don't, what is the sign for "not equal to"? Some people believe it's the greater or equal sign. Those people might end up being programmers, and will end up thinking that acting from 0 to n-1 acts from 0 to n...where n is, of course, already n-1 - due to the strictly less sign)
P.S: Oh, the joys of PHP.
-
The function isn't SQL-injection-proof (which is dumb), but the values are escaped before they are passed in, so SQL injection isn't possible with this code. XSS makes no appearance whatsoever.
@anima said:
P.S: Oh, the joys of PHP.
What does any of this have to do with PHP? Both mistakes are language-agnostic.
-
Yeah... the only WTF here is that the function assumes someone else has done the sanitization. At least sanitization was done.
-
Call me an idiot - I quoted the first occurence of such code and almost expected it not to be escaped. I guess this adds insult to injury, the guy cannot say he wasn't aware of it. It's 1am and I'm quite fed up of reading through this code.
$userID = $_POST['userID'];
$setLevels = $_POST['setLevels'];
//If flash variables were passed, update info, else return error message...
if(isset($_POST['userID']) || isset($_POST['setLevels'])){
if($setLevels != ""){
$levVar = explode(",", $setLevels);
if($levVar[5] == 1){
$SQL1 = "UPDATE users SET class = 2 WHERE userID = $userID"
$result = mysql_query($SQL1) or die("There was an error updating the Levels. Please contact your system administrator.");For the XSS, would you accept something potentially flash-breaking as one? After all, most of the site is in flash. Here goes:
while($Info = get_sql($result)){
//Make Variables of Array for easy access
//userID, username, password, class, status, frags, online, las...
$userID = $Info['userID'];
$username = show_dbHTML($Info['username']);
$class = $Info['class'];
$status = $Info['status'];
$online = $Info['online'];
$lastLogin = $Info['lastLogin'];
$age = $Info['age'];
$genre = $Info['genre'];
$platform = $Info['platform'];
$clanID = $Info['clanID'];
print " <user>\n";
print " <userID>$userID</userID>\n";
print " <username>$username</username>\n";
print " <class>$class</class>\n";
print " <status>$status</status>\n";
print " <online>$online</online>\n";
print " <lastLogin>$lastLogin</lastLogin>\n";
print " <age>$age</age>\n";
print " <genre>$genre</genre>\n";
print " <platform>$platform</platform>\n";
print " <clanID>$clanID</clanID>\n";
print " </user>\n";
Anyone who has worked in making something output XML would know about <![CDATA[ the life-saver. And in case you're wondering:function get_sql($sql_result)
{
global $result;
$sql_rs = mysql_fetch_array($sql_result, MYSQL_ASSOC);
return $sql_rs;
}The global really trumps me, though. Why would anyone global() that?
@morbiuswilters said:
The function isn't SQL-injection-proof (which is dumb), but the values are escaped before they are passed in, so SQL injection isn't possible with this code. XSS makes no appearance whatsoever.
@anima said:
P.S: Oh, the joys of PHP.
What does any of this have to do with PHP? Both mistakes are language-agnostic.
PHP is often coupled, I'll have you know, with bad practice. But I guess most languages are.
-
@anima said:
function get_sql($sql_result)
{
global $result;
$sql_rs = mysql_fetch_array($sql_result, MYSQL_ASSOC);
return $sql_rs;
}The global really trumps me, though. Why would anyone global() that?
$result isn't even used in the function, unless it's an anonymization error. And the whole function is just a poor replacement for mysql_fetch_assoc().
-
@anima said:
Anyone who has worked in making something output XML would know about <![CDATA[ the life-saver.
<![CDATA[ is only a lifesaver if you assume nobody is going to put a ]]> in their input. Better to escape entities.
-
@morbiuswilters said:
@anima said:
function get_sql($sql_result)
{
global $result;
$sql_rs = mysql_fetch_array($sql_result, MYSQL_ASSOC);
return $sql_rs;
}The global really trumps me, though. Why would anyone global() that?
$result isn't even used in the function, unless it's an anonymization error. And the whole function is just a poor replacement for mysql_fetch_assoc().
I did not bother anonymizing, as the thing is pretty much anonymized already (there are no details worth anonymizing in the code I've posted). That's why I said that the global trumps me; there is absolutely no need for it.
snover: assuming you convert brackets to < and >, that is. But since the function in question does neither...
-
You should also use mysql_real_escape_string instead of mysql_escape_string. Else you can still inject due to character encodings.
-
@anima said:
snover: assuming you convert brackets to < and >, that is. But since the function in question does neither...
I’m confused, because I know it doesn’t, and I’m saying that it should, and that you shouldn’t rely on <
