Whats wrong with this security script?

**Jerry Stuckle** · Mar 28 '07, 07:45 PM

Re: Whats wrong with this security script?

Nosferatum wrote:

This script is meant to limit access by sessions, using username and
password from mysql db and redirect users after login according to a
given value belonging to each user in the db (10,20,30,40).
>
(the included config is just server settings, the login is just a
login form).
>
The script appear to connect but will not redirect users, it seems
that even with correct login details, it won't validate.
>
this code is in top of each protected page granting access to users
with user level 10:
<?php $allow = array (10);include ("../protect/protect.php"); ?>
>
>
THE SCRIPT (protect.php):
>
<?php
>
session_start ();
>
// --------------------------------THE
VARIABLES---------------------------------- //
>
@include ("config.php ");
>
// ----------------------------------THE CODE
------------------------------------ //
>
function clearance ($user_value, $pass_value, $level_value,
$userlevel_valu e, $table_value, $column1, $column2, $path) { //
Function to see if user can login
>
$check = mysql_query ("SELECT $userlevel_valu e FROM $table_value
WHERE username='$user _value' AND password='$pass _value'"); // Query to
see if user exists
>

You should check to see if $check contains a result set or false (the
latter indicating an error).

$verify = mysql_num_rows ($check);
>
>
>
$get = mysql_fetch_arr ay ($check);
>

Don't try to fetch the array unless the return from mysql_query() is a
result set and mysql_num_rows is 0.

if (count ($level_value) != 0) { // If the allow array contains
userlevels
>
if (in_array ($get[$userlevel_valu e], $level_value) && $verify 0)
{ // Search allow to see if userlevels match
>
$_SESSION['username'] = $user_value; // Register sessions
$_SESSION['password'] = $pass_value; // password
$_SESSION['userlevel'] = $get[$userlevel_valu e];
>
}
//redirect users according to user level
if ($verify 0); {
$row = mysql_fetch_arr ay($check);

You just fetched the array up above. This will attempt to get the
second row in the result set. is this what you want?

}
>
switch($row['userlevel_valu e']) {
case '10':
header("locatio n:/hidden/folder1/index.php");
break;
case '20':
header("locatio n:/hidden/folder2/index.php");
break;
case '30':
header("locatio n:/hidden/folder3/index.php");
break;
case '40':
header("locatio n:/hidden/folder4/index.php");
break;
default:
printf("Invalid username and password<br>\n" );
}
//end redirect
>
>
>
} else {
>
if ($verify == 0) { // If attempt fails then redirect to login page
>
$_SESSION = array();
>
$error = "Sorry, invalig login";
>
@include ("login.php" );
>
exit;
>
}
>
if ($verify 0) { // If attempt is good then register the user
>
$_SESSION['username'] = $user_value;
$_SESSION['password'] = $pass_value;
>
}
>
}
>
}
>
function protect ($level_value, $password_value , $userlevel_valu e,
$table_value, $column1, $path) { // Function to keep pages secure
>
if (!isset ($_SESSION['username'])) { // If session doesn't exist
then get user to login
>
if (isset ($_POST['username']) && isset ($_POST['password'])) {
>
$error = "Sorry, username or password doesnt fit";
>
}
>
$_SESSION = array();
>

$_SESSION is already an array - which you just wiped out. Don't do
this. Unset the appropriate array values if necessary.

@include ("login.php" );
>

Why are you including this twice? Make it a function and include it
once at the top. Then call that function if necessary.

exit;
>
} else { // If user is logged in check to see if session is valid and
that they have the required userlevel
>
$check = mysql_query ("SELECT $password_value , $userlevel_valu e FROM
$table_value WHERE $column1='$_SES SION[username]'"); // Query to see
if user exists
>
$verify = mysql_num_rows ($check);
>
$get = mysql_fetch_arr ay ($check);
>
if ($verify == 0) {
>
$_SESSION = array();
>

Again, don't try to set $_SESSION to an array.

$error = "Something wrong with your login";
>
@include ("login.php" );
>

And a third time?

exit;
>
}
>
if ($verify 0 && count ($level_value) != 0) {
>
if (!in_array ($get[$userlevel_valu e], $level_value)) { // Check to
see if the users userlevel allows them to view the page
>
$error = "Sorry, no access";
>
@include ("login.php" );
>
>

FOUR times?
exit; // Ensure no other data is sent

>
}
>
}
>
>
>
}
>
}
>
if (isset ($_POST['username']) && isset ($_POST['password'])) { // If
user submits login information then validate it
>
clearance ($_POST['username'], $_POST['password'], $allow,
$userlevel, $table, $username, $password, $path);
>
}
>
protect ($allow, $password, $userlevel, $table, $username, $path);
>
mysql_close ($link); // Close the database connection for security
reasons
>
// -----------------------------------THE END
------------------------------------ //
>
?>
>

Just what I saw from a quick glance. There may be more.

--
=============== ===
Remove the "x" from my email address
Jerry Stuckle
JDS Computer Training Corp.
jstucklex@attgl obal.net
=============== ===

**shimmyshack** · Mar 29 '07, 10:55 AM

Re: Whats wrong with this security script?

On 28 Mar, 16:40, "Nosferatum " <John.Ola...@gm ail.comwrote:

This script is meant to limit access by sessions, using username and
password from mysql db and redirect users after login according to a
given value belonging to each user in the db (10,20,30,40).
>
(the included config is just server settings, the login is just a
login form).
>
The script appear to connect but will not redirect users, it seems
that even with correct login details, it won't validate.
>
this code is in top of each protected page granting access to users
with user level 10:
<?php $allow = array (10);include ("../protect/protect.php"); ?>
>
THE SCRIPT (protect.php):
>
<?php
>
session_start ();
>
// --------------------------------THE
VARIABLES---------------------------------- //
>
@include ("config.php ");
>
// ----------------------------------THE CODE
------------------------------------ //
>
function clearance ($user_value, $pass_value, $level_value,
$userlevel_valu e, $table_value, $column1, $column2, $path) { //
Function to see if user can login
>
$check = mysql_query ("SELECT $userlevel_valu e FROM $table_value
WHERE username='$user _value' AND password='$pass _value'"); // Query to
see if user exists
>
$verify = mysql_num_rows ($check);
>
$get = mysql_fetch_arr ay ($check);
>
if (count ($level_value) != 0) { // If the allow array contains
userlevels
>
if (in_array ($get[$userlevel_valu e], $level_value) && $verify 0)
{ // Search allow to see if userlevels match
>
$_SESSION['username'] = $user_value; // Register sessions
$_SESSION['password'] = $pass_value; // password
$_SESSION['userlevel'] = $get[$userlevel_valu e];
>
}
//redirect users according to user level
if ($verify 0); {
$row = mysql_fetch_arr ay($check);
}
>
switch($row['userlevel_valu e']) {
case '10':
header("locatio n:/hidden/folder1/index.php");
break;
case '20':
header("locatio n:/hidden/folder2/index.php");
break;
case '30':
header("locatio n:/hidden/folder3/index.php");
break;
case '40':
header("locatio n:/hidden/folder4/index.php");
break;
default:
printf("Invalid username and password<br>\n" );
}
//end redirect
>
} else {
>
if ($verify == 0) { // If attempt fails then redirect to login page
>
$_SESSION = array();
>
$error = "Sorry, invalig login";
>
@include ("login.php" );
>
exit;
>
}
>
if ($verify 0) { // If attempt is good then register the user
>
$_SESSION['username'] = $user_value;
$_SESSION['password'] = $pass_value;
>
}
>
}
>
}
>
function protect ($level_value, $password_value , $userlevel_valu e,
$table_value, $column1, $path) { // Function to keep pages secure
>
if (!isset ($_SESSION['username'])) { // If session doesn't exist
then get user to login
>
if (isset ($_POST['username']) && isset ($_POST['password'])) {
>
$error = "Sorry, username or password doesnt fit";
>
}
>
$_SESSION = array();
>
@include ("login.php" );
>
exit;
>
} else { // If user is logged in check to see if session is valid and
that they have the required userlevel
>
$check = mysql_query ("SELECT $password_value , $userlevel_valu e FROM
$table_value WHERE $column1='$_SES SION[username]'"); // Query to see
if user exists
>
$verify = mysql_num_rows ($check);
>
$get = mysql_fetch_arr ay ($check);
>
if ($verify == 0) {
>
$_SESSION = array();
>
$error = "Something wrong with your login";
>
@include ("login.php" );
>
exit;
>
}
>
if ($verify 0 && count ($level_value) != 0) {
>
if (!in_array ($get[$userlevel_valu e], $level_value)) { // Check to
see if the users userlevel allows them to view the page
>
$error = "Sorry, no access";
>
@include ("login.php" );
>
exit; // Ensure no other data is sent
>
}
>
}
>
}
>
}
>
if (isset ($_POST['username']) && isset ($_POST['password'])) { // If
user submits login information then validate it
>
clearance ($_POST['username'], $_POST['password'], $allow,
$userlevel, $table, $username, $password, $path);
>
}
>
protect ($allow, $password, $userlevel, $table, $username, $path);
>
mysql_close ($link); // Close the database connection for security
reasons
>
// -----------------------------------THE END
------------------------------------ //
>
?>

It's just a bit confused right now, with $userlevel_valu e,
$level_value, $get['userlevel_valu e'] and $userlevel - I might have
forgotten one or made one up.

Try sticking to a convention, eg. I would have $arrAllowedLeve ls for
$level_value.
I'm sure you get bored of passing so many things to each function, if
you can logically group the variables passed in it would be easier to
keep track of, can you either use an array of "query details"
$arrQuery =
array('columns_ selected'=>arra y('username','p assword'),'tabl e_name'=>'my_ta ble');
or table details, or just form the query outside and pass it in as a
string, wrapping the logic inside 2 functions when you are including a
file called protect.php seems uneeded for this purpose. You could
start by writing the whole lot as a nice clean script, and think about
wrapping it up later. It might help you get the logic straight.

because of the way you have coded the clearance() query, at the moment
it allows anyone to authenticate without a correct username or
password, and then for this to persist inside the session, allowing
unrestricted access (and other nasties). Remember to use
$var=mysql_real _escape_string( (string)$var[,$link]) before you pass a
value into a query, see the manual for more (I am assuming that config
handles the db link as well)

a couple of other points.
what is $level_value, it seems to be an array and the logic suggests
that you want to use it as "if the user has [at least?] this level,
then let them in" - but what if your users have a greater level than
this? You will have to add each level into the array to allow them
access too.
Why could you not use an integer called $intMinClearanc eLevel, if the
user's level is at least equal to it, they can pass, this is a minor
change but simplifies your logic and the need to add each level into
the array.
Call exit() after a call to header( 'Location: ' . $strAbsoluteUri );
and use the absolute uri, although browsers tend to do well, it could
be ambiguous in certain circumstances (probably more so for the coder
than the browser!)
If you do want to "be efficient" and only include files if and when
they are needed - which as Jerry points out - can make for over
inclusion, use include_once so that php will not include multiple
times!
Don't be disheartened though, it will all come together

Whats wrong with this security script?

Whats wrong with this security script?

Comment

Comment