Not sure why this won't redirect the user

[Adam93]

Hi guys,

The idea of this script is to check if the website is under maintenance, and if it is, direct the user to the maintenance page. If the user is signed in, and has privileges to bypass the maintenance page and continue to look at the website, then they won't be redirected, but I can't seem to get it to work? I've checked all the tables in database, I've been through the code multiple times. It's not displaying an error, it's just running through the code and then doing nothing?

 

(Maintenance has already been set to '1' in the database)

 

index.php

<?php
include "core.php";
?>
<?
$updatecounter = mysql_query("UPDATE webviews SET count = count+1 WHERE id = '1'");
if (!$updatecounter) {
die ("Can't update the counter : " . mysql_error());
}
?>

core.php

<?php
session_start();
include "config.php";
include "security.php";
$id   = $_SESSION['userinfo']['id'];
if ($_SESSION['logged'] == true) {
	$qry="SELECT * FROM fuserights WHERE userid='$id'";
	$result=mysql_query($qry);
	
	if($result) {
		if(mysql_num_rows($result) == 1) {
		
			$checks = mysql_fetch_assoc($result);
			$access = $checks['bypassmaintenance'];		
			}
			}
	
if($access == 1)
{ 
}else {

$result = mysql_query("SELECT * FROM maintenance") or die(mysql_error());  

while($row = mysql_fetch_assoc($result)) {
 if($row['check'] == 1) {
   header('location:/maintenance/');
 }
}
}
}
?>

config.php

<?php
$server   = "***";
$username = "***";
$password = "***";
$db_name  = "***";
$connect = mysql_connect($server, $username, $password) or die(mysql_error());
mysql_select_db($db_name, $connect) or die(mysql_error());
mysql_query("SET NAMES utf8");
?>

security.php

<?php
$array = array(
    "union",
    "sql",
    "mysql",
    "database",
    "cookie",
    "coockie",
    "select",
    "from",
    "where",
    "benchmark",
    "concat",
    "table",
    "into",
    "by",
    "values",
    "exec",
    "shell",
    "truncate",
    "wget",
    "/**/",
	"1=1",
	"xss"
);
foreach ($array as $d) {
    $string = security($_SERVER['QUERY_STRING']);
    if (strpos(strtolower($string), $d) != false) {
        $ip         = $_SERVER['REMOTE_ADDR'];
        $loc        = $_SERVER['PHP_SELF'];
        $browseros  = $_SERVER['HTTP_USER_AGENT'];
        $oslanguage = $_SERVER['HTTP_ACCEPT_LANGUAGE'];
        $date       = date("d.m.Y / H:i:s");
        $file       = security('' . $loc . '?' . $string . '');
        $type       = "SQL Injection";
        $queryvalid = mysql_query("SELECT * FROM `hacker-attacks` WHERE file='$file' and type='SQL Injection' LIMIT 1");
        $validator  = mysql_num_rows($queryvalid);
        if ($validator > "0") {
            echo '<meta http-equiv="refresh" content="0;url=index.php" />';
            exit();
        } else {
            $log    = "INSERT INTO `hacker-attacks` (ip, date, file, type, browseros, oslanguage) VALUES ('$ip', '$date', '$file', '$type', '$browseros', '$oslanguage')";
            $result = mysql_query($log);
            echo '<meta http-equiv="refresh" content="0;url=index.php" />';
            exit();
        }
    }
}

function security($input)
{
    $input = mysql_real_escape_string($input);
    $input = strip_tags($input);
    $input = stripslashes($input);
    return $input;
}

