Page 1 of 2

How secure is this

Posted: Tue Sep 06, 2005 3:04 am
by s.dot
Recently someone has broken into 2 or 3 of my members accounts. I don't know how he/she is doing it, so perhaps someone could take a look at my login code and suggest security improvements.

They could be guessing people's passwords, although that seems a bit unlikely. =/

Code: Select all

// User is logging in from index.php or login.php
if($_POST['action'] == "loggingin")
{
	$uname = strtolower(mysql_real_escape_string(strip_tags($_POST['username'])));
	$password = md5(mysql_real_escape_string(strip_tags($_POST['password'])));
	$unencrypted = $_POST['password'];
	$result = mysql_query("SELECT id FROM users WHERE username = '$uname' AND password = '$password'");
	$affected_rows = mysql_num_rows($result);
	if($affected_rows == 1)
	{
		$result = mysql_query("SELECT id, activated, username AS uuname FROM users where username = '$uname'", $sqlconnect);
		if ($myrow = mysql_fetch_assoc($result))
		{
			if ($myrow['activated'] != "y")
			{
				die("You have not yet activated your account.  Please click on the activation link that was sent to your email when you signed up");
         }
			if($_POST['rememberme'] == "true")
			{
				setcookie("id", $myrow['userid']);
				setcookie("username", $myrow['uuname']);
				$userpass = md5($myrow['uuname'].$myrow['id']);
				setcookie("userpass", $userpass);
				setcookie("username2", $myrow['uuname'], 31536000);
				setcookie("userpass2", $unencrypted, 31536000);
			} ELSE
			{    			
				setcookie("id", $myrow['userid']);
				setcookie("username", $myrow['uuname']);
				$userpass = md5($myrow['uuname'].$myrow['id']);
				setcookie("userpass", $userpass);
				setcookie("username2", "", time()-1);
				setcookie("userpass2", "", time()-1);
			}
			function microtime_float() 
			{ 
				list($usec, $sec) = explode(" ", microtime()); 
				return ((float)$usec + (float)$sec); 
			}
			$time_start = microtime_float();
			mysql_query("UPDATE users SET lastactive = '$time_start' WHERE username = '$uname'");
			$date = date("F jS");
			$date2 = date("g:i A");
			$date3 = "$date at $date2";
			mysql_query("UPDATE users SET lastlogin = '$date3' WHERE username = '$uname'");
  			header("Location: showme.php?u=$uname");
		} else
		{
			echo "Sorry, no records were found! perhaps you have not yet <a href=\"index.php\">registered</a>.";	
		}	
	} else
	{
		setcookie("username2", "", 31536000);
		setcookie("userpass2", "", 31536000);
		header ("Location: login.php?login=error");
	}
}

Posted: Tue Sep 06, 2005 3:18 am
by dbevfat
Well, as far as I can tell, you have two security holes here:

1) you store unencrypted username and password in a cookie!
By this, you first send the username/password at least one more time than necessary. You have to make sure, you only send this valuable information ONCE, that is from the client to server at the moment of login and NEVER more. This is a serious issue if someone would somehow listen to the connection. You could even encrypt it on the client side (with javascript) and only send it encrypted. This way, the plain password would never be transfered between machines.

The other way that hacker would get his hands on these two is the client's cookie. This could be done by either having a physical access to that file, or a simple XSS attack on your site.

2) you store unencrypted password in the database!
A password should be very private. You should encrypt the password BEFORE you store it in the database. Then, when someone logs in, you should encrypt THAT password, too, and then just compare the encrypted values.

I really suggest you dig into http://phpsec.org/, specially into http://phpsec.org/projects/guide/, because you have serious security issues here.

Regards

Posted: Tue Sep 06, 2005 3:23 am
by s.dot
Regarding issue #2, I don't store it unencrypted in the database. The value in the database is md5()'d. And I check the value of their md5()'d $_POST['password'] against the value in the database.

