PhP login security

[Readie]

Right, a while ago I wrote a login script for the administration panel I keep talking about, and it occured to me that it might be a good idea to get it checked over by someone who knows what they're about; so that any changes that need making can be done before people actually start using it.

So, if someone could spare the time I'd very much appreciate it if they could give it a look over and either give it their seal of aproval or condemn it and tell me why :)

---

General info:

Administration panel is all self contained in one file, using $_GET to control the page. Varying permission levels are used and a function is used to output the links based on rank. The code is all commented (with search codes) - and they pop up once or twice in the code i'm copying across

---

The .htaccess rewrites used for the page display are:

RewriteRule ^admin/lostpass/([^/]+)/?$ /admin.php?page=lostpass&lostid=$1 [L]
RewriteRule ^admin/([^/]+)/?$ /admin.php?page=$1 [L]

---

The login script itself is:

$user = mysql_real_escape_string($_POST['username']);
$pass = mysql_real_escape_string($_POST['password']);
 
mysql_connect('localhost','<SNIP>','<SNIP>') or die("Could not connect to server");
mysql_select_db('<SNIP>');
mysql_set_charset('Latin1');
 
if((isset($user) && $user != "") && (isset($pass) && $pass != "")) {
$pass_enc = md5($pass);
$sql = 'SELECT * FROM adminpanel WHERE username="' . $user . '" AND password="' . $pass_enc . '"';
$loginvalid = mysql_query($sql);
 
if($result = mysql_fetch_array($loginvalid)) {
$_SESSION['rank'] = mysql_result($loginvalid,0,"rank");
$_SESSION['valid_user'] = 'valid';
$_SESSION['logged_in_as'] = $user;
$_SESSION['display'] = mysql_result($loginvalid,0,"display");
$_SESSION['email'] = mysql_result($loginvalid,0,"email");
} else {
$_SESSION['valid_user'] = 'invalid';
}
} elseif((isset($user) && $user != "") || (isset($pass) && $pass != "")) {
$fillall = '<td><div class="alert"><span class="r"><b>Error: Please enter a username and a password</b></span></div></td>
</tr>
<tr>';
}

---

Some relevent variable declaration, and a (single) example of page-specific variables are shown below:

$lirank = $_SESSION['rank'];
$livalid = $_SESSION['valid_user'];
$liuser= $_SESSION['logged_in_as'];
$lidisplay = $_SESSION['display'];
$liemail = $_SESSION['email'];
$pageid = mysql_real_escape_string($_GET['page']);
 
 
/* News post writer */
$perm_np_view = '30';
$perm_np_link = 'News post writer';
$perm_np_url = 'newspost';

---

The control for page display (once logged in) is:

if(($livalid == "valid") && ($pageid == $perm_np_url) && ($lirank <= $perm_np_view)) {
$pageidtaken = 'yes';
$title = $perm_np_link;
$cont = '<SNIP>';
}

---

The home page (for logged in users) is:

