3

A php newbie here. Below is the code I'm using to build a login system to enter mypage.php

It's working great but it is quite naive, anyone can type mypage.php in the url and avoid the login page. How can I build it more secure?

Thanks a lot! 

    if(isset($_POST['submit'])) {
        $user = $_REQUEST['user'];  
        $pass = $_REQUEST['pass'];
        $sql = "SELECT * FROM login WHERE user='".$user."'";
        $res = $this->new_db->select($sql); 
        $row = $this->new_db->get_row($res);
        if (isset($row)) {  //user exists?
            if($row["pass"] == $pass){
$_SESSION['userId'] = $row['user'];// TRYING WITH SESSIONS
                header("Location: mypage.php");
            } else {
                echo "wrong pass";
            } 
        } else {
            echo "user does not exist";    
        }
    }

Then in mypage.php

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

//contents

} else {
echo "there's an error";

}

It is printing "there's an error" why?? Thanks a lot

user712027
  • 572
  • 6
  • 9
  • 22

6 Answers6

4

Three things:

  1. Don't store passwords in plain text. Given this code, I can only assume that's what you're doing. You should store the passwords hashed, hash the password the user enters, and compare those.
  2. You have a SQL injection vulnerability. Any time you're receiving input from the user that's destined for a database query, at the very least you should wrap it in mysql_real_escape_string().
  3. On the logged-in page (on any logged-in page) you'll want to track whether or not the user is logged in. One simple way to do this is to have the login form set a $_SESSION value indicating the user's current logged-in status. Then on any page which requires a user to be logged in, check for that value. If it exists, they've previously logged in. If it doesn't, they haven't. It's simple, but good enough to get your going for what you need.
David
  • 208,112
  • 36
  • 198
  • 279
1

yes, there might be sql-injection in your code in order to prevent you may use mysql_real_escape_string function

heximal
  • 10,327
  • 5
  • 46
  • 69
1

Check my answer here i posted before some time, which explains how you should go with login systems.

Open-source rich HTML editor

Community
  • 1
  • 1
Ibrahim Azhar Armar
  • 25,288
  • 35
  • 131
  • 207
  • I'm doing $_SESSION['userId'] = $row['user']; – user712027 Aug 01 '11 at 12:39
  • and then if(isset($_SESSION['userId'])) { in mypage.php , but mypage.php shows nothing even when the login data entered is correct – user712027 Aug 01 '11 at 12:40
  • that means your username and password was incorrect. i suppose. while inserting the values into your database did you use sha1() to convert it into hash, can you post the screenshot of your database table ? – Ibrahim Azhar Armar Aug 01 '11 at 12:40
  • no errors even when I have the error reporting on. Not even in Firebug console. Do I have to add anything else to $_SESSION['userId'] = $row['user']; in my login page? – user712027 Aug 01 '11 at 12:43
  • `$_SESSION['userId']` does not exist initially, it will only be created if your username and password matched with the one store in database. if your username and password matches then only $_SESSION['userId'] will be created. this is what is happening in your case, if it doesn't gives you access to that page, that means the $_SESSION variable does not exist or not have been created, if you could post a screenshot of your database on how have you built the tables, i could help you more. – Ibrahim Azhar Armar Aug 01 '11 at 12:50
  • you have to include session_start(); in the beginning of your code. sorry i forgot to mention it. – Ibrahim Azhar Armar Aug 02 '11 at 05:04
1

You will need to put something in mypage.php to check to see if the user is "logged in". I have done this in the past with the Zend Auth module from the Zend Framework. The cool thing about it is it can be used alone, (you don't have to make a whole Zend Framework site to use the Auth module). I used the Zend Auth Page to figure out how to use it.

Then, once I setup the auth session using the Zend Session, I just checked at any other page to see if the user was "logged in" with something like this:

private function _loggedIn()
{
    $loggedIn = false;
    $Namespace = new Zend_Session_Namespace('Zend_Auth');
    foreach ($Namespace as $index => $value) {
        $loggedIn = ($value->user_id);
    }
    return $loggedIn;
}
Steve
  • 1,596
  • 1
  • 10
  • 21
0

First of all, you have to protect your mysql query from sql injection. This can be achieved by adding mysql_real_escape_string like that:

$user = mysql_real_escape_string($_REQUEST['user']);

Then, if you don't want users to be able to visit mypage.php without being logged in, you should set some cookie in your login script if the login is successful, and then, on mypage.php, check that cookie to see if it matches in your database. Something like that:

login.php:

if($row["pass"] == $pass){
   setcookie("userid",$user);
   setcookie("passhash",sha1($pass));
...

mypage.php

$res = mysql_query("select * from login where user='".mysql_real_escape_string($_COOKIE['userid'])."' limit 1");
$row = mysql_fetch_assoc($res);

if($_COOKIE['passhash'] == sha1($row['pass']))
{
    die("logged in OK");
}
else
{
    die("please log in");
}
  • using COOKIES to check user credentials ?? it is totally wrong, Cookies was not meant for this. you should use Sessions instead. – Ibrahim Azhar Armar Aug 01 '11 at 12:32
  • @Ibrahim Azhar Armar Sessions use cookies to store a session ID so by using sessions you are essentially using cookies. Sessions also may not be feasible on a high traffic website or a website where servers are distributed (each server would have its own session entries). +1 to poster to cookies – Bailey Parker Aug 01 '11 at 12:43
  • yes, agreed Cookies stores the current session id, but COOKIE was not meant to be used for this. if i am wrong do you have any link to prove i am wrong? – Ibrahim Azhar Armar Aug 01 '11 at 12:55
-1
if (isset($_POST['submit'])) {
  $user = $_REQUEST['user'];  
  $pass = $_REQUEST['pass'];

  $sql = "SELECT user FROM login WHERE user='".mysql_real_escape_string($user)."' AND pass=SHA1('".mysql_real_escape_string($pass)."')";
  $res = $this->new_db->select($sql);
  $row = $this->new_db->get_row($res);  

  if ($row['user'] != "") {  //user exists?
    header("Location: mypage.php");
  }else{
    echo "username and password combination is wrong";
  }
}
Jesse
  • 580
  • 4
  • 7
  • you should use Sessions for checking login credentials, this is not at all secure and make no sense. – Ibrahim Azhar Armar Aug 01 '11 at 12:22
  • I agree that he should use sessions...I was taking his code and making it more secure then it was. Your answer of using md5 is incorrect though as well. MD5 has been shown to be insecure. You should always use SHA1 or something more secure – Jesse Aug 01 '11 at 12:23
  • even `sha1()` is insecure and can be broken if you know, md5() or sha1() as i have asked him to use anything of his choice. your code uses the incorrect logic to deal with OP's problem. – Ibrahim Azhar Armar Aug 01 '11 at 12:29
  • MD5 and SHA1 are insecure. But they're still used. What makes them more secure is using salts (give "md5 salts php" a Google). – Bailey Parker Aug 01 '11 at 12:45
  • or how about bcrypt. http://stackoverflow.com/questions/4795385/how-do-you-use-bcrypt-for-hashing-passwords-in-php – Ibrahim Azhar Armar Aug 01 '11 at 13:02
  • Or how about [AES](http://dev.mysql.com/doc/refman/5.0/en/encryption-functions.html#function_aes-encrypt) – Jesse Aug 01 '11 at 15:13
  • @Jesse most times passwords should be stored hashed, not encrypted as hashes are irreversible and encryption is reversible. So unless you need the users password (which you shouldn't) use SHA256/SHA512/bcrypt – Ethan H Jul 04 '12 at 04:14