Page 1 of 2 12 Last
  • Jump to page:
    #1
  1. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Apr 2008
    Posts
    53
    Rep Power
    0

    Secure PHP Login Script


    Can you tell me if this php login script is secure? And / Or where it can be improved.

    check_login.php
    PHP Code:
    <?php

    session_start
    ();
    include(
    'config.php');

    trim($_POST['username']);
    trim($_POST['password']);

    $username $_POST['username'];
    $password $_POST['password'];

    if(!
    preg_match("/^[-a-z0-9 ']{4,12}+$/i",$_POST['username'])){
        echo 
    "Username error";
        exit();
    }

    sqlconnect();
    $sqlquery mysql_query("SELECT * FROM users WHERE username = '$username' AND password = '".md5($password)."'");

    if (
    mysql_num_rows($sqlquery) > 0) {
        
    session_regenerate_id();
        
    $sqldata mysql_fetch_assoc($sqlquery);
        
    $_SESSION['USERID'] = $sqldata['userid'];
        
    $_SESSION['LOGGEDIN'] = true;
        
        
    session_write_close();
        
    header("Location: members.php");
        exit();
    } else {
        echo 
    "login error";
        echo 
    $password;
        echo 
    md5($password);
        
    }
    ?>
    members.php
    PHP Code:
    <?php

    session_start
    ();

    if ((
    $_SESSION['LOGGEDIN'] = false) OR (!$_SESSION['USERID'])) {
        include(
    'header.php');
        echo 
    "<p>Please login before playing.</p>\n";
        include(
    'footer.php');
        exit();
    }

    include(
    'header.php');

    ?>
  2. #2
  3. Getting better 1 line @ a time
    Devshed Beginner (1000 - 1499 posts)

    Join Date
    Feb 2004
    Location
    Sheffield
    Posts
    1,013
    Rep Power
    38
    After a quick look, the first bit seems fine as you do a regex for alphanumerics, so that should be fine.

    You might want to store the username and md5(password) inside the session also so it can revalidated. Because at the moment you just store an ID and loggedin= true.

    I don't know if this can be done, but if it could someone would just need to hijack the session and set "loggedin=true" and make up a user id which could be 1, as your user id is likely to be just a number. If they do that they are in without having to ever get past your authenitcation SQL statement.

    Hope that helps

    Jake
  4. #3
  5. "That Guy"
    Devshed Novice (500 - 999 posts)

    Join Date
    Apr 2005
    Location
    Wouldn't you like to know? (To come beat me up maybee)
    Posts
    677
    Rep Power
    372
    As said above, store the md5 password instead of a loggedin value in the session. It will easily be hijacked otherwise.
    Last edited by Brokenhope; May 18th, 2008 at 02:21 PM.
  6. #4
  7. Permanently Banned
    Devshed Specialist (4000 - 4499 posts)

    Join Date
    Jun 2006
    Location
    In a whale
    Posts
    4,127
    Rep Power
    0
    Am I the only one who noticed the MySQL injections on the way in?

    [phpnet=mysql_real_escape_string]Clean[/phpnet] your data. [phpnet=htmlspecialchars]It[/phpnet] helps.

    Comments on this post

    • jakenoble agrees
    • jordoncm agrees
  8. #5
  9. Getting better 1 line @ a time
    Devshed Beginner (1000 - 1499 posts)

    Join Date
    Feb 2004
    Location
    Sheffield
    Posts
    1,013
    Rep Power
    38
    Ah yes that as well ryon420. If your doing the regex on the username AND password, is there any room for code injections, seen as no chars can be injected, just numbers and letters.
  10. #6
  11. Permanently Banned
    Devshed Specialist (4000 - 4499 posts)

    Join Date
    Jun 2006
    Location
    In a whale
    Posts
    4,127
    Rep Power
    0
    Yes. Look carefully at the regex. Notice anything like `'` in it? Yeah, you are calling for an injection there.
  12. #7
  13. Getting better 1 line @ a time
    Devshed Beginner (1000 - 1499 posts)

    Join Date
    Feb 2004
    Location
    Sheffield
    Posts
    1,013
    Rep Power
    38
    Ah missed that, well spotted.
  14. #8
  15. No Profile Picture
    I AM A GOLDEN GOD
    Devshed God 1st Plane (5500 - 5999 posts)

    Join Date
    Apr 2003
    Location
    Camarillo, California
    Posts
    5,929
    Rep Power
    1170
    Observations and general comments.
    PHP Code:
    if (($_SESSION['LOGGEDIN'] = false) OR (!$_SESSION['USERID'])) { 
    Look at that first 'comparison' carefully. That conditional will always return true, because you're actually assigning boolean false to $_SESSION['LOGGEDIN'].

    If you're going to use MD5 as your hash algo, you can just perform this in the SQL statement, as MD5 is an innate MySQL function as well. I would strongly suggest using at least SHA1 and 'salting the hash' as well.

    You're only using the `userid` field from the SELECT statement, so you should only perform the SELECT on that column. No need to pull all values down if you're only using one. It's a good habit to get away from using the SELECT ALL ('*') anyway.

    Your conditional checking the num_rows should only check for 1. In other words:
    PHP Code:
    if ( mysql_num_rows($resultResource) === ) { } 
    Your DB should absolutely limit the ability to store a non-unique user name, but this is good practice anyway. Use a UNIQUE constraint on your `username` field.

    NEVER store the password in the session. It's too easy to hijack session data on a shared server.

    Using a regular expression is a good idea, but for the sake of the user experience, you should return them to the page to reinput valid data. Using JavaScript to double up with a client side validation is also a good idea. I still don't think it takes the place of an escaping function or a prepared statement. Take as many steps possible, don't make assumptions.

    Comments on this post

    • jakenoble agrees
    • Velocipes agrees : Great post :)
    "Seriously, we're not a search engine, we're actual people." ~ ManiacDan

    BookMooch.com : Give books away. Get books you want.
  16. #9
  17. c0der
    Devshed Novice (500 - 999 posts)

    Join Date
    Jun 2005
    Location
    Vancouver
    Posts
    664
    Rep Power
    159
    If I feed that script the username "' OR 1 -- ", who's logged in? Surely not the first user, who just happens to be an admin?

    What happens if I use the name "admin" and set my password to "<script>alert(document.cookie);</script>"? Uh oh, is that XSS I see? Granted, it's more difficult to exploit since this is post data, but how hard could it be to find another XSS-able site to submit a form for me? It could be one big happy XSS chain of mayhem! Ever been Rick Roll'd? XSS is the same idea.

    What if I were to gain select access on your database. How many of those passwords do you suppose I could crack using a service like the one offered at milw0rm.org?

    All these problems (except for the XSS thing, see htmlspecialchars) could be corrected by following the advice given above. I just thought I'd give you some examples of how your code could actually be exploited by someone with malicious intent (or a curious disposition).

    By the way, those trims at the beginning of your program aren't doing anything other than using up CPU cycles.

    Comments on this post

    • ryon420 agrees : All hail the XSS guru :tntworth:
    • Velocipes agrees : Thanks, working on a fix as recommended
    Last edited by Joseph Taylor; May 18th, 2008 at 05:40 PM.
  18. #10
  19. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Apr 2008
    Posts
    53
    Rep Power
    0
    Originally Posted by lnxgeek
    NEVER store the password in the session. It's too easy to hijack session data on a shared server.
    Other have suggested storing the password md5 hash in the session to re-validate. What data would you recommend storing in the session?

    Would a cookie be better than using a session?

    Originally Posted by Joseph Taylor
    If I feed that script the username "' OR 1 -- ", who's logged in? Surely not the first user, who just happens to be an admin?
    Would the preg_match not restrict you from sending this because it contained ' ?
  20. #11
  21. Permanently Banned
    Devshed Specialist (4000 - 4499 posts)

    Join Date
    Jun 2006
    Location
    In a whale
    Posts
    4,127
    Rep Power
    0
    Would the preg_match not restrict you from sending this because it contained ' ?
    Just read the five links I posted in post number 4 and you're set.
  22. #12
  23. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Apr 2008
    Posts
    53
    Rep Power
    0
    Yer, I am working on that
  24. #13
  25. No Profile Picture
    I AM A GOLDEN GOD
    Devshed God 1st Plane (5500 - 5999 posts)

    Join Date
    Apr 2003
    Location
    Camarillo, California
    Posts
    5,929
    Rep Power
    1170
    Originally Posted by Velocipes
    Other have suggested storing the password md5 hash in the session to re-validate. What data would you recommend storing in the session?

    Would a cookie be better than using a session?
    I use a `logins` table separate from the user's table to revalidate each page request. In this method the session stores 1) the user id referencing the `user` record, 2) a unique random token that can either be regenerated upon each successful validation or persist through the entire session. The `logins` table also stores the user id value and that random token, along with the session id value and a timestamp. I find it a good way to abstract the login data away from the actual user data, and it's much more sanitary than storing any user data in the session (other than a useless user id value that is).

    You're already using session_regenerate_id() on each successful validation, which I also recommend. This is a good way to ward off a session hijack in and of itself.

    The session stores a cookie referencing the session data, so no need to set another one storing any sort of data; the only reason to set a cookie is to implement a 'remember me' function that persists after the browser closes and the session ends. Your script checks for a cookie, attempts to validate based on that alone, then next checks for session data, attempts to validate on that, then throws up the login page if these two methods fail.

    Comments on this post

    • Joseph Taylor agrees : Thanks for the insights on session hijacking. Sounds like something I should investigate further.
    "Seriously, we're not a search engine, we're actual people." ~ ManiacDan

    BookMooch.com : Give books away. Get books you want.
  26. #14
  27. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Oct 2006
    Posts
    144
    Rep Power
    0

    Talking


    also, always keep in mind that never return all fields in your query

    PHP Code:
    $sqlquery mysql_query("SELECT * FROM users WHERE username = '$username' AND password = '".md5($password)."'"); 
    instead , you should use count(*)

    PHP Code:
    $sqlquery mysql_query("SELECT count(*) as CNT FROM users WHERE username = '".addslashesh($username)."' AND password = '".md5(addslashesh($password))."'"
    use "require_once" instead include() ...

    This script will affect execution time .
    Last edited by geekjob; May 21st, 2008 at 12:26 AM.
  28. #15
  29. Expert Debugger
    Devshed Beginner (1000 - 1499 posts)

    Join Date
    Apr 2006
    Location
    Dev Shed Forums (-_^)v
    Posts
    1,021
    Rep Power
    1306
    On the side note ... from the thread ... yahoo mail can be exploited if u have the victim's cookie ...

    that is i can access your yahoo mail account if i have your session cookie, i have tried this on my account and i was successful in doing that ....

    so cookie data plays an important role ... in security
Page 1 of 2 12 Last
  • Jump to page:

IMN logo majestic logo threadwatch logo seochat tools logo