SitePoint Sponsor

User Tag List

Results 1 to 12 of 12
  1. #1
    SitePoint Addict
    Join Date
    Jan 2005
    Location
    Hiding from the world
    Posts
    335
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Is my login script safe using session variables

    Hi,

    i am creating a login script for my site and was hoping someone would be nice enough to have a look at my script and check i am not missing an important security hole. Basically is it safe once i have checked the cookie variables against the DB to set a session variable that is used while they are logged in, rather than having to query the DB each page load.


    here is the code that does the checking to see if a cookie is already remembering the password to use to check against the DB.

    hope this makes sense, any help/suggestions much appreciated

    PHP Code:

    <?php
    //Let's check if there is a cookie set with password
    if(isset($_COOKIE['username']) && isset($_COOKIE['password']) ){
        
        
    //Check if there is a session set so we don't need to check the database otherwise do a database check
        
    if($_SESSION['logged'] !== 'user' || $_SESSION['logged'] !== 'member' || $_SESSION['logged'] !== 'admin'){

            
            
    //lets get the variables
            
    $username mysql_real_escape_string($_COOKIE['username']);
            
    $password mysql_real_escape_string($_COOKIE['password']);
            
            
    //lets do a query on the database now we know they are wanting to login
            
    $query_login mysql_query("SELECT * FROM users WHERE username = '$username' and password = '$password'")or die(mysql_error()); 
            
    //get the results for the login
            
    $row_login mysql_fetch_array($query_login);
            
            
    $total_login mysql_num_rows($query_login);
            if(
    $total_login == 0){
                    
    //it failed so we set an error message
            
    $_SESSION['unwelcome'] = 'username and/or password not recognised';
            
    $_SESSION['welcome'] = '<a href="login.php">Login</a>';    
        }
        else{
            
    //it worked and we have a match so set a welcome message
                    
    $_SESSION['welcome'] = '<H4>Welcome '.$row_login['username'].' - <a href="logout.php">log out</a></h4>';
            
    $_SESSION['unwelcome'] = '';    
            
    //ok so they are good lets set a session variable to allow certain bits to be visible
            //options user,member,admin
            
    $_SESSION['logged'] = $row_login['type'];
        }


            
        }
    }
    else {
    //no cookie was found so we just ask them to log in
    $_SESSION['welcome'] = '<H4><a href="login.php">Login</a></h4>';
    }

    ?>


    then on my pages i just have something simple like

    PHP Code:

    <?php echo $_SESSION['welcome'].' '.$_SESSION['unwelcome'];?>
    <h1>hello page</h1>
    <?php if($_SESSION['logged'] == 'user'){ ?>
    <p>some text only registered people see</p>
    <?php ;}
    elseif(
    $_SESSION['logged'] == 'admin'){ echo 'lets do some admin';}
    else{
    ?>
    <p>Log in to see extra stuff</p>
    <?php }?>
    If i am a product of your imagination you should try harder!

  2. #2
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    I don't see anything syntactically wrong here, but I would make a few suggestions and comments.

    1) RE: "if($total_login == 0)" What if the row count is 3? A better check would be against the actual expected value of 1, failing on anything else.
    2) I don't see any usage of the md5/sha256 hashing methods in your call to check password. Are you storing the passwords in plain text in the cookie? If so, this is not very secure. Other sites can read cookies. I would store the password in the DB as a sha256 salted hash. Also store the salt. In the cookie, store only the hashed pass. Fetch the user by username only, rehash the cookie's stored password, then compare.

  3. #3
    SitePoint Addict
    Join Date
    Jan 2005
    Location
    Hiding from the world
    Posts
    335
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    great thanks for having a look. I'll look at changing the total_login number as what you've pointed out makes sense.

    Yep definitely Hashing the password with a salt. just haven't added that bit in yet as i would only confuse myself whilst getting the basic parts to work

    thanks much appreciated
    If i am a product of your imagination you should try harder!

  4. #4
    Hosting Team Leader silver trophybronze trophy
    cpradio's Avatar
    Join Date
    Jun 2002
    Location
    Ohio
    Posts
    5,162
    Mentioned
    152 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Serenarules View Post
    2) I don't see any usage of the md5/sha256 hashing methods in your call to check password. Are you storing the passwords in plain text in the cookie? If so, this is not very secure. Other sites can read cookies. I would store the password in the DB as a sha256 salted hash. Also store the salt. In the cookie, store only the hashed pass. Fetch the user by username only, rehash the cookie's stored password, then compare.
    This might be a mis-understanding (as I haven't had my caffeine yet), but are you saying to store the hash in the cookie (without the salt taken into consideration)? As I'd argue, that is just as bad as storing it plain text in the cookie anymore.

    Wouldn't you store the salted hash in the cookie and then you can compare it with the username in your query (since your password column should contain the salted hash too)?

    I guess I am not seeing why you would need to rehash the cookie's stored password, as the database should have the sha256 salted hash and the cookie should have stored that same value too.

  5. #5
    SitePoint Addict
    Join Date
    Jan 2005
    Location
    Hiding from the world
    Posts
    335
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    best get this bit right - i was going to do this

    PHP Code:
    $salt "whatever1234";


     
    setcookie('password'md5($salt.$_POST['password']), time()+60*60*24*365,'/','www.mysite.org'); 
    i think i was going to insert the same hashed salted password into my password column. is this the correct thing to do?

    thanks
    If i am a product of your imagination you should try harder!

  6. #6
    Hosting Team Leader silver trophybronze trophy
    cpradio's Avatar
    Join Date
    Jun 2002
    Location
    Ohio
    Posts
    5,162
    Mentioned
    152 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Noppy View Post
    i think i was going to insert the same hashed salted password into my password column. is this the correct thing to do?
    Yes, that is what I would have done, and then your SQL could remain AS IS. But I'm still curious if I missed something that @Serenarules ; was implying.

  7. #7
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by cpradio View Post
    This might be a mis-understanding (as I haven't had my caffeine yet), but are you saying to store the hash in the cookie (without the salt taken into consideration)? As I'd argue, that is just as bad as storing it plain text in the cookie anymore.

    Wouldn't you store the salted hash in the cookie and then you can compare it with the username in your query (since your password column should contain the salted hash too)?

    I guess I am not seeing why you would need to rehash the cookie's stored password, as the database should have the sha256 salted hash and the cookie should have stored that same value too.
    Not exatly. Let me elaborate.

    First, I wouldn't use a global salt. If guessed, all your users base are belong to me. Sorry, bad joke. =)

    I prefer using unique salts, generated at the time of registration, store that in the users record. Not in the cookie. Never in the cookie.

    When logging in, the record is fetched by username only. Then whatever he input for his password is rehashed using the stored salt (from above) and compared.

    If login is successful, the hashed password and username is stored in the cookie like normal.

    The reason this setup is good is because it will accomodate both global and per-user salts.

    I apologize for the unclarity. I must be off my game today, I've had to clarify a few things I've said so far. LOL

  8. #8
    Hosting Team Leader silver trophybronze trophy
    cpradio's Avatar
    Join Date
    Jun 2002
    Location
    Ohio
    Posts
    5,162
    Mentioned
    152 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Serenarules View Post
    Not exatly. Let me elaborate.

    First, I wouldn't use a global salt. If guessed, all your users base are belong to me. Sorry, bad joke. =)

    I prefer using unique salts, generated at the time of registration, store that in the users record. Not in the cookie. Never in the cookie.
    Okay, I agree with that.

    Quote Originally Posted by Serenarules View Post
    When logging in, the record is fetched by username only. Then whatever he input for his password is rehashed using the stored salt (from above) and compared.
    Agree with you there too

    Quote Originally Posted by Serenarules View Post
    If login is successful, the hashed password and username is stored in the cookie like normal.
    Just a bit more clarity, the salted hashed password and username is stored in the cookie, correct? So when they revisit you can compare the values in the cookie against the values in the database (they should match equally without any additional work).

    Quote Originally Posted by Serenarules View Post
    I apologize for the unclarity. I must be off my game today, I've had to clarify a few things I've said so far. LOL
    No worries, just wanted to make sure you were implying what I was hoping but wasn't necessarily reading

  9. #9
    Resident OCD goofball! bronze trophy Serenarules's Avatar
    Join Date
    Dec 2002
    Posts
    1,911
    Mentioned
    26 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by cpradio View Post
    Just a bit more clarity, the salted hashed password and username is stored in the cookie, correct? So when they revisit you can compare the values in the cookie against the values in the database (they should match equally without any additional work).
    Correct. =)

    However, if you REALLY want to get into security, you could go all out and implement a ticket system.

    Read on for ticket system.

    Upon successful login, cache browser type/ver, client ip, time of login, user ID, and generate a ticket id.

    Store ticket ID only in the cookie. Do not even need username!

    Upon page refresh, lookup the ticket, compare current browser type/ver, client ip, check time of login for timeout status (reset if required), then, if all pass, just fetch the user's record via user ID.

    No need to mess with messy passwords. Even if a ticket cookie is stolen, it's properties won't match up against what the thief is using.

    In addition, you could seed each FORM based page with a one-up authorization code, to be compared upon POST. Both the ticket cookie and the posted auth code must be correct in order to process.

    But this is only if you really want to go all-the way.

  10. #10
    SitePoint Addict
    Join Date
    Jan 2005
    Location
    Hiding from the world
    Posts
    335
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    thanks, this is all very useful info. Not sure i need to be super secure as not storing any sensitive info but may just give unique salts a go to be on the safe side and future proof

    thanks for looking over my code
    If i am a product of your imagination you should try harder!

  11. #11
    . shoooo... silver trophy logic_earth's Avatar
    Join Date
    Oct 2005
    Location
    CA
    Posts
    9,013
    Mentioned
    8 Post(s)
    Tagged
    0 Thread(s)
    You shouldn't even put the hashed version of a password into a cookie. What you need is a token, one that is continually regenerated after periods of activity from the user.
    Logic without the fatal effects.
    All code snippets are licensed under WTFPL.


  12. #12
    SitePoint Addict kduv's Avatar
    Join Date
    May 2012
    Location
    Maui, HI
    Posts
    211
    Mentioned
    5 Post(s)
    Tagged
    0 Thread(s)
    I personally wouldn't use md5 or sha1. Neither hashing method was meant for passwords ... but instead for speed which makes brute force attacks much easier. I instead prefer to use mcrypt VIA PHPass. It also takes care of all your salting and stretching needs.

    Here's a great article on password security: http://www.openwall.com/articles/PHP-Users-Passwords
    Keith
    Freelance web developer
    http://www.duvalltech.com/


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •