Is this secure login script ?

[Tapan]

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:

<?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:

<?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.

[guido2004]

What does the function fix() do?

[elduderino]

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

[crmalibu]

There's many potential problems.

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

[Tapan]

Hi,

Fix is this:

function fix($value)
{
        if (get_magic_quotes_gpc())
        {
                $value = stripslashes($value);
        }

        return "'" . mysql_real_escape_string($value) . "'";
}

Thanks!

[SituationSoap]

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.

[sk89q]

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.

[Itch]

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?

[sk89q]

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.

[wheeler]

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.

[Tapan]

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.

[logic_earth]

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.

[oeyvind]

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 --

[cranial-bore]

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.

[Tapan]

Now my main concern is on how to save this script from getting attacked using session hijacking ? Any solution to that ?

[sk89q]

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.