1
\$\begingroup\$

This is a simple login script. Thought I'm not sure when it's appropriate to use includes over just embedding the code. I'm also not sure if I should make cookies, but I guess I don't really need to right now. Also looking at my code would it be easy to create a 'Remember Me' function?

<?php
$validUser;
$validPass;
$win;
if($_POST)
{
include('admin/functions.php');
// Database Connection
AllYourBase();

if(CheckEmpty($_POST['username']))
{
    $username = $_POST['username'];
    $validUser = true;
}
else
    $validUser = false;

if(CheckEmpty($_POST['password']))
{
    $password = $_POST['password'];
    $validPass = true;
}
else
    $validPass = false;

$query = mysql_query(sprintf("SELECT * FROM Users WHERE username = '%s'", 
                mysql_real_escape_string($username))) or die(exit());
$result = mysql_fetch_assoc($query);

// Split the Salt
$fs = substr($result['salt'], 0, 10);
$ls = substr($result['salt'], -10);

// Add The Salt
$pass = md5($fs.$password.$ls); 
$name = $result['fname']. ' ' . $result['lname'];

// Authenticate 
if($result['password'] == $pass)
{
    session_start();
    $_SESSION['name'] = $name;
    $_SESSION['auth'] = true;
    $win = true;
    // Javascript Redirect not Header()
    Redirect('index.php');
}
else
    $win = false;
}
?>

<html>
<head>

</head>
<body>
<form action="<?php echo $_SERVER['PHP_SELF'] ?>" method="POST">
    <?php if($_POST && $win == false): ?>
        <div class="error">Login Unsuccessful</div>
    <?php endif ?>
        <input type="text" size="10" name="username" /><br />
    <?php if($_POST && $validUser == false): ?>
        <div class="error">Empty Username</div>
    <?php endif ?>
        <input type="password" size="10" name="password" /><br />
    <?php if($_POST && $validPass == false): ?>
        <div class="error">Empty Password</div>
    <?php endif ?>
    <input type="submit" text="Submit" />
</form>
<a href="register.php">Registration Page</a>
</body>
</html>
\$\endgroup\$
  • 1
    \$\begingroup\$ I'd really recommend utilizing OpenID, where you would gain the security of the services you accept (i.e. Twitter, Facebook, Google). The Zend Framework has a module available to assist with this. \$\endgroup\$ – bitsoflogic Sep 13 '11 at 18:59
  • 1
    \$\begingroup\$ Yeah people keep telling me to use the frameworks but I would like to really understand what I'm doing and how things work before jumping into a framework \$\endgroup\$ – Howdy_McGee Sep 13 '11 at 19:10
2
\$\begingroup\$

Rather than an else statement to set false on invalid username/password why not just default to false and only set to true if it worked. Also do you care unless both are valid?

$validUserAndPassword  = false;

if (CheckEmpty($_POST['username']) && CheckEmpty($_POST['password']))
{
    $username = $_POST['username'];
    $password = $_POST['password'];
    $validUserAndPassword = true;
}

The DB is an expensive resources. There is no point in connecting to the DB or even getting stuff from the DB unless you have a valid username and password.

$ result = array();
if ($validUserAndPassword)
{
    AllYourBase();

    $query = mysql_query(sprintf("SELECT * FROM Users WHERE username = '%s'", 
                mysql_real_escape_string($username))) or die(exit());
    $result = mysql_fetch_assoc($query);
}

I am pretty sure that splitting the salt provides little extra entropy.

// Split the Salt
$fs = substr($result['salt'], 0, 10);
$ls = substr($result['salt'], -10);

// Add The Salt
$pass = md5($fs.$password.$ls); 

I think doing authentication your self is a bad idea (this is a specialized field with lots of gotchas). Anway don't you want to spend time on your application rather than doing mundane tasks like authentication?

I am not an expert on front-ends so I have no good recommendations on packages that can do it for you. But I did play around with openid-selector (available via google source), which is the same authentication package that the stack exchange sites use. This allows the users to login-in to your site using the open-id credentials (google/open-id/facebook/yahoo/stack-exchange)).

\$\endgroup\$
  • \$\begingroup\$ Thanks for your help. The reason I check if the username / pass is empty is because IF it's false - it displays an error message at the bottom in HTML that tells them the field is empty. If I set them both to false right out the gate it will display the message no matter what so I'm not sure any other way to do this. \$\endgroup\$ – Howdy_McGee Sep 13 '11 at 15:20
2
\$\begingroup\$

Thought I'm not sure when it's appropriate to use includes over just embedding the code.

