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 &lt; and &gt, 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 &lt; and &gt, 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 <![CDATA[ as a magic bullet to allow for unescaped entities in your XML output; if you are serving untrusted data, you should always, always escape the entities.

     

    @Daid said:

    You should also use mysql_real_escape_string instead of mysql_escape_string. Else you can still inject due to character encodings.
    No. NO. Please, use prepared statements. There is absolutely no reason you should still be concatenating strings that include potentially unsafe data to form an SQL query, regardless of whether or not you are using some magical string escaping method.



  • I'm working on an inherited CMS which makes similar assumptions -- i.e. that all data will be escaped using magic_quotes_gpc (shudder).

    Of course (character encoding and other vulnerabilities aside), that assumption breaks down when the developer retrieves the data from a session instead of $_GET, $_POST, or directly from a cookie.

    The convoluted fallout was that users with names like O'Hara got error messages when they tried to make purchases, BUT the purchases were recorded, BUT they weren't charged, BUT their account balances remained at 0.  Oh, and then when the purchases were automatically cancelled because no payments had been made, the accounts were credited for the purchase value, leaving users with spurious store credit in their accounts.

    Yes, this one's in PHP too.



  • @snover said:

    No. NO. Please, use prepared statements. There is absolutely no reason you should still be concatenating strings that include potentially unsafe data to form an SQL query, regardless of whether or not you are using some magical string escaping method.
     

    *nods



  • I've seen worse than that.

    Actual sql queries stored in a sql table, concatenated in php through a filtered php array of corresponding values. 

    Prepared queries? Bah! Stored procedures! Bah! Sanitization? Magic quotes FTW!

    And of course the mantra was "php suxorz, we all agree"


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.