1
\$\begingroup\$

There can be some errors in the script, if u find them thanks, but the main thing is to get useful suggestions about how can I do this better and secure.

ini_set('session.use_only_cookies', true); 
session_start();
//Create a random salt value
$salt = 'Hjkhkjh9089&j98098';
$tokenstr = (str) date('W') . $salt; 
//Create a md5 hash to be used for token.
$token = md5($tokenstr);

///checking if already logged in 
if ($_REQUEST['token'] == $token AND $_SESSION['user_ip']=$_SERVER['REMOTE_ADDR'] ) { 
   ...grant user with some data 
}


//user login block   
if (($_POST['login']!="") OR ($_COOKIE["username"]!='' AND $_COOKIE["password"]!='') AND $_SESSION['user']=='') 
{

    if ($_SESSION['temp_ban']==1)
    {

    $p='login_error';
    }
        else
        {
        $ip=$_SERVER['REMOTE_ADDR'];
        $result2=  mysql_query("SELECT * FROM banned_ip_list WHERE banned_ip='$ip'") or die("unable to connect to msql server: " . msql_error()) ;
        $row2 = mysql_fetch_array($result2);
            if (!$row2=="")
            {
            $_SESSION['temp_ban']=1;
            $p='login_error';
            }
                else
                {
                    if ($_COOKIE["username"]!='' AND $_COOKIE["password"]!='')
                    {
                    $login_username=security2($_COOKIE["username"]);
                    $login_password=security2($_COOKIE["password"]);
                    $remember_me='on';
                    }
                    else
                    {
                    $login_username=security2($_POST['username']);
                    $login_password=security2($_POST['password']);
                    $remember_me=security2($_POST['remember_me']);
                    }
                    $result = mysql_query("SELECT * FROM users WHERE    username='$login_username' AND password=SHA1('$login_password')") or die("unable to connect to msql server: " . msql_error()) ;

                    $row = mysql_fetch_array($result);
                        if ($row!="")
                        {
                            if($remember_me!='')
                            {
                            $expire=time()+60*60*24*7;
                            setcookie("username", $login_username , $expire);
                            setcookie("password", $login_password , $expire);
                            }

                        $_SESSION['user_ip']=$_SERVER['REMOTE_ADDR'];
$_SESSION['user_id']=$row['id'];
$_SESSION['user']=$row['username'];
$_SESSION['ch_insert_temp_key']=1;
$_SESSION['token'] = $token; 
output_add_rewrite_var('token', $token);                


$_POST['login']='';
unset($_SESSION['try_error']);
                        }
                            else
                            {
                                if (!$_SESSION['try_error'] AND !$_SESSION['temp_ban'])
                                {
                                $_SESSION['try_error']=11;
                                }
                                if ($_SESSION['try_error']-1==1)
                                {
                                $_SESSION['temp_ban']=1;
                                $ip=$_SERVER['REMOTE_ADDR'];
                                $banned_time=date('Y-m-d H:i:s');

                                $result3 = mysql_query("INSERT INTO banned_ip_list (banned_ip,banned_time) VALUES ('$ip','$banned_time')") or die("unable to connect to msql server: " . msql_error()) ;
                                /////////////log
                                $event_type_id=10;
                                $additional_info='';
                                $event_object='';

                                include $log_entry;
                                //////////////////
                                }
                                else {$_SESSION['try_error']--;}
                            $p='login_error';
                            }

                }
        }

}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ This is not something you should write yourself. Find an appropriate module OS project where this is already implemented and use it. It is way to easy to add security leaks. \$\endgroup\$ Aug 30 '11 at 14:16
  • \$\begingroup\$ can u suggest me something via link ? :) \$\endgroup\$
    – David
    Aug 30 '11 at 14:47
  • \$\begingroup\$ I really like the random salt ;-) For the password hashing, use PHPass, for how to handle the login or remember me you can check this answer (mine) on StackOverflow. Last note: Never store passwords other than in the database, and hashed. \$\endgroup\$ Sep 28 '11 at 15:13
2
\$\begingroup\$

Let's start with the SQL injection vulnerability in your code in

mysql_query("SELECT * FROM users WHERE    username='$login_username' AND password=SHA1('$login_password')") or die("unable to connect to msql server: " . msql_error()) ;

In this piece of code you put the user defined $login_username straight to a SQL query. I see that there's a function called security2 that does something to username but I cannot be sure that it executes mysql_real_escape_string that would prevent SQL injection. If it does not, then a malevolent user might be tempted to enter a user name that may corrupt your database.

Poor Bobby Tables might cause problems here .

Rest of the thoughts are (at least at first) in random order

Uou should probably migrate from mysql_ to mysqli_ functions. mysqli is the MySQL Improved Extension for PHP that is supposed to work better and do much more.

In PHP, it's almost always better to use === than == to test equality.

You could try to be consistent with the use of ''and "".

The code would benefit if you would define more functions to partition the logic. Maybe even define some classes.

\$\endgroup\$
2
  • \$\begingroup\$ can u explain both things you've said ? \$\endgroup\$
    – David
    Aug 30 '11 at 10:44
  • \$\begingroup\$ Actually, he said four things. 1- He gave you two operator use advices 2- He told you to write smaller, more to-the-point functions (divide and conquer principle) 3- he showed you your code has a weakness since it hasn't got classes 4- he told you to use MySQLi and explained why. And on top of that he showed you a vulnerability, which means a exploitable bug. It's a great post, kudos Aleksi! \$\endgroup\$
    – Kheldar
    Aug 31 '11 at 15:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.