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.
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
<?
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;
}
?>
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.
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?
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.
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?
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.
<?
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.
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.
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.