How secure is this

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

How secure is this

Post 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");
	}
}
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
dbevfat
Forum Contributor
Posts: 126
Joined: Tue Jun 28, 2005 2:47 pm
Location: Ljubljana, Slovenia

Post 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
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post 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?
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post 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");
	}
}
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
User avatar
dbevfat
Forum Contributor
Posts: 126
Joined: Tue Jun 28, 2005 2:47 pm
Location: Ljubljana, Slovenia

Post 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
User avatar
Weirdan
Moderator
Posts: 5978
Joined: Mon Nov 03, 2003 6:13 pm
Location: Odessa, Ukraine

Post by Weirdan »

original snippet did allow response-splitting attack via setting the cookie to user-supplied value [setcookie("userpass2", $unencrypted, 31536000);]
php_wiz_kid
Forum Contributor
Posts: 181
Joined: Tue Jun 24, 2003 7:33 pm

Post 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.
User avatar
dbevfat
Forum Contributor
Posts: 126
Joined: Tue Jun 28, 2005 2:47 pm
Location: Ljubljana, Slovenia

Post 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.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
php_wiz_kid
Forum Contributor
Posts: 181
Joined: Tue Jun 24, 2003 7:33 pm

Post by php_wiz_kid »

Sorry guys, I was just telling what I read. Thought it might be of some help.
Roja
Tutorials Group
Posts: 2692
Joined: Sun Jan 04, 2004 10:30 pm

Post 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.
php_wiz_kid
Forum Contributor
Posts: 181
Joined: Tue Jun 24, 2003 7:33 pm

Post 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.
Roja
Tutorials Group
Posts: 2692
Joined: Sun Jan 04, 2004 10:30 pm

Post 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.
User avatar
neophyte
DevNet Resident
Posts: 1537
Joined: Tue Jan 20, 2004 4:58 pm
Location: Minnesota

Post 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.
Locked