SitePoint Sponsor

User Tag List

Results 1 to 16 of 16
  1. #1
    SitePoint Evangelist Tapan's Avatar
    Join Date
    May 2005
    Location
    India
    Posts
    563
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)

    Question Is this secure login script ?

    Hi,

    I am on a shared hosting platform and i need to know if the following is secure login or not ?

    This is login.php file:

    Code:
    <?php
    session_start();
    
    if (isset($_POST["btnLogin"]))
    {
            $errcnt = 0;
    
            if (empty($_POST["username"])) { $errcnt++; $error[] = "Invalid Username"; }
            if (empty($_POST["password"])) { $errcnt++; $error[] = "Invalid Password"; }
    
            if ($errcnt == 0)
            {
                    $_SESSION["username"] = $_POST["username"];
                    $_SESSION["password"] = md5($_POST["password"]);
    
                    header ("Location: index.php");
                    exit ();
            }
    }
    ?>
    <h3 align="center">Login</h3>
    <form method="post" action="login.php">
    <table align="center" cellpadding="5" border="0" align="center">
    <tr>
            <td align="right">Username:</td>
            <td><input type="text" name="username" size="30"></td>
    </tr>
    <tr>
            <td align="right">Password:</td>
            <td><input type="password" name="password" size="30"></td>
    </tr>
    <tr>
            <td colspan="2">&nbsp;</td>
    </tr>
    <tr>
            <td align="center" colspan="2"><input type="submit" name="btnLogin" value="Login" /></td>
    </tr>
    </table>
    </form>
    This is access.php file which is on top of every page and check the login details:

    Code:
    <?php
    session_start();
    require_once ("dbconn.php");
    
    $errcnt = 0;
    
    if (empty($_SESSION["username"])) { header ("Location: login.php"); }
    if (empty($_SESSION["password"])) { header ("Location: login.php"); }
    
    $q0 = sprintf ("SELECT * FROM users WHERE username = %s AND password = %s LIMIT 1", fix($_SESSION["username"]), fix($_SESSION["password"]));
    $r0 = mysql_query($q0) or die("Query Failed");
    if (mysql_num_rows($r0) <> 1)
    {
            $_SESSION["username"] = "";
            $_SESSION["password"] = "";
            header ("Location: login.php");
    }
    ?>
    Please let me know. Thanks.

  2. #2
    From Italy with love silver trophybronze trophy
    guido2004's Avatar
    Join Date
    Sep 2004
    Posts
    9,500
    Mentioned
    163 Post(s)
    Tagged
    4 Thread(s)
    What does the function fix() do?

  3. #3
    SitePoint Wizard
    Join Date
    Oct 2005
    Location
    London
    Posts
    1,678
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Yeah what's fix()?

    For one, you should be using mysql_real_escape_string on all input from the user to avoid mysql injection:

    http://uk2.php.net/mysql_real_escape_string

  4. #4
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2008
    Posts
    5,757
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    There's many potential problems.

    I'd suggest some general reading on php security.
    http://shiflett.org/articles
    http://phpsec.org/projects/guide/

  5. #5
    SitePoint Evangelist Tapan's Avatar
    Join Date
    May 2005
    Location
    India
    Posts
    563
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Hi,

    Fix is this:

    Code:
    function fix($value)
    {
            if (get_magic_quotes_gpc())
            {
                    $value = stripslashes($value);
            }
    
            return "'" . mysql_real_escape_string($value) . "'";
    }
    Thanks!

  6. #6
    SitePoint Addict
    Join Date
    Apr 2009
    Posts
    248
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by elduderino View Post
    Yeah what's fix()?

    For one, you should be using mysql_real_escape_string on all input from the user to avoid mysql injection:

    http://uk2.php.net/mysql_real_escape_string
    You should be using parameterized SQL, especially on anything relating to login scripts. Escaping the string doesn't always work.

  7. #7
    SitePoint Wizard
    Join Date
    Mar 2008
    Posts
    1,149
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    mysql_real_escape_string() only poses an issue if you change your encoding in a way where the library is not aware of the change and/or you didn't validate whether the string was encoded correctly.

    There are two major issues with that code, assuming that your PHP settings specify that sessions must use cookies and only cookies to persist the session:
    1. access.php doesn't check where the user came from, so a bad site can just redirect a form to your site and make your page perform some action as an authenticated user (read up on CSRF).
    2. header() does not end the execution of the page. Even though the browser is redirected, the rest of the page will still be executed. You have an exit() in login.php though, so you may just have forgotten it in access.php.


    I think that's all. Bit in a rush right now.

    Also, not a security issue, but you're supposed to send absolute URIs with the Location header according to the specs.

  8. #8
    SitePoint Member
    Join Date
    Feb 2009
    Posts
    14
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I'm not sure I follow. What do you mean by checking where a person came from? I understand the exploit, but what would you do to validate "where" a person is coming from?

  9. #9
    SitePoint Wizard
    Join Date
    Mar 2008
    Posts
    1,149
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Well, that means that you somehow need an identifying piece of information that tells you the user's previous page.

    You have two options:
    • HTTP referrer
    • Something you put in $_GET or $_POST from the previous page

    The HTTP referrer is automatically sent with the browser. For this particular purpose, it's trustworthy because the browser defines the referrer.* However, the issue is that it is disabled on some environments, and so what are you to do when it's missing? If you outright deny the user, then some people will be unable to use your website.

    On the other hand, if you send a piece of information that an attacker cannot guess or get, then you know that the user came from a trusted page because you can verify that information to be correct. The problem with this is that it completely complicates everything for you as the programmer. That piece of information could be a cryptographically secure hash you generate when the user logs in and is passed on every important action.

    *While the referrer is trustworthy, if you just do a blanket check for your domain, if you have user-submitted content, the user could create a link to the resource in question and it would fall within your domain blanket check. In such a situation, a more specific referrer match would fix this issue.

    There are actually a few other things that help you identify a user (but not the specific page the person came from) such as IP addresses, but they have problems of their own.

  10. #10
    SitePoint Wizard wheeler's Avatar
    Join Date
    Mar 2006
    Location
    Gold Coast, Australia
    Posts
    1,369
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    besides the issues highlighted, why not just set a session variable such as $_SESSION['authorized'] on a successful login, and check for that each page load? I don't think there is any advantage to validating the username and password on every page.
    Studiotime - Time Management for Web Developers
    to-do's, messages, invoicing, reporting - 30 day free trial!
    Thomas Multimedia Web Development

  11. #11
    SitePoint Evangelist Tapan's Avatar
    Join Date
    May 2005
    Location
    India
    Posts
    563
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    So:

    wheeler: you mean to say that in login.php i just check it once and set the $_SESSION["authorized"] = "1"; and then keep on checking in access.php whether its 1 or not.

    SituationSoap: What is parameterized sql ? Please give example.

    sk89q: What to do ?!

    Thanks.

  12. #12
    . shoooo... silver trophy logic_earth's Avatar
    Join Date
    Oct 2005
    Location
    CA
    Posts
    9,013
    Mentioned
    8 Post(s)
    Tagged
    0 Thread(s)
    I'll also let you in on a little secret.
    The password value doesn't need to be escaped.
    After you run it though a hash it will only contains a-f 0-9.

    Also when taking a password input, do not filter or escape it.
    Send it directly to the hashing algorithm! Filtering and escaping is not an issue now.
    Logic without the fatal effects.
    All code snippets are licensed under WTFPL.


  13. #13
    SitePoint Enthusiast
    Join Date
    Jul 2005
    Location
    Norway
    Posts
    88
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    If you only allow the characters a-z A-Z 0-9 and maybe a few other characters you could make the username pretty safe from sql-injection by doing a preg_match('/^[a-zA-Z0-9]+$/', $username) on the username. This way, they won't be able to sneak in some special characters like ' or --

  14. #14
    SitePoint Wizard cranial-bore's Avatar
    Join Date
    Jan 2002
    Location
    Australia
    Posts
    2,634
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Also when taking a password input, do not filter or escape it.
    Send it directly to the hashing algorithm! Filtering and escaping is not an issue now.
    I'd recommend running trim() because often if someone uses the clipboard to paste a password there will be a space added to the end (depending on where it was copied from).
    Of course you need to trim() when the password is initially set to be consistent and avoid locking people out of their accounts.

  15. #15
    SitePoint Evangelist Tapan's Avatar
    Join Date
    May 2005
    Location
    India
    Posts
    563
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Now my main concern is on how to save this script from getting attacked using session hijacking ? Any solution to that ?

  16. #16
    SitePoint Wizard
    Join Date
    Mar 2008
    Posts
    1,149
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Make sure that your PHP settings specify that sessions must use cookies and only cookies to persist the session.

    Don't remember the INI settings off my head.


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
  •