Page 1 of 1
secure login redux?
Posted: Fri Apr 15, 2005 12:32 am
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;
}
?>
Posted: Fri Apr 15, 2005 4:23 am
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

Posted: Fri Apr 15, 2005 7:20 am
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.
Posted: Sat Apr 16, 2005 2:26 am
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?
Posted: Sat Apr 16, 2005 6:41 am
by feyd
- isset()
- 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.
- search the forums.
Posted: Sat Apr 16, 2005 5:44 pm
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?
Posted: Sat Apr 16, 2005 5:58 pm
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.
Posted: Sat Apr 16, 2005 6:43 pm
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.
Posted: Mon Apr 18, 2005 10:41 pm
by pella.d
Anyone? Any thoughts on that last revision?
Posted: Tue Apr 19, 2005 6:41 am
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.
Posted: Tue Apr 19, 2005 7:12 am
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.