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:


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]

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


$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"? :D

[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 (http://www.phpbuilder.com/board/showthread.php?s=&postid=10441321#post10441321);)

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

[seby]

yeah, at first i was selecting the userid,password from the db the comparing them to the cookie.

i went ahead and remove the password from the cookie.
at some point i will add sessions since not all users accept cookie, just an alternative.

thanx.

[seby]

can someone help me add sessions to the script, I would like to release a member system which is 98% complete but would like to use sessions and cookies in case the user does not accept cookies.

unless i misunderstand sessions, wouldn't i have to store the session in the cookie? i mean if i was to use sessions, how will i be able to know the users id? or will i have to select username from database instead of selecting userid?

will there be any conflicts using both cookies and sessions?

[BuzzLY]

Sessions are completely compatible with cookies. The way to look at it is this:

Sessions are used to store values that are needed throughout the session. Once you close your browser (all instances of the browser if you have more than one open), the session is closed, and you no longer have access to those values if you reopen the browser and try to access them.

Sessions do NOT require cookies. The session values are stored on the server, and a session cookie is created on the user's computer. That cookie is called PHPSESSID, and is a single value, a unique code that identifies your session. If the user has cookies disabled (grrrrr), then the session id is passed through the URL on all links on your page.

Now... if you want to remember information between sessions (such as a user's username and password), then you need to store that info in cookies. When the user hits a page that requires them to be logged in, your logic goes something like this:
start the session
check the "loggedin" session variable
if not logged in, check for cookie variables
if there are cookies, set the session variables to the cookie values, then set the session "loggedin" variable to TRUE
if there are no cookies, and the user is not logged in, then redirect the user to the login page.
after login is complete (and verified), set session variables and cookies as necessary.
The cool thing about this is that you can assign access levels, then use those access levels to restrict access to certain areas of the site (such as admin functions, or "premium members only" areas.

[daveyboy]

Now... if you want to remember information between sessions (such as a user's username and password), then you need to store that info in cookies. When the user hits a page that requires them to be logged in, your logic goes something like this:

NO PASSWORDS in cookies!!!

if you want users to stay logged in beyond the duration of a php session (usually about 60 mins/till the browser is closed), you need to build a twin-level login system, whereby users can maybe post messages or use members-only functions without giving their password, but will need to have logged in this session to access more sensitive functions, say changing their password. any user-related data you need to store between php sessions, u will either have to dump in cookies or, better yet, in a db or file on the server using custom session code.

[coditoergosum]

I'm not sure if I saw anyone suggest logging the IP address :eek: This solves the "I don't want this person to post so I'll log in 3 times and lock them out" problem, and the "steal your cookie" problem, as well as just generally increase security.

What I do, and suggest you do as well, is insert all of the login information into a table in a database (a secure database mind you... what's the point in putting security info in an insecure DB?). Then store a cookie on the user's machine with just the username (encrypted for a little more security). Then the script basically gets the cookie, attempts to fetch the username details (password, IP, mother's maiden name, etc.), and checks the saved IP against the $_SERVER[REMOTE_ADDR] string. If everything checks out then display whatever secured content. Chances are you already have a DB and a table setup with usernames, passwords and such for the login part of things... might as well re-use that same table. You could even code it to just prompt for the password if a "good-login, bad IP" condition happens... just to minimize user impact.

Then, you can take it a step further and build a 3-strikes-you're-out system based on IP. Lock out the IP that's doing the mass login attempts, not the login Id.

[Weedpacket]

Originally posted by coditoergosum
I'm not sure if I saw anyone suggest logging the IP address :eek: This solves the "I don't want this person to post so I'll log in 3 times and lock them out" problem, and the "steal your cookie" problem, as well as just generally increase security. And also causes problems for everone whose IP is assigned dynamically, and/or whose ISP has more than one proxy.

[coditoergosum]

Originally posted by Weedpacket
And also causes problems for everone whose IP is assigned dynamically, and/or whose ISP has more than one proxy.

This is true, but necessary when you need tight security. That's why I suggest to just prompt for the password (pre-populate the username) if the IP changes. People with dynamic IPs and/or are being proxies just have to deal with things like that. Dynamically assigned IPs are pretty rare these days anyway... by dynamic I mean an IP is assigned every time you connect... broadband with a "dynamic" IP that changes every 6 months is pretty much static anyway... which brings me to another point, you should also require new passwords after a certain interval (30 - 90 days or so), that way if a password does get compromised somehow eventually they will lose access. You could lessen the impact further by logging each IP they successfully athenticate from, then if they are getting thrown between proxies eventually they'll all (or most of them) get logged.

It's all a question of how secure the system needs to be vs. how usable the system needs to be. If you don't need a high level of security just set a cookie that says "logged_in", unencrypted, and never expires.

[BuzzLY]

Originally posted by daveyboy
NO PASSWORDS in cookies!!!

I agree. When I compare passwords, I compare MD5 hashes. In other words, when they enter the password initially, do an md5 hash on it, and on the one in the database. When the data is posted, only the hash is compared (protects against sniffers). Also, the md5 hash is stored in the cookie, not the actual password.

[silent]

I work for a decently sized web site (around 3,000 users or so) and when I first began writing for them, I didn't know PHP. I learned it on the fly. I didn't know about sessions!!!

here's how I used cookies WITHOUT storing any sensitive information in the cookie -

When the user logged in, I set a "logcode" - which was an MD5'd random string of numbers and letters. I stored this in the cookie, along with the user_id, and put the logcode in the database.

Any page that either required the user to be logged in, or just had optional user functions, included a file called "detectuser.php"

This file checked the logcode in the user's cookie against the one in the database. If they matched (which they should - unless somebody else is trying to monkey with the cookies), then a new logcode was assigned to the cookie and the database.

I found that this was pretty damned secure.

If anybody wants some code, let me know... but I generally recommend using sessions anyway!