secure login redux?

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

Post Reply
pella.d
Forum Newbie
Posts: 11
Joined: Mon Apr 11, 2005 3:05 am

secure login redux?

Post by pella.d »

Hey, last time I posted my secure login script, there were numerous holes pointed out.
Before I added a logout function or anything to it i thought i'd get it looked at again.
I had someone help me implement sessions, but i want to know if the method i used is secure and whatnot.
thanks

Code: Select all

<?
session_start();
// Check for session cleared
if ($_SESSION['accessClear'] == 1){
	
}
// If no session, check for posted from login form
else if(isset($HTTP_POST_VARS['username']) && isset($HTTP_POST_VARS['password'])){
	$username = @safeString($HTTP_POST_VARS['username']);
	$password = @safeString($HTTP_POST_VARS['password']);
	$username = @substr($username, 0, ;
	$password = @sha1($password);
	include 'dbconnect.inc.php';
	// do the sql call in here to check to see if the username and password are correct
	$qstr = "SELECT username, password FROM members WHERE username = '".$username."' and password = '".$password."' LIMIT 1";
	$result = mysql_query($qstr);
	if (mysql_num_rows($result)) {
		$_SESSION['accessClear'] = 1;
	} else {
		include 'login.php';
		exit();
	}
// If neither session nor form post then exit to login.
} else {
	include 'login.php';
	exit();
}
//Makes username and password strings safe.
function safeString($targetVariable) {
	$targetVariable = @strip_tags($targetVariable);
	$targetVariable = @stripslashes($targetVariable); 
  	return $targetVariable;
}
?>
malcolmboston
DevNet Resident
Posts: 1826
Joined: Tue Nov 18, 2003 1:09 pm
Location: Middlesbrough, UK

Post by malcolmboston »

looks good to me :)

i see you are checking user input and making sure it cant be injected with bad code.

Looks fine to me, wait for feyd to respond, he'll give you ther definitive answer :D
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

  • line 4 can potentially produce a php core notice. (unknown index..)
  • safeString() may have problems. For example, it appears that a username of "foo'bar" may make your query fail. If this is possible, SQL injection in a major threat.
  • stripslashes() should only be called if magic quotes are on. You'll have to escape single quotes (at least) for the selection to properly work.
pella.d
Forum Newbie
Posts: 11
Joined: Mon Apr 11, 2005 3:05 am

Post by pella.d »

Thanks for the responses,
1. How could I rewrite that properly and still do the same check for the variable. And whats an unknown index error.
2. in the safeString() function I used to have, addSlashes() or something like that, but someone told me it was useless to haev addSlashes() and stripslashes in the same function, which seemd to make sense to me. What do you think the proper functions would be to use?
3. Magic quotes?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

  1. isset()
  2. you need to check if magic quotes are on. (get_magic_quotes_gpc()). If they are, stripslashes. Otherwise, continue on like normal. In the end, you need to replace single quotes with either two single quotes or an escaped single quote. This will allow mysql to make that single quote apart of the string you are sending, instead allowing SQL injection.
  3. search the forums.
pella.d
Forum Newbie
Posts: 11
Joined: Mon Apr 11, 2005 3:05 am

Post by pella.d »

ok so i did a check for magic quotes, and magic_quotes_gpc() is on. So that means I should leave stripslashes in the script. But how can I replace single quotes with two single quotes?

edit: I just read an article on replacing quotes, and it says to never use stripslashes.
And can I not just escape single quotes with addslashes?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

hint: look for a function that works on strings.. that replaces things.

And, stripslashes() should be dynamically run if magic quotes are on. Not explicitly run, because your current server configuration has it on... it could turn off suddenly.
pella.d
Forum Newbie
Posts: 11
Joined: Mon Apr 11, 2005 3:05 am

Post by pella.d »

Ok hopefully this is the last revision

Code: Select all

<?
session_start();
// Check for session cleared
if (isset($_SESSION['accessClear'])){
	
}
// If no session, check for posted from login form
else if(isset($HTTP_POST_VARS['username']) && isset($HTTP_POST_VARS['password'])){
	$username = @safeString($HTTP_POST_VARS['username']);
	$password = @safeString($HTTP_POST_VARS['password']);
	$username = @substr($username, 0, ;
	$password = @sha1($password);
	include 'dbconnect.inc.php';
	// do the sql call in here to check to see if the username and password are correct
	$qstr = "SELECT username, password FROM members WHERE username = '".$username."' and password = '".$password."' LIMIT 1";
	$result = mysql_query($qstr);
	if (mysql_num_rows($result)) {
		$_SESSION['accessClear'] = 1;
	} else {
		include 'login.php';
		exit();
	}
// If neither session nor form post then exit to login.
} else {
	include 'login.php';
	exit();
}
//Makes username and password strings safe.
function safeString($targetVariable) {
	$targetVariable = @strip_tags($targetVariable);
	$targetVariable = @stripslashes($targetVariable); 
	$targetVariable = str_replace("'", "''", $targetVariable);
  	return $targetVariable;
}
?>
Now I have a couple questions about what I've done.
1. For this session accessClear variable, is it at all possible for someone to create a session variable or anything themselves? Is the method I used secure?
2. Instead of replacing single quotes with more quotes. Can I just remove them? If the username is only allowed to be letters and numbers in the first place, can I just remove all instances of anythign otherwise? Unless my revision handles the quotes fine.
pella.d
Forum Newbie
Posts: 11
Joined: Mon Apr 11, 2005 3:05 am

Post by pella.d »

Anyone? Any thoughts on that last revision?
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

Generally I like to do the logins through a flag in the user row.
Although sessions are quasi-safe, it is far from bad practice to use them ;)
I just like databases more, as they offer more flexibility and possibilities.
jason
Site Admin
Posts: 1767
Joined: Thu Apr 18, 2002 3:14 pm
Location: Montreal, CA
Contact:

Post by jason »

Before you use either $username or $password in the query, run them through mysql_real_escape_string, please. saveString() doesn't do anything to make the string safer. to use in the query, and a string can still be crafted to get around the protection you try to add.
Post Reply