Login Security

[dreamkat]

Is this secure enough or is it hackable?
Oh and the player id== 11 thing, is because account 11 is a test account.

if($loggedin == '0')
{
if(isset($_POST['submit']))
{

if((!isset($_POST['login'])) || (!isset($_POST['password']))
|| ($_POST['login'] == '') || ($_POST['password'] == '')){
$error++;
echo "<font size='2'>Please fill out the form completely.</font><br>";
}
//user data
$player = @mysql_query("SELECT * FROM `players` WHERE `loginuser` = '".$_POST['login']."'");
$player = @mysql_fetch_assoc($player);

if($player['loginuser'] == false){
$error++;
echo "<font size='2'>User does not exist!<br></font>";
}
else if($player['password'] != md5($_POST['password'])){
echo "<font size='2'>Wrong password!</font><br>";
$error++;
}
//if there is no errors continue
elseif($error==0){
$_SESSION['id'] = $player['id'];
$_SESSION['loginuser'] = $player['loginuser'];
$_SESSION['username'] = $player['username'];
$_SESSION['password'] = $player['password'];
$_SESSION['level'] = $player['level'];
$_SESSION['avatar'] = $player['avatar'];
$_SESSION['money'] = $player['money'];
$_SESSION['badip'] = $player['badip'];
date_default_timezone_set('US/Pacific');
$date = date("M d, Y h:i a");

$ip = $_SERVER['REMOTE_ADDR'];

$update = @mysql_query("UPDATE `players` SET `lastlogin` = '$date' WHERE `id` = '".$_SESSION['id']."'");

$updateip = @mysql_query("UPDATE `players` SET `lastip` = '$ip' WHERE `id` = '".$_SESSION['id']."'");

echo 'You are now logged in!';

if($_SESSION['ip']!=$player['lastip']&&$_SESSION['id']!='11'){

mysql_query("UPDATE `players` SET `ipsaves`='1' WHERE 
`id`='{$player['id']}'") or die();

}
if($_SESSION['ipsaves']>0 && $_SESSION['ip']==$_SESSION['lastip']&&$_SESSION['ip']!=''&&$_SESSION['id']!='11') {
$from = 1;
$to = $player['id'];
$title = "Account Security";
$content = "
Alot of text about security was here, I just want to keep it short for you guys ;)
-Automatic message
";
mysql_query("INSERT INTO messages (from_user, to_user, message_title, message_contents, message_date) VALUES
 ('$from','$to','$title','$content')");


}

}
}
else
{
echo '
<p>Not logged in. <a href=join.php>Register?</A></p>
 <p> or forgot password?</p><bR>
<form action=index.php method=post>
<font style=font-size:12px>Login Name
<br>
<input type=text class=lg name=login><br>
<P>Password</font>
<input type=password class=lg name=password><p>
<br><button type="submit" name="submit" value="Submit">
<img src="obutton.png"/>
</button>
</form>';
}
}
elseif($loggedin==1)
{
//Nothing special..just alot of text and user information
}

I'm new to security in PHP, so any input would be great.

[laserlight]

You should indent your code properly.

What is $loggedin? It looks like you are using it without defining it. (I am assuming that register_globals is off.) Also, it looks like you did not account for SQL injection.

[dreamkat]

ah, It seems I cant edit my post, sorry ^; I'm new to this.
register_globals are off.
Hm, no I dont know much about how they hack,
Would I prevent it by using

mysql_real_escape_string($_POST['login']),
mysql_real_escape_string($_POST['password']));

Well $loggedin is apart of config.php, This is the piece of it.

//If there is no session
if((!isset($_SESSION['loginuser'])) || (!isset($_SESSION['id'])) || (!isset($_SESSION['username'])) || (!isset($_SESSION['password']))
|| (!isset($_SESSION['avatar'])) || (!isset($_SESSION['level'])) || (!isset($_SESSION['money']))){

unset($_SESSION['loginuser']);
unset($_SESSION['username']);
unset($_SESSION['password']);
unset($_SESSION['avatar']);
unset($_SESSION['level']);
unset($_SESSION['money']);
//not logged in
$loggedin = 0;
}
else
{
//logged in
$loggedin = 1;
}