$guestip     = $_SERVER['REMOTE_ADDR'];
$querybanned = mysql_query("SELECT * FROM `bans` WHERE ip='$guestip'");
$banned      = mysql_num_rows($querybanned);
$row         = mysql_fetch_array($querybanned);
$reason      = $row['reason'];
if ($banned > "0") {
    die("<center><font size='7' color='red'><b>You are banned</b></font><br>
Reason: $reason<br>  <br /><img src='images/banned.png' /></center>");
}
?>

Any help would be appreciated!

Thanks.

[cyberRobot]

The PHP manual says to add an exit statement after calls to the header() function to prevent further code from executing. So this:

if($row['check'] == 1) {
    header('location:/maintenance/');
}

 

Should be this:

if($row['check'] == 1) {
    header('location:/maintenance/');
    exit;
}

 

 

If that doesn't fix the issue, have you tried adding debugging statements to see what aspects are being executed? For example, you could comment out the header() function and add an echo statement to see if your if test is passing when it should.

if($row['check'] == 1) {
    echo '<div>Execute maintenance redirect</div>';
    //header('location:/maintenance/');
    exit;
}

 

[Adam93]

Thank's for the reply,

 

So after adding the exit statement, the script still wasn't working. Also tried with the debugging statement you gave as an example, but still no luck. Just getting a blank index.php page. Really bizarre. 

[cyberRobot]

Is PHP set to display all errors and warnings? Adding the following debugging lines to the top of your script should make sure errors and warnings are being displayed:

<?php
//REPORT ALL PHP ERRORS
error_reporting(E_ALL);
ini_set('display_errors', 1);
?>

 

 

Also, the debugging statement was an example. You can add these statements throughout the script to see where the program is getting stuck. The statements can also be used to view the contents of the variables which may be helpful for tracking down the bug.

 

Of course, you may want to start at the beginning of your script with the debugging statements. Once you know the first one works, move on to the next and continue until the problem is found. You may want to remove or comment out the debugging statements when they are no longer necessary. That way it's easier to remove them when the program is fixed.

[Adam93]

Oh, I'm now getting the following error;

Notice: Undefined index: userinfo in /home/public_html/core/core.php on line 7

Notice: Undefined index: logged in /home/public_html/core/core.php on line 8

Notice: Undefined index: logged in /home/public_html/index.php on line 3

Never seen this error before. Any ideas?

[Ch0cu3r]

You are getting those notices because those array keys/indexes does not exist in the $_SESSION array. How is your $_SESSION data set?

[Psycho]

Some tips to start out with:

 

Never ASSUME which branch of your code is being executed. Any time you have branching logic (e.g. If/Else) and you are not getting the results you expect - Test It!

 

Also, put comments in your code. Yes, it may make sense while you are writing it, but comments add a lot of value. When someone else is reviewing your code (such as those you are asking help from) it save a lot of time to read the comment and understand the context of the following code rather than having to look at the variables or functions and then backtrack through the code to see what the values are or what the functions do. Plus, when you have to go back into the code weeks, months, years later it will save you a lot of time as well.

 

Indent your code to show the logical structure. I can see you did indent, but it is not very well structured (e.g. the closing brackets at the end for one example).

 

Don't query "*" when you aren't using all the data.

 

What is this?

 

if($access == 1)
{ 

}else {
    //Code follows

 

Why not just use

 

if($access != 1)
{ 
   //Put code here
}

 

Or, since I assume the value of $access will be either 1 or 0, you could just use

 

if(!$access)
{ 
   //Put code here
}

 

There are several gaps i the logic. I would suggest actually drawing the logic out on paper in a flow chart type format. This helps to ensure you cover all eventualities. For example, I'm not sure why you check the user ID in the session and the 'logged' value in the session? If you have a user ID in the session data doesn't that mean the user is logged in? Seems multiplicative.

 

Plus, you should do the most important things first. The current logic checks if the user has rights to override maintenance before you check if the environment is in maintenance. Why not check if the environment is in maintenance first? If not, you can bypass the rest of the logic? Plus, since this is a maintenance mode script, you need to assume you are in maintenance mode if any query fails (i.e. the DB could be down).

 

Here is a quick and dirty rewrite (not tested)

 

<?php
 
//Set to false to suppress debugging messages
$__DEBUG = true;
 
session_start();
include "config.php";
include "security.php";
 
//Create function to redirect
function gotoMaintenance()
{
    header('location:/maintenance/');
    exit;
}
 
//Check if environment is in maintenance mode
$query = "SELECT check FROM maintenance";
$result = mysql_query($query);
//If query failed assume maintenance mode
if(!$result)
{
    if($__DEBUG) { echo "Query: {$query}<br>Error: " . mysql_error(); exit(); }
    gotoMaintenance();
}
 
//Check maint mode value
$server = mysql_fetch_assoc($result);
//Check if server is in maint mode.
if($server['check'])
{
    //Set id to session value if set, false otherwise
    $id = isset($_SESSION['userinfo']['id']) ? $_SESSION['userinfo']['id'] : false;
    //Check if user has override
    $query = "SELECT bypassmaintenance FROM fuserights WHERE userid='$id'";
    $result = mysql_query($query);
    //If query failed assume maintenance mode
    if(!$result)
    {
        if($__DEBUG) { echo "Query: {$query}<br>Error: " . mysql_error(); exit(); }
        gotoMaintenance();
    }
    
    //Check user override status
    $user = mysql_fetch_assoc($result);
    if(!$user['bypassmaintenance'])
    {
        //User does not have override status
        if($__DEBUG) { echo "Server is in maint mode and user does not have override status."; exit(); }
        gotoMaintenance();
    }
}
 
//If the code reaches this point . . . 
//Server is not in maintenance mode or user has maint mode override
 
?>

[Jacques1]

There's actually a quick way to improve the code quality and maybe even solve the problem: Delete that terrible “security” script.

 

I'm actually pretty sure this is just a prank to make fun of people who steal code from the Internet. I mean, an SQL injection vulnerability in a function against SQL injections? That can't be true. The rest is also pretty funny: So whenever the English word “by” occurs in the URL, then I'm reported as an evil hacker and thrown out, but it's perfectly fine to perform a full-blown SQL injection through POST parameters or cookies?

 

Like I said, it's a prank. It was funny, but now it's time to become serious.