Editing the cookie would do no good for a person, because I have set up a check to see if a user has entered a password along with the cookie.

The reason I store the unencrypted password in a cookie is for the 'remember me' option. Is there a way around storing it unencrypted?

Posted: Tue Sep 06, 2005 3:44 am
by s.dot
Okay, I've stripped all the stuff that has to deal with the 'remember me' option, so I could focus on the essentials. Suggestions on how to improve the security of the following code are well appreciated. :-D

Code: Select all

// User is logging in from index.php or login.php
if($_POST['action'] == "loggingin")
{
	$uname = strtolower(mysql_real_escape_string(strip_tags($_POST['username'])));
     //md5 posted password
	$password = md5(mysql_real_escape_string(strip_tags($_POST['password'])));

     // see if a record can be found where the username and md5()'d password match
	$result = mysql_query("SELECT id FROM users WHERE username = '$uname' AND password = '$password'");
	$affected_rows = mysql_num_rows($result);
	if($affected_rows == 1)
	{
		$result = mysql_query("SELECT id, activated, username AS uuname FROM users where username = '$uname'", $sqlconnect);
		if ($myrow = mysql_fetch_assoc($result))
		{

                // store the users ID
			setcookie("id", $myrow['userid']);
                // store the users username
			setcookie("username", $myrow['uuname']);
                // simple algorhytm used to check the cookie on a different page
			$userpass = md5($myrow['uuname'].$myrow['id']);
			setcookie("userpass", $userpass);
                // all is well, so log them in
  			header("Location: showme.php?u=$uname");
		} 	
	} else
	{
                          // no records were found, log them out
		header ("Location: login.php?login=error");
	}
}

Posted: Tue Sep 06, 2005 4:04 am
by feyd
your back-to-back queries seem a bit redundant, and potential to programmer error. ;)

your cookies are very easy to hack since you store the components needed to create the hash out in the wild... keep them in session variables, not the cookie itself.

Posted: Tue Sep 06, 2005 4:43 am
by dbevfat
scrotaye wrote:Regarding issue #2, I don't store it unencrypted in the database. The value in the database is md5()'d.
My bad, missed that one, sorry.
scrotaye wrote:Editing the cookie would do no good for a person, because I have set up a check to see if a user has entered a password along with the cookie.
Editing is not an issue. All I have to do is steal your cookie somehow and then I'm you!
scrotaye wrote:The reason I store the unencrypted password in a cookie is for the 'remember me' option. Is there a way around storing it unencrypted?
There are ways, but every "remember me" system is insecure.

The simplest would be that upon login you create a token, for that, you could use md5(username + password + some random value). You store that token in the database in the user's record (you'd need a new field for that, of course). You also post that token to the client, as a cookie. The coming-back user is then validated by this token rather then by username/password. This, of course, presents the same "cookie theft" issue, but at least you're not sending and storing u/p anywhere. I bet there must be a better way to do it (seen a thread recently, either here or at sitepoint.com), I'm just not that familiar with the topic. First thing that pops to mind is added security like timeout (google style, remember me for 14 days) and locking the token to a specific user agent (a cookie belongs to a specific web-browser), but that might not be a good idea, because user-agent changes at least when the user upgrades his browser. Dunno really.

Regards

Posted: Tue Sep 06, 2005 4:52 am
by Weirdan
original snippet did allow response-splitting attack via setting the cookie to user-supplied value [setcookie("userpass2", $unencrypted, 31536000);]

Posted: Fri Sep 09, 2005 3:11 pm
by php_wiz_kid
MD5 hashed passwords can be easily cracked:
"Sporting over 12 million entries, project GDataOnline is one of the largest non-RainbowTable based MD5 crackers on the internet. The database spans over 7 languages, 35 topics, and contains common mutations to words that include numbers and capitalization. Average crack time for 5 hashes: .04 seconds. No more waiting weeks for your results!"
http://it.slashdot.org/article.pl?sid=05/08/21/1946254

