3

I was using MySQL before, and was told it was unsafe, so now I have recoded my admin login panel in PDO, which users here and other forums said can not be injected. But the hacker is still getting in... I edited the page after the login and told the hacked to tell me what I've put on it and the hacker told me...

I need to know if my code is safe. He is telling me that he is getting in though SQL.

So first I stored their IP in a session so if their IP changes it will log them out (or username)

if ( isset($_SESSION['last_ip']) == false )
{
    $_SESSION['last_ip'] = $_SERVER['REMOTE_ADDR'];
}
if ( $_SESSION['last_ip'] !== $_SERVER['REMOTE_ADDR'] )
{
    session_unset();
    session_destroy();
}

Then here is my login:

session_start();
include 'functions/functions.php';  
$db = mysqlconnect();
$password = md5($_POST['mypassword']);
$mod = 1; 
$statement = $db->prepare("SELECT * FROM users WHERE username = ? AND password = ?");
$statement->execute(array($_POST['myusername'],$password));
$result = $statement->fetchObject()->mod;
$count = $statement->rowCount();
if ( $result == 1 ) {
    $db = mysqlconnect();
    // Register $myusername, $mypassword and redirect to file "login_success.php"
    $_SESSION['user'] = $_POST['myusername'] ;
    //Test if it is a shared client
    if ( !empty($_SERVER['HTTP_CLIENT_IP']) ) {
        $ip=$_SERVER['HTTP_CLIENT_IP'];
        //Is it a proxy address
    } elseif ( !empty($_SERVER['HTTP_X_FORWARDED_FOR']) ) {
        $ip=$_SERVER['HTTP_X_FORWARDED_FOR'];
    } else {
        $ip=$_SERVER['REMOTE_ADDR'];
    }
    $sqll = "UPDATE users SET lastip=? WHERE username=?";
    $q = $db->prepare($sqll);
    $q->execute(array($ip,$_SESSION['username']));
    $_SESSION['user'] = $_POST['myusername'] ;
    $sqlll = "INSERT INTO user_log (username,ip) VALUES (?, ?)";
    $qq = $db->prepare($sqlll);
    $qq->execute(array($_SESSION['username'],$ip));
    header("Location: home.php");
} else {
    echo "Wrong Username or Password";
}

Can the code be injected ?

And this is my home.php page which stops users from viewing it.

/// My conenct is here                    
$sql = "SELECT * FROM users WHERE username='$_SESSION[user]'";
$result = mysql_query($sql) or die(mysql_error());
$values = mysql_fetch_array($result);


if( isset($_SESSION['user']) ) {

} else {
    echo "Bye Bye";
    die;
}

if ( $values['mod'] == 1 ) {    
    echo "welcome";  
} else {
    echo"Your account has been reported for hacking";
    die;
}
tereško
  • 58,060
  • 25
  • 98
  • 150
