is this secure?

[seby]

Greetings,
I'm looking for some advice on the below code.
I would like to know how dependable does the login code look and if anyone has anything that would improve it in a security stand-point..

I know nothing is 100% secure, but would like to know if anyone sees any flaws.

1) username,userid,password is fetched form the database, if no user is found give the user a userid of 0.

2) if user is not found proceed to let the user login.

3) if user is found then send them to index.php.

code:

PHP Code:

error_reporting(E_ALL &~E_NOTICE);
require('./global.php');

// check if user already has a cookie
$fetchuserinfo = $data->query(
    "SELECT userid
     FROM admins
     WHERE userid = '".$_COOKIE['gameuid']."'");
$userinfo = $data->fetch_array($fetchuserinfo);

if(mysql_num_rows($fetchuserinfo) == 0)
{
    $userinfo['userid'] = 0;
}

if ($userinfo['userid'] != 0)
{
    // if user is already logged in then send them to index.php
    header('Location: index.php');
    exit;
}

if ($_POST['submit'])
{
    $validate = $data->query("SELECT userid,username,password
    FROM `admins` WHERE `username` = '" .
    addslashes(htmlspecialchars($_POST['username'])) . "'");

    // check if username entered is valid
    if (mysql_num_rows($validate) == 0)
    {
        echo 'Username does not exist';
        exit;
    }
    else
    {
        $user = $data->fetch_array($validate);

        // check if password matches
        if(md5($_POST['password']) != $user['password'])
        {
            echo 'Sorry, your password is incorrect.';
            exit;
        }
        else
        {
            setcookie('gameuser', $user['username'], time() + (3600 * 24 * 7));
            setcookie('gameuid', $user['userid'], time() + (3600 * 24 * 7));
            setcookie('gamepass', md5($user['password']), time() + (3600 * 24 * 7));

            // user is now logged in and sent to index.php
            header('Location: index.php');
            exit;
        }
    }
}
else
{
    // login form template code
    eval("echo(\"" . template("loginpage") . "\");");
}


[Thatsmej]

PHP Code:

$fetchuserinfo = $data->query("SELECT userid FROM admins
WHERE userid = '".$_COOKIE['gameuid']."'");


test if you can add a quote in here ( " )

if sow you should add an addslashes here..

i believe it dependse on the server config..

[AstroTeg]

Thumbs up on the md5 hashing.

The only thing I noticed (and its a little too early in the morning for me so it might not be the only thing). Your query:

PHP Code:

$fetchuserinfo = $data->query("SELECT userid FROM admins
WHERE userid = '".$_COOKIE['gameuid']."'");


In theory, a user could create a cookie which looked similar to the cookie your code looks for, but with a user ID chosen by the user instead of one assigned by the site. To make it a little more bullet proof, include the password and the user name in the above select statement. That way if the user tries doctoring some of the cookie data, they'll have to get the user name, password hash, and the user ID just right. This should make it more difficult for the user to just guess at user IDs.

Otherwise, it looks pretty good from where I'm at...

[Shrike]

Use PHP's session support if you really need some tighter security, that way the only data stored on the client is the randomly generated PHPSESSID.

To prevent brute-force hacking, give each user 3 attempts at logging in before locking their account in some way.

Bear in mind that if you allow people to select their own username and password, you could effectively give access details away during the registration process, if you print messages such as 'this username is inuse'

Also giving a user cookie a 1 week lifetime is not very secure (in the case of shared PC's

my2c

[Weedpacket]

Originally posted by Shrike
To prevent brute-force hacking, give each user 3 attempts at logging in before locking their account in some way.

And then, because I don't want you posting here any more, I'll try and log in as you three times and so lock you out

[Shrike]

Thats the price you pay for security

Also only works if you know my username. On a forum ok, but on for e.g. an online bank account, not so likely ;p

[Weedpacket]

Originally posted by Shrike
Thats the price you pay for security

Can you say "Denial of service"?

[Shrike]

Lets just force people to pass an IQ test before being allowed to buy a computer

[procoder]

Why are you storing the password in a cookie? that is very bad.

[DeadFly]

Originally posted by procoder
Why are you storing the password in a cookie? that is very bad.

Agreed.
I usually just store the UserID and UserName in cookies, and nothing more. If I need a password from them (for admin purposes), I make a prompt for it.

[agoe]

he's storing passwords the md5-way : hard to decrypt.

to comment the script:
i would use sessions
i would put both userid/username and password in the same query so you get "wrong username or password" instead of "wrong username" or "wrong password": saves time and server capacity, if only a little bit. plus the security-issues mentioned above.

[Weedpacket]

Originally posted by agoe
he's storing passwords the md5-way : hard to decrypt.

Doesn't matter. I just steal the cookie.

[Weedpacket]

Originally posted by Shrike
Lets just force people to pass an IQ test before being allowed to buy a computer

Been there

[drawmack]

You're relying on cookies and post data. You cannot achieve security while relying on cookies and post data. All you can do is make it harder to hack. Prevent sql insertions and you're okay.

BTW: do not store any version of the password in a cookie. What if someone hacks md5 tomorrow? Besides MD5 isn't all that secure. It's only 128 bits long which means you only really get 64 bits of security from it based on collision attacks. For security you should be using SHA-256.

[Merve]

Don't store the password in a cookie; just match it against the password in the DB...no need to store it any form cookie.