Devnetwork says our best bet is using the crypt() function. I personally use a script that hashes in SHA256. I don't know if that's anymore secure though. I don't really know how to fix your problem, but the random value for the remember me option seems like the best bet.

Posted: Fri Sep 09, 2005 3:21 pm
by dbevfat
md5 passwords can't be easily cracked. If you use some salt together with the password, you'll give a lot of work to a possible cracker.

Posted: Fri Sep 09, 2005 3:27 pm
by feyd
don't get into an md5 discussion, we've only talked about it, and its inherant security or lack thereof in several of their own threads.. we don't need to hijack this thread for it.

Posted: Fri Sep 09, 2005 3:34 pm
by php_wiz_kid
Sorry guys, I was just telling what I read. Thought it might be of some help.

Posted: Fri Sep 09, 2005 5:59 pm
by Roja
php_wiz_kid wrote:Devnetwork says our best bet is using the crypt() function. I personally use a script that hashes in SHA256. I don't know if that's anymore secure though. I don't really know how to fix your problem, but the random value for the remember me option seems like the best bet.
Don't speak for a network of thousands of people with unique and different opinions. If anything, most of the threads discussing it have NOT said that. In fact, most of the threads discussing the reduced security of md5 here suggest moving to sha256. Crypt has multiple options: MD5 (same problem), DES (less secure), and Blowfish (arguably more secure than md5 but less than sha256).

Further, you didn't understand what you quoted/read either. MD5 hashed passwords cannot be easily "cracked". IF you are not using a salt (which would be the wrong way to use md5), and you are not using a complex password, then IF that exact simple password was already computed in a lookup/rainbow table, then it can be looked up. That is a completely different thing from CRACKING a hash.

Please, take the advice of feyd, if you want to bone up on md5, do a search. You are definitely not on the same page as most of "Devnetwork", so shouldn't speak for the non-uniform collection of opinions here. :)

But, regardless, if anyone has questions about md5 and hashing functions, feel free to open a new thread, or PM me. I'm happy to clear things up.

Posted: Fri Sep 09, 2005 7:05 pm
by php_wiz_kid
I was just trying to help. I posted what I read because I thought it might be of use. I wasn't speaking for anybody. I also appologized for my mistake. Be fair, I have just as much of a right to post my findings as does everyone else in this forum without being made to look stupid. I try to post accruate information but I'm not perfect. I wasn't trying to make a discussion of MD5, I was merely posting what I had read earlier on this forum. Let's be professionals and correct mistakes, not exploit them and point fingers to make others look less intelligent. I'm sorry I misread, or I didn't understand what I read as well as I thought.

Posted: Fri Sep 09, 2005 7:19 pm
by Roja
php_wiz_kid wrote:I was just trying to help. I posted what I read because I thought it might be of use. I wasn't speaking for anybody. I also appologized for my mistake. Be fair, I have just as much of a right to post my findings as does everyone else in this forum without being made to look stupid. I try to post accruate information but I'm not perfect. I wasn't trying to make a discussion of MD5, I was merely posting what I had read earlier on this forum. Let's be professionals and correct mistakes, not exploit them and point fingers to make others look less intelligent. I'm sorry I misread, or I didn't understand what I read as well as I thought.
I wasn't pointing fingers or trying to make you look less intelligent. You used the phrase "Devnetwork says", which does imply you were speaking for others, and thats why I wanted to clear up the misconceptions. I've often been wrong, and I'm the first to admit it.. so no, I wasn't trying to single you out, I was trying to clarify the issue and make clear that wasn't the voice of all of devnetwork.

Sorry if I was harsh.. not my intention.

Posted: Fri Sep 09, 2005 10:28 pm
by neophyte
The ol' hide the password game. Everytime I'm in a spot where I'm thinking "gotta have password", I think how else can I do this? Tokens mixed with stored unique date in an md5 is a great work around.