user1405062
  • 85
  • 1
  • 1
  • 8
  • 1
    You should validate that `$_POST['myusername']` is formatted properly before using it in SQL. A simple `preg_match()` should be sufficient, or you could escape it to another variable, then search for the variable. Also, where does `$_SERVER['HTTP_CLIENT_IP']` come from? – ghoti May 26 '12 at 11:05
  • I thought in pdo you don't need to format variables ?? – user1405062 May 26 '12 at 11:09
  • What is the hacker doing? Why do you say "he's getting in", and why do you think it's SQL injection? – Robbie May 26 '12 at 11:15
  • 1
    If the hacker is already in, how do you know this is the code that actually gets executed? – tripleee May 26 '12 at 11:17
  • The hacker told me its sql injection. And My ip code which ive posted means if he is inactive or changes his ip it will log him out. When the user logs in it stores the users ip and username in the db. So i know hes signing in because the amount of data and different ips from that user. And when i say getting in i mean the login redirects tot he home.php if the user is a mod i write testing on the home.php page and ask the hacker to tell me what ive wrote and the hacker will tell me what i have wrote on the page after the login script – user1405062 May 26 '12 at 11:19
  • have you salted and hashed the password? – Daniel A. White May 26 '12 at 11:45
  • And what's in home.php to verify the season? – Robbie May 26 '12 at 11:54
  • I have edited the post with the home.php source – user1405062 May 26 '12 at 12:35
  • 1
    You shouldn't trust the proxy/forwarded $_SERVER variables. Those are client-provided data and trivial to forge. The only client IP valie that's reliable is `$_SERVER['REMOTE_ADDR']`, which is determined by php, NOT the user. – Marc B May 26 '12 at 13:02
  • Marc's right, but those variables are not being used to verify the session, so it's ok, as far as I can see. User1405062, where do you clear the user's session? Sorry for all this, but i'm trying to track the"hacker's" flow, a it's still not obviously SQL injection. Have you cleared his session (100% guarantee it's cleared) as that IP code d doesn't mean it's been cleared since his last login? – Robbie May 26 '12 at 13:27
  • I clear the session on the logout.php page..... – user1405062 May 26 '12 at 14:06
  • And he logged out? Just checking he's not taking you for a ride - but right now, there's nothing there that's obviously hackable except session fixation (which is what I'm asking about) or session ID theft (which you're trying to rule out with the IP). – Robbie May 26 '12 at 14:18
  • Actually - have seen something that may confuse: $q->execute(array($ip,$_SESSION['username'])); should be $q->execute(array($ip,$_SESSION['user'])); - although that's not going to let him log in, it may mess with your tracking a wee bit. – Robbie May 26 '12 at 14:20
  • So whats best to do is make it so when he moved page destroy the session ? So like Add to the config page which is inculed onto every page session destroy then ask him to tell me what it says ont he page again when he refreshes the page it will log him out ?? – user1405062 May 26 '12 at 14:21
  • Use the "accepted answer" here: http://stackoverflow.com/questions/508959/truly-destroying-a-php-session i.e. For now, create yourself a NEW session variable in your login script. If the user arrives at "home.php" without that NEW session variable, send him to the logout script. (This ensures that old users are all ogged out and will need to log in to your new system.) Also check login (that same link has the code too). Then ask the hacker to go for it. If he's still getting in, I think we'll need more code to check the full flow of events. – Robbie May 26 '12 at 14:42
  • I logged every staff login and found out he was login though another staff members account – user1405062 May 26 '12 at 15:01
  • possible duplicate of [Best way to prevent SQL Injection in PHP](http://stackoverflow.com/questions/60174/best-way-to-prevent-sql-injection-in-php) – PeeHaa May 26 '12 at 15:24
  • Why are you still using `md5()` as I already asked you on [your previous question](http://stackoverflow.com/questions/10762591/php-simple-login-script-white-page). And why are you still mixing mysql_* and PDO as ohters have stated on that question? – PeeHaa May 26 '12 at 15:27
  • What `$mod = 1;` do? Sure the user don't access http://mywebsite.com/?mod=1 and you allow access? – Gabriel Santos May 26 '12 at 22:18
  • @RepWhoringPeeHaa Where he is using `mysql_*`?. EDIT: Sorry, have not read the edit. – Gabriel Santos May 26 '12 at 22:19
  • @user1405062 Probably he is doing session poisoning. I am sure the vilnerable file is `home.php`. – Gabriel Santos May 26 '12 at 22:26
  • i would suggest to use session_set_save_handler() to customize your session handling , also missing some quoting before executing your queries to database and filtering the POST values before passing into sessions. Make sure the *hacker hasn't already have a user credentials from another user into website already ask users to update their passwords, if 1 user has been **attacked** by the hacker his credentials might have been exposed to hacker allowing access to user website at moderator level privilege – Gntem May 28 '12 at 07:54
  • I know a session ID can be stolen, if that helps somehow.. – eric.itzhak May 28 '12 at 12:59
  • PDO can still be injected just as easily as with mysql. It comes down ENTIRELY to how you construct the query strings. – Marc B May 28 '12 at 20:47

2 Answers2

1

Assuming the queries you show are the only queries you use, it's funny how the hacker uses the very code you thought would block him. Very often in the IT world the front door is triple locked but the backdoor is open. In this case it's not the backdoor, but the little window in the door that you use to see if you are not opening to a crook.

Use prepared statements and variable binding on all queries.

That is the general rule. You just forgot the "all" in "all queries".

Your statement:

$sql = "SELECT * FROM users WHERE username='$_SESSION[user]'";

is your open window. Quite obviously your hacker spoofed the username in the session data. The simple solution is to:

  1. validate the username or any data you receive from outside ($_SESSION, $_POST, some $_SERVER and others) BEFORE you do anything with it. Certainly before you perform any database interaction.
  2. use variable binding like you did in the other queries.

Removing tags or using mysql_real_escape_string is of less value in this case because the hack is perfectly possible without any unstorable characters.

By the way you don't really need to use PDO to use variable binding. It is perfectly possible in mysql too. 

$verbLok = mysqli_connect(Iam, not, gonna, tellyou);
$result = array();
if (($stmtLok = $verbLok->prepare( "SELECT * FROM users WHERE username = ?  limit 1")))
{        
    $stmtLok->bind_param("s", $_SESSION[user]);
    $stmtLok->execute();
    $result = $stmtLok->get_result();
    $stmtLok->close();
}
mysqli_close($verbLok);

should do the trick for you. $result will hold an array of result rows. I am not too familiar with conventional queries anymore, but as far as I remember it's similar to your code.

Interesting reading on how a SQL code injection attack is possible on your query can be found on this page.

Hans Hokke
  • 11
  • 2
-1

A good SQL Injection prevention would be to escape any and all text that could be used to terminate the SQL command mid-evaluation to start a new command.

To do that you should escape any quotes in the input, and use better SQL — use backticks around fields and table names, use quotes to wrap any non-code text, etc.

SELECT * FROM `users` WHERE `id` = '1'

Then escape all quotes with mysql_real_escape_string, or by manual replacements with #039; etc, also to be sure escape any HTML to make sure it doesn't mess up other things, use htmlentities etc.

casraf
  • 21,085
  • 9
  • 56
  • 91
  • 3
    The point of PDO is so you don't have to do this. Your write your query with _placeholders_, prepare it, then execute it with data passed in, which is escaped automatically by the PDO driver. – Bojangles May 28 '12 at 20:54
  • you dont need to use pdo if you just use mysql_real_escape_string, ` and ' and some type checking on the data you expect. also using brackets ( ) around parts of your query helps. check if the returned result is what you want, if not, die() – Tschallacka May 28 '12 at 21:45
  • On my own SQL framework I just escape the data before I ever put it in the database with about 3-4 different methods. I always check my sites for SQL Injection and they never work. – casraf May 29 '12 at 01:15
  • Escaping with 3-4 different methods is not necessary. Either escape with the method provided by the mysql API, or else use parameters. No other method of escaping is needed. – Bill Karwin Sep 17 '13 at 00:03
  • Placeholders are easier, more efficient, and are actually being maintained. – Colonel Thirty Two Sep 17 '13 at 00:38