It's appropriate to use includes when you have reusable chunks of code that will be or could be used in more than one place in your application. This way, if you need to make a change to a certain chunk of code, you'll only need to make it once (in the included file) and not in every place that you embedded that code. It's always a great idea to keep your code DRY.

I'm also not sure if I should make cookies, but I guess I don't really need to right now. Also looking at my code would it be easy to create a 'Remember Me' function?

You cannot have remember me functionality without using cookies. Basically, you need to add a checkbox to your form. Then, write two separate pieces of code. The first bit of code will create a cooke containing a unique token (obviously, only if the checkbox is in the post data) that is unique to his user account. The second bit will need to check for a known token (stored in your datebase perhaps) and automatically log him in. You can find many examples of this by searching for some php authentication scripts on Google.

One thing that sticks out to me in your code is the die(exit()). Just die() or exit() is sufficient as they are exact equivalents. See here.

As for further improvements, Google is your friend.

Update - Concerning the redirect question in comments

For Redirection, there are pros and cons to each method. PHP's Header() is preferable because it handle's the redirect on the server side. However, if any output has been sent to the browser using Header() will throw an error. In such a case, the javascript method can still be leveraged to redirect the user. The downside to using Javascript for redirection is that javascript can be disabled therefor disabling your redirect.

You could use a combination of the two redirect methods. Something like this perhaps:

<?php

function Redirect($url)
{
    // if headers have been sent, use javascript
    if (headers_sent())
    {
        // write redirect
        echo '<script>window.location = "' . $url . '"</script>';

        // fallback in case javascript is disabled
        echo '<noscript>Click <a href="' . $url . '">here</a> to continue.</noscript>';
    }

    // if header have not been sent, use php to redirect
    else
    {
        header('Location: ' . $url);
        exit;
    }
}
\$\endgroup\$
  • \$\begingroup\$ So having a Javascript Redirect over a Header() Redirect is acceptable? This was a major concern; I started with Header() redirects but was told that it was better to use javascript. \$\endgroup\$ – Howdy_McGee Sep 13 '11 at 19:20
  • \$\begingroup\$ @Howdy_McGee I've updated my answer to address your question. Hope that helps bro! \$\endgroup\$ – Johnny Sep 15 '11 at 18:59
1
\$\begingroup\$

You could retrieve only the fields you need from the database, namely salt, fname, lname, password - provided the current snippet of code.

This way, you won't even have to worry about the extra-load coming along with the other columns of the table "Users" encompassed by the wildcard symbol.

\$\endgroup\$
1
\$\begingroup\$

Take a look at the Stack Exchange questions:

How can I store my user's passwords safely?

What password hash should I use?

Basically, md5 isn't good enough. MD5 is designed to be fast, and that's bad. You really shouldn't write your own password hashing method. In PHP, you should be using crypt:

// Generate hash - This will also make a salt for you
$hashed = crypt($password);

// Check hash - This uses the salt from $hash_from_database as in the input salt
$hashed = crypt($password, $hash_from_database);
if($hashed == $hash_from_database) {
    // success, passwords match
}

Why you should do this

What you're looking for is a key derivation function. Popular examples include PBKDF2, crypt, bcrypt, and the ever-popular "append a salt and hash".

Why is "append a salt and hash" the worst of these?

  • What happens when the password format changes? In crypt and bcrypt, the salt and the hashing algorithm are stored with the hash, so if you change the hash function, you can still check old passwords (with no extra work).

  • How fast is it? If someone gets a hold of your database with password + salt, how long would it take them to break one? The answer is not very long. MD5 is designed to be fast, and that's exactly what you don't want when you're hashing a password. So how many times should you loop? What happens when you need to do change that loop size? Store all old passwords? Keep doing that every time computers get faster?

Use someone else's password system

\$\endgroup\$
  • 1
    \$\begingroup\$ If nobody tried to write their own hashing methods then there would be no hashing methods ;p and what's the point of crypting if you can't decrypt. \$\endgroup\$ – Howdy_McGee Sep 20 '11 at 1:17
  • 1
    \$\begingroup\$ @Howdy_McGee - Not being able to decrypt it is the entire point. You can compare a password to a hashed password though (see lines 5-8 of my post), which is what you want. \$\endgroup\$ – Brendan Long Sep 20 '11 at 1:29
  • \$\begingroup\$ @Howdy_McGee - Obviously some people come up with their own hashing methods, but they know what they're doing and most people don't (you and me included). MD5 is a cryptographic hash function and you're trying to write your own key derivation function. Would you write your own hash function and expect it to be secure? No, you use one that someone who knows what they're doing came up with. With that in mind, what makes you think you know how to write a key derivation function? I'll give some examples of what you're missing in my answer. \$\endgroup\$ – Brendan Long Sep 20 '11 at 1:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.