SitePoint Sponsor

User Tag List

Page 1 of 3 123 LastLast
Results 1 to 25 of 52
  1. #1
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)

    need some help with login system

    hi, I am having trouble with my login code, I am sure some of it is just wrong but I am a little stuck (I am very new)

    This is my index.php, it has my login script and also includes my home.php page that shows a login form.
    PHP Code:
    <?php
    if (!isset($_SESSION['username'])) {
    if (isset(
    $_POST['username']))
    {
    // connect to database
    require_once $_SERVER['DOCUMENT_ROOT'] . 'connect.php';

    // load functions
    require_once $_SERVER['DOCUMENT_ROOT'] . 'functions.php';

    // sanitize input. safe = mysqli_real_escape_string
    $username safe($link$_POST['username']);
    $password safe($link$_POST['password']);

    // search database for username/password entered.
    $sql "SELECT username, password FROM members WHERE username = $username AND password = $password";
    if (!
    mysqli_query($link$sql))
        {
        include 
    'home.php';
        
    $error 'Incorrect username and or password.';
        include 
    'error.php';
        }
            else
            {
            
    session_start();
            
    $_SESSION['username'] = $username;
            
    header(Location'userpage.php');
            }
        }
        else
        include 
    'home.php';
        }
    ?>
    any tips? I get my error and the login form no matter if my username/password is correct or not.

  2. #2
    SitePoint Evangelist AlienDev's Avatar
    Join Date
    Feb 2007
    Location
    UK
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    The literal strings in your SQL need wrapped in quotes:
    WHERE username = '$username' AND password = '$password'";
    Me on StackOverflow | Blog & personal website.

    I mostly use: PHP, Java, JavaScript, Android.

  3. #3
    SitePoint Wizard
    Join Date
    Nov 2005
    Posts
    1,191
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Also, mysqli_query will only return false if the query fails, not if it returns zero rows.
    Use SELECT COUNT(*) AS count FROM ... and check whether count is zero.

  4. #4
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    awesome, thank you both. trying to research how to implement COUNT now as I haven't dealt with it before.

    do I change my SELECT to

    $sql = "SELECT COUNT(*) FROM members WHERE username = '$username' AND password = '$password'";

    and then check the result somehow? that's my next thing to research heh

  5. #5
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    I changed that part of my code to this:

    PHP Code:
    $check mysqli_query($link"SELECT username, password FROM members WHERE username = '$username' AND password = '$password'");
    $result mysqli_num_rows($check);
    if (
    $result != 1
    and it seems to work.. is that acceptable code though? I had trouble figuring out how to implement COUNT

  6. #6
    SitePoint Evangelist AlienDev's Avatar
    Join Date
    Feb 2007
    Location
    UK
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    That code is fine. You want to know the username anyway, so COUNT() would be the wrong query.
    Me on StackOverflow | Blog & personal website.

    I mostly use: PHP, Java, JavaScript, Android.

  7. #7
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    ok thank both of you VERY much, it seems to be working perfectly - I can now move on different things.

  8. #8
    SitePoint Wizard
    Join Date
    Nov 2005
    Posts
    1,191
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by AlienDev View Post
    That code is fine. You want to know the username anyway, so COUNT() would be the wrong query.
    You already know this, if you didn't you couldn't perform the query.'
    Either way is fine.

  9. #9
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    going to bump this instead of making a new thread, just wondering if someone can tell me if my code is ok.

    I added some remember-me and md5 stuff, and it all seems to work fine, but not sure if I am setting sessions/cookies correctly - if someone could confirm that I am doing it right or wrong that would be great. Here is my index.php code:

    PHP Code:
    <?php
    if (!isset($_SESSION['username'])) {
    if (isset(
    $_POST['username']))
    {
        require_once 
    $_SERVER['DOCUMENT_ROOT'] . 'connect.php';
        require_once 
    $_SERVER['DOCUMENT_ROOT'] . 'functions.php';
        
    $username safe($link$_POST['username']);
        
    $password safe($link$_POST['password']);
        
    $remember $_POST['remember'];
        
    $mix md5($password 'mysalt');
        
    $check mysqli_query($link"SELECT username, password FROM members WHERE username = '$username' AND password = '$mix'");
        
    $result mysqli_num_rows($check);
        if (
    $result != 1)
            {
                
    header('Location: home.php');
            }
            else if (
    $result && $remember == "")
            {
                
    session_start();
                
    $_SESSION['username'] = $username;
                
    header('Location: home.php');
            }
            else if (
    $result && $remember "on")
            {
                
    session_start();
                
    $_SESSION['username'] = $username;
                
    setcookie("mycookie"$usernametime()+7200);
                
    header('Location: home.php');
            }
        }
        else
            include 
    'home.php';
        }
    ?>
    oh and this is my logout.php, is it right? seems to work...

    PHP Code:
    <?php
    session_start
    ();
    unset(
    $_SESSION['username']);
    session_unset();
    session_destroy();
    header('Location: home.php');
    ?>

  10. #10
    Non-Member
    Join Date
    Oct 2009
    Posts
    1,852
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I have no idea what you have in your safe() function, but whole approach is wrong.
    There is 2 rules that must be followed unconditionally.
    1. No "safe" function must be applied to the password.
    2. Any string literal that goes to the query, must be prepared using mysql_real_escape_string function()

    Another most important issue.
    You should align your code properly.
    Code indention were invented not for beauty!
    But to let programmer understand what's going on.
    Once I look into your code, I cant.

    Next. Each location header must be followed by exit statement, to stop further execution.

    Less important issue.
    Why do you double your code?
    Programming itself stands for eliminating repeated code fragments.
    PHP Code:
            else {
              if (
    $remember "on") {
                
    setcookie("mycookie"$usernametime()+3600*24*365);
              }
              
    session_start();
              
    $_SESSION['username'] = $username;
              
    header('Location: home.php');
              exit;
            } 
    isn't it less code to write and easier to understand?

  11. #11
    SitePoint Wizard
    Join Date
    Nov 2005
    Posts
    1,191
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Few things:

    session_start() needs to be at the beginning of the srcipt or $_SESSION won't exist, and you won't have access to $_SESSION['username']

    The script doesn't stop because you used header('Location: someurl') it continues to execute, so it is a good idea to exit after a redirect.

    You have $result = 1 in your conditions, this will always evaluate to true as you are simply assigning 1 to $result.

    With remember me, you need to set a token in the cookie and in the database (similar to a password check), and check these are equal, otherwise someone can just send a cookie with anybody's username in it and get access to their account.

  12. #12
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    thank you both. I have cleaned it up a bit, and it works great.

    added exit to all headers and cleaned up my doubled code. also, 'safe' = mysqli_real_escape_string. but I don't need to apply that to $_POST['password']?

    only thing I'm confused about is this:

    "With remember me, you need to set a token in the cookie and in the database (similar to a password check), and check these are equal, otherwise someone can just send a cookie with anybody's username in it and get access to their account."

    my remember me is working now, but I guess I still need to do this.

    do I just add a column to my users table and adjust that column according to whether or not they have selected remember me? I guess I also need to modify my setcookie somehow? currently it is like this:

    setcookie("loggedin", $username, time()+3600*24*365);

  13. #13
    Non-Member
    Join Date
    Oct 2009
    Posts
    1,852
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    but I don't need to apply that to $_POST['password']?
    No.
    It has nothing to do with $_POST['password']
    It is used to escape some characters in the string that goes to the query. It is $mix that goes to the query. So function should be applied to the $mix variable.

    o I just add a column to my users table and adjust that column according to whether or not they have selected remember me?
    Most likely.
    Put there a string which is similar to your $mix. Say, md5($username.$password . 'myremembersalt');
    and put the same into cookie.

  14. #14
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    ok thanks yet again! I love this forum.

    cleaned up code further, now trying to figure out how to do the cookie thing.

    thanks!

  15. #15
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Quick follow up, do I put this unique string in place of the $username in my cookie.

    this was my cookie code before:
    PHP Code:
    if ($rememberme == "on") {
      
    setcookie("loggedin"$usernametime()+3600*24*365);

    should it be like this:
    PHP Code:
    if ($rememberme == "on") {
      
    $rememberme md5('$username''$password' 'mysalt');
      
    setcookie("logged"$remembermetime()+3600*24*365);
      
    $sql "INSERT INTO members SET cookie = '$rememberme' WHERE username = '$username'"

    and then if they log out just set the cookie column to empty.

    is this right?

  16. #16
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    or would it be safe to do without the database insert, just put a md5'd token in the cookie instead of a simple $username would be ok?

    also, since I'd be md5ing username+password+mysalt, if the username and/or password exceeds 32 chars does that matter, will md5 always produce a 32 char string regardless of length of the string?

  17. #17
    Non-Member
    Join Date
    Oct 2009
    Posts
    1,852
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    just md5 token without a database is not an option.
    Because you cannot create a query to check.
    So, you can either put this token into database for the further checking, or set another cookie, with user id, for the same purpose

  18. #18
    SitePoint Evangelist AlienDev's Avatar
    Join Date
    Feb 2007
    Location
    UK
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Shrapnel_N5 View Post
    No.
    Put there a string which is similar to your $mix. Say, md5($username.$password . 'myremembersalt');
    and put the same into cookie.
    Definitely don't use md5. You will very quickly (say at around 2000 users with 'remember me' set) get people signed in as another user.

    See this: http://laurent.bachelier.name/2010/0...y-is-not-easy/
    Me on StackOverflow | Blog & personal website.

    I mostly use: PHP, Java, JavaScript, Android.

  19. #19
    SitePoint Wizard
    Join Date
    Nov 2005
    Posts
    1,191
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Not if you have also a username cookie.

  20. #20
    SitePoint Evangelist AlienDev's Avatar
    Join Date
    Feb 2007
    Location
    UK
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by hash View Post
    Not if you have also a username cookie.
    Then I'll just log in as myself, and change the cookie value to the admin's username and destroy the website.

    A username cookie is possibly the worst security practice possible. Even worse than md5().
    Me on StackOverflow | Blog & personal website.

    I mostly use: PHP, Java, JavaScript, Android.

  21. #21
    SitePoint Wizard
    Join Date
    Nov 2005
    Posts
    1,191
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Did you actually read this thread?

  22. #22
    SitePoint Evangelist AlienDev's Avatar
    Join Date
    Feb 2007
    Location
    UK
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by hash View Post
    Did you actually read this thread?
    Off Topic:

    I was the first person to reply. Are you going to actually talk about PHP or try making silly points without thinking?
    Me on StackOverflow | Blog & personal website.

    I mostly use: PHP, Java, JavaScript, Android.

  23. #23
    SitePoint Wizard
    Join Date
    Nov 2005
    Posts
    1,191
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    md5 has collisions, but what happens when you pair that with some other data like a user id? The 2000 you speak of is best illustrated by the "birthday problem". If the name and birthday had to be the same then the probability is reduced by orders of magnitude.

    Why did I ask if you read the thread? Because you missed my post earlier it seems.

  24. #24
    SitePoint Evangelist AlienDev's Avatar
    Join Date
    Feb 2007
    Location
    UK
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by hash View Post
    md5 has collisions, but what happens when you pair that with some other data like a user id? The 2000 you speak of is best illustrated by the "birthday problem". If the name and birthday had to be the same then the probability is reduced by orders of magnitude.
    Yay, let's reduce the probability of a hack from 1/2000 to 1/(2000/365). Is that seriously your security advice? Worst advice ever.

    Why did I ask if you read the thread? Because you missed my post earlier it seems.
    I just re-read it and you don't seem to know the difference between a cookie and the session.
    Me on StackOverflow | Blog & personal website.

    I mostly use: PHP, Java, JavaScript, Android.

  25. #25
    SitePoint Wizard
    Join Date
    Nov 2005
    Posts
    1,191
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I'm not sure what your point is. You offer no advice and your numbers are nonsensical.

    If you think setting a userid/name cookie along with an md5 based on some random data is a security threat, please explain. And if possible offer a better solution.
    Off Topic:


    Quote Originally Posted by AlienDev View Post
    ... you don't seem to know the difference between a cookie and the session.
    cookies are what you eat after the session


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
  •