//logged in is to check if user is logged in
//0 means not logged in, 1 means they are
if($loggedin == '0')
{
    if(isset($_POST['submit']))
    {
        if((!isset($_POST['login'])) || (!isset($_POST['password']))
        || ($_POST['login'] == '') || ($_POST['password'] == '')){
            $error++;
            echo "<font size='2'>Please fill out the form completely.</font><br>";
        }

    //Get user data, to check if the login matches.

    $player = @mysql_query("SELECT * FROM `players` WHERE `loginuser` = '".$_POST['login']."'");
    $player = @mysql_fetch_assoc($player);

//loginuser means the username they login with

    if($player['loginuser'] == false){
        $error++;
        echo "<font size='2'>User does not exist!<br></font>";
    }
    else if($player['password'] != md5($_POST['password'])){
        echo "<font size='2'>Wrong password!</font><br>";
        $error++;
    }

    //if there is no errors continue

    elseif($error==0){

//Sessions
            $_SESSION['id'] = $player['id'];
            $_SESSION['loginuser'] = $player['loginuser'];
            $_SESSION['username'] = $player['username'];
            $_SESSION['password'] = $player['password'];
            $_SESSION['level'] = $player['level'];
            $_SESSION['avatar'] = $player['avatar'];
            $_SESSION['money'] = $player['money'];
            $_SESSION['badip'] = $player['badip'];

        date_default_timezone_set('US/Pacific');
        $date = date("M d, Y h:i a");
        $ip = $_SERVER['REMOTE_ADDR'];

//Updating users last login and ip

        $update = @mysql_query("UPDATE `players` SET `lastlogin` = '$date' WHERE `id` = '".$_SESSION['id']."'");
        $updateip = @mysql_query("UPDATE `players` SET `lastip` = '$ip' WHERE `id` = '".$_SESSION['id']."'");

//They are now logged in

        echo 'You are now logged in!';
        if($_SESSION['ip']!=$player['lastip']&&$_SESSION['id']!='11'){
            mysql_query("UPDATE `players` SET `ipsaves`='1' WHERE
            `id`='{
                $player['id']
            }
            '") or die();
        }
        if($_SESSION['ipsaves']>0 &&
       $_SESSION['ip']==$_SESSION['lastip'] && $_SESSION['ip']!='') {
//1 is the id of the owner(me)
                $from = 1;
                $to = $player['id'];
                $title = "Account Security";
                $content = "
                Alot of text about security was here, I just want to keep it short for you guys ;
                )
                -Automatic message
                ";
                mysql_query("INSERT INTO messages (from_user, to_user, message_title, message_contents, message_date) VALUES
                ('$from','$to','$title','$content')");
            }
        }
    }
    else
    {
        echo '
        <p>Not logged in. <a href=join.php>Register?</A></p>
        <p> or forgot password?</p><bR>
        <form action=index.php method=post>
        <font style=font-size:12px>Login Name
        <br>
        <input type=text class=lg name=login>
        <br>
        <P>Password</font>
        &lt;
        <input type=password class=lg name=password>
        <p>
        <br>
        <button type="submit" name="submit" value="Submit">
        <img src="lobutton.png"/>
        </button>
        </form>
        ';
    }
}
elseif($loggedin==1)
{
    //Nothing special..just alot of text and user information
}

[laserlight]

dreamkat wrote:

ah, It seems I cant edit my post, sorry ^; I'm new to this.

No worries. You may want to post your updated version with proper formatting as it is difficult to trace the flow of control otherwise.

dreamkat wrote:

register_globals are off.

That's good 🙂

dreamkat wrote:

Hm, no I dont know much about how they hack,
Would I prevent it by using

Yes, that is appropriate for incoming variables that should be treated as strings in the SQL statement. That said, I would not use it on the password because you will be putting it through md5 anyway.

dreamkat wrote:

Well $loggedin is apart of config.php

That sounds wrong. It looks like this code should be part of your authentication check, not a configuration file.

By the way, instead of writing:

$update = @mysql_query("UPDATE `players` SET `lastlogin` = '$date' WHERE `id` = '".$_SESSION['id']."'");
$updateip = @mysql_query("UPDATE `players` SET `lastip` = '$ip' WHERE `id` = '".$_SESSION['id']."'");

Write:

mysql_query("UPDATE `players` SET `lastlogin` = '$date', `lastip` = '$ip' WHERE `id` = '".$_SESSION['id']."'");

Actually, I would write it as:

mysql_query(sprintf("UPDATE `players` SET `lastlogin`='%s', `lastip`='%s' WHERE `id`=%d",
    date("M d, Y h:i a"), $_SERVER['REMOTE_ADDR'], $_SESSION['id']));

assuming that the id is an integer. If you want to be paranoid (which can sometimes be a good thing for security), apply mysql_real_escape_string for the return value of date and $_SERVER['REMOTE_ADDR'].