if(($livalid == "valid") && ($pageidtaken != "yes")) {

---

The password recovery script is:

if($pageid == "lostpass") {
$pageidtaken = 'yes';
$title = 'Password recovery';
 
/* ---------- [03.01] RANDOM STRING GENERATOR ---------- */
 
function genRandStr($minLen, $maxLen) {
$alphaLowerArray = array('a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z');
$numArray = array(0, 1, 2, 3, 4, 5, 6, 7, 8, 9);
 
if (isset($minLen) && isset($maxLen)) {
if ($minLen == $maxLen) {
$strLen = $minLen;
} else {
$strLen = rand($minLen, $maxLen);
}

$finalArray = array_merge($alphaLowerArray, $numArray);
$count = count($finalArray);

$str = '';
$i = 1;
while ($i <= $strLen) {
$rand = rand(0, $count);
$newChar = $finalArray[$rand];
$str .= $newChar;
$i++;
}
return $str;
}
}
 
/* ---------- [03.02] LOSTID TRUE: SQL / VALIDATION / ACTUAL PAGE ---------- */
 
$lostid = mysql_real_escape_string($_GET['lostid']);
if(isset($lostid) && $lostid != "") {
$lost_check_sql = 'SELECT * FROM pwd_recovery WHERE lostid="' . $lostid . '"';
$lost_check_result = mysql_query($lost_check_sql);
if(mysql_fetch_array($lost_check_result)) {
$lost_email = mysql_result($lost_check_result,0,"email");
$lost_new_pass = genRandStr(10, 10);
$lost_new_pass_enc = md5($lost_new_pass);
$lost_new_sql = 'UPDATE adminpanel SET password="' . $lost_new_pass_enc . '" WHERE email="' . $lost_email . '"';
$delsql = 'DELETE FROM pwd_recovery WHERE lostid="' . $lostid . '"';
$lost_user_sql = 'SELECT * FROM adminpanel WHERE email="' . $lost_email . '"';
$lost_user_result = mysql_query($lost_user_sql);
$lost_user = mysql_result($lost_user_result,0,"username");
mysql_query($lost_new_sql) or die('Could not connect to server');
mysql_query($delsql);
$email_subject = 'Admin panel password reset';
$email_headers = 'From: <SNIP>' . 'X-Mailer: php';
$email_body = '<SNIP>';
if(mail($lost_email, $email_subject, $email_body, $email_headers)) {
$lostcont = '<SNIP CONFIRMATION>';
} else {
$lostcont = '<SNIP ERROR>';
}
} else {
$lostcont = '<SNIP ERROR>';
}
 
/* ---------- [03.03] LOSTID FALSE: SQL / VALIDATION / ACTUAL PAGE ---------- */
 
} else {
$lost_email = mysql_real_escape_string($_POST['email']);
if(!isset($lost_email) || $lost_email == "") {
$lostcont = '<div class="cent"><br /><div class="grey"><span><b>Password recovery</b><br /><br />
Enter your E-mail:<br />
<form method="post">
<input type="text" name="email" /><br /><br />
<input type="submit" value="Submit" />
</form></span></div><span class="small"><a href="/admin">Back to admin login</a></span></div>';
} else {
$lost_check_sql = 'SELECT * FROM adminpanel WHERE email="' . $lost_email . '"';
$lost_check_result = mysql_query($lost_check_sql);
if(mysql_fetch_array($lost_check_result)) {
$lost_setid = genRandStr(20, 20);
$lost_checkb_sql = 'SELECT * FROM pwd_recovery WHERE email="' . $lost_email . '"';
$lost_checkb_result = mysql_query($lost_checkb_sql) or die('Could not connect to server');
if(mysql_fetch_array($lost_checkb_result)) {
$lost_setid_sql = 'UPDATE pwd_recovery SET lostid="' . $lost_setid . '" WHERE email="' . $lost_email . '"';
mysql_query($lost_setid_sql) or die('Could not connect to server');
} else {
$lost_setid_sql = "INSERT INTO pwd_recovery (lostid, email) VALUES('$lost_setid', '$lost_email')";
mysql_query($lost_setid_sql) or die('Could not connect to server');
}
$email_subject = '<SNIP>';
$email_headers = 'From: <SNIP>' . 'X-Mailer: php';
$email_body = '<SNIP>';
if(mail($lost_email, $email_subject, $email_body, $email_headers)) {
$lostcont = '<SNIP CONFIRMATION>';
} else {
$lostcont = '<SNIP ERROR>';
}
} else {
$lostcont = '<SNIP ERROR>';
}
}
}
 
}

---

Finally, the actual page is:

if(($pageid == "logout") && ($livalid)) {
session_destroy();
header('Refresh: 2; url="/admin"');
echo $start . $logout;
} elseif($pageid == "logout") {
header('Location: /admin');
} elseif($pageid == "lostpass") {
echo $start . $lostcont;
} else {
echo $start;
if(isset($livalid)) {
if($livalid == valid) {
echo $logtab1 . $cont . $logtab2;
} else {
echo $form1 . $alert . $form2;
}
} else {
echo $form1 . $fillall . $form2;
}
}
echo $end;

---

As I said, it would be awesome if someone who knows their stuff could check this over and give a little feedback.

Also... Is there any way at all of indenting a bit when doing code on these forums? :)

Regards,
Readie

[CyBerAliEn]

This forum does not enabled indenting as far as I know... *tear* :(

I had some difficulty fulling understanding your code and following it; but I think I have a lot of it down.

Only thing I really caught as being possibly "off" is this line of code on your "actual page":

if(($pageid == "logout") && ($livalid)) { 

From your code, it appears you are setting the variable 'livalid' to either 'valid' (string) or 'invalid' (string). But this conditional test is looking for an evaluation of "true" (such as boolean true or even integer 1, etc). I do not believe PHP will evaluate 'valid' as "true". Perhaps this line can be updated so that you are checking for:

$livalid=='valid'

Other notes... what happens if someone (not logged in) tries to access a "restricted" page? From your code, it doesn't seem like there is any sort of "hey, you dont have access"?

Note I did not really bother checking your password recovery.

[Readie]

if(($pageid == "logout") && ($livalid)) {

Oops, that is meant to be

$livalid == "valid"

Thanks for catching that one :)

If a non-logged-in user tries to access a page that requires login then nothing happens - they still see the login form.

Cheers for the time you spent :)

P.S. On not checking the password recovery... Can't say I blame you on that one, it is a bit of a wall of text :)

[Matthew1980]

Hi there Readie,

Just noticed something to point out in your password recovery part:-

$alphaLowerArray = array('a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z');
$numArray = array(0, 1, 2, 3, 4, 5, 6, 7, 8, 9);

can be achieved with:-

$alphaLowerArray = range('a', 'z');
$numArray = range('0','9');

Just saves typing the alphabet out ;-p

MRb

[Readie]

Ahh, thanks Matthew :)

I had no idea that a range() function even existed until I saw it being used in Kings On Steeds' post that I replied to the other day :)