Quick security check please :)

Craig321 · 16 Mar 2006 at 18:12

Hi,

Can someone check over this quickly for security please? I needs to be as secure as possible as the site it's going on has around 7,000 uniques a day.

Config.php

Code:

<?php

function add_magic_quotes($array) {
	foreach ($array as $k => $v) {
		if (is_array($v)) {
			$array[$k] = add_magic_quotes($v);
		} else {
			$array[$k] = addslashes($v);
		}
	}
	return $array;
}
if (!get_magic_quotes_gpc()) {
	$_GET	= add_magic_quotes($_GET);
	$_POST   = add_magic_quotes($_POST);
	$_COOKIE = add_magic_quotes($_COOKIE);
}

	$host = "localhost";
	$dbuser = "****_****";
	$dbpass = "******";
	$dbname = "****_********";
	
	mysql_connect("$host","$dbuser","$dbpass");
	mysql_select_db($dbname);

?>

Show_News.php
This is the part that will be on the front page:

Code:

<?PHP
include "config.php";

	$sql = "SELECT ID, Title, ShortNews, Date FROM News";
	$result = mysql_query($sql);

	while ($field = mysql_fetch_array($result)) {

	$ID = $field["ID"];
	$Title = $field["Title"];
	$ShortNews = $field["ShortNews"];
	$Date = $field["Date"];
	
?>

<script language="JavaScript">
	function POPUP_NEWS() {
		window.open('show_story.php?id=<?PHP echo $ID; ?> ','EANITHING','toolbar=no,location=no,directories=no,status=yes,menubar=no,resizable=no,copyhistory=no,scrollbars=no,width=500,height=300');
	}
</script>

<?PHP

	echo "
	<a href=\"javascript:POPUP_NEWS()\" onmouseover=\"window.status='Show news'; return true\">$Title</a> - $Date
	<hr>
	$ShortNews
	";

}

?>

Show_Story.php
This is the pop up page:

Code:

<?PHP
error_reporting(0);

include "config.php";

$ID = $_GET['id'];

if($ID == "")
{
	echo "Item ID incorrect";
}
else
{

	$sql = "SELECT Title, LongNews, Date FROM News WHERE ID = $ID";
	$result = mysql_query($sql);

	while ($field = mysql_fetch_array($result)) {

		$Title = $field["Title"];
		$LongNews = $field["LongNews"];
		$Date = $field["Date"];

		echo "
		$Title - $Date
		<hr>
		$LongNews
		";

	}
}

if (mysql_affected_rows() < 1)
{
	echo "Incorrect item ID";
}


?>
<title><? echo $Title; ?></title>

Items can only be posted by administrators, it will be protected with htaccess so no worries there.

Any help is appreciated.

Thanks
Craig.

Craig321 · 16 Mar 2006 at 20:03

Also, just to let you know. This script is read only to public, all they can do is view the small version of the story on the front page and then the full story when they click the title of the news item which will pop up in a new window, they don't have any interaction at all with e.g. comments, posting etc.

Beansprout · 16 Mar 2006 at 20:41

Code:

[b]$ID = $_GET['id'][/b];

if($ID == "")
{
	echo "Item ID incorrect";
}
else
{

	$sql = "SELECT Title, LongNews, Date FROM News WHERE ID = [b]$ID[/b]";

www.php.net/mysql_real_escape_string. Read

Craig321 · 16 Mar 2006 at 21:09

So this:

Code:

function quote_smart($value)
{
    // Stripslashes
    if (get_magic_quotes_gpc()) {
	    $value = stripslashes($value);
    }
    // Quote if not integer
    if (!is_numeric($value)) {
	    $value = "'" . mysql_real_escape_string($value) . "'";
    }
    return $value;
}

in the config.php file and this:

Code:

 		    quote_smart($_POST['blah']),
		    quote_smart($_POST['blah'])

in the query would do the trick?

Craig.

Craig321 · 16 Mar 2006 at 21:17

Right then, now I have this:

Config.php

Code:

<?php
function add_magic_quotes($array) {
	foreach ($array as $k => $v) {
		if (is_array($v)) {
			$array[$k] = add_magic_quotes($v);
		} else {
			$array[$k] = addslashes($v);
		}
	}
	return $array;
}
if (!get_magic_quotes_gpc()) {
	$_GET	= add_magic_quotes($_GET);
	$_POST   = add_magic_quotes($_POST);
	$_COOKIE = add_magic_quotes($_COOKIE);
}

function quote_smart($value)
{
	// Stripslashes
	if (get_magic_quotes_gpc()) {
		$value = stripslashes($value);
	}
	// Quote if not integer
	if (!is_numeric($value)) {
		$value = "'" . mysql_real_escape_string($value) . "'";
	}
	return $value;
}

	$host = "localhost";
	$dbuser = "****_****";
	$dbpass = "********";
	$dbname = "****_********";
	
	mysql_connect("$host","$dbuser","$dbpass");
	mysql_select_db($dbname);

?>

Show_News.php

Code:

<?PHP
include "config.php";


	$sql = "SELECT ID, Title, ShortNews, Date FROM News LIMIT 2";
	$result = mysql_query($sql);

	while ($field = mysql_fetch_array($result)) {

	$ID = $field["ID"];
	$Title = $field["Title"];
	$ShortNews = $field["ShortNews"];
	$Date = $field["Date"];
	
?>

<script language="JavaScript">
	function POPUP_NEWS() {
		window.open('show_story.php?id=<?PHP echo $ID; ?> ','EANITHING','toolbar=no,location=no,directories=no,status=yes,menubar=no,resizable=no,copyhistory=no,scrollbars=yes,width=500,height=300');
	}
</script>

<?PHP

	echo "
	<span class=\"style1\">
		<a href=\"javascript:POPUP_NEWS()\" onmouseover=\"window.status='Show news'; return true\">$Title</a> - $Date
		<hr>
		$ShortNews
	</span><br><br>
	";

}

?>

Didn't think I needed the extra coding in the query above as it doesn't have any WHERE blah??

Show_Story.php

Code:

<style>
.style1 {
	font-family: Verdana;
	font-size: 12px;
}
</style>
<?PHP
error_reporting(0); // Turn error reporting off just incase

include "config.php";

$ID = $_GET['id'];

if($ID == "")
{
	echo "Item ID incorrect";
}
else
{

	$sql = sprintf("SELECT Title, LongNews, Date, Time FROM News WHERE ID = %s", quote_smart($ID));
	$result = mysql_query($sql);

	while ($field = mysql_fetch_array($result)) {

		$Title = $field["Title"];
		$LongNews = $field["LongNews"];
		$Date = $field["Date"];
		$Time = $field["Time"];

		echo "
		<span class=\"style1\">
		<b>$Title - $Date $Time</b>
		<hr>
		$LongNews
		</span>
		";

	}
}

if (mysql_affected_rows() < 1)
{
	echo "Incorrect item ID";
}


?>
<title><? echo $Title; ?></title>

Anything else I could do to make it safer ?

Thanks very much
Craig.

Beansprout · 16 Mar 2006 at 21:22

Looks good methinks. Since $id is a number (I presume) you can use is_numeric() and, depending on what values it can take, you could do some range checking too.

Never, ever trust user input. Always test it

Craig321 · 16 Mar 2006 at 21:28

Thats the one! I was trying to think of the function that checks if it's a number or not, all I could think of was it_int or is_integer, I'll add is_numeric in aswell

Also, no range checking as news could get to 100++

Craig.

Gman · 16 Mar 2006 at 21:35

I could be wrong as i'm not the best at this but wouldn't it be better to use the quote_smart function as soon as you get the variable here:

$ID = $_GET['id'];

insted of waiting till you get down to the the sql query.

I know its probably not needed but IMO by putting it in the sql query itself it tends to make the query less legible to people looking at the code.

Craig321 · 16 Mar 2006 at 21:36

Right then

Code:

if(is_numeric($ID))
{
	queries, more checks etc etc
}
else
{
	echo "ID needs to be a number";
}

That's added and working.

Don't think anyone's going to be getting past that in a hurry, unless I've missed something?

The checks/security:

Checks if id= is numeric
Check if id= is actually filled in
If < 1 affected row then echo an error
Added mysql_escape_string (silly me forgot it)

Also, is there any benefit in using die instead of else or is there no need with my news script?

'#//Edit

Also, is there any simple java/php or whatever it needs I can use to make returns automatically into so when writing a news item a
"

" can just be put in and then on post a would be added?

Thanks
Craig.

Augmented · 16 Mar 2006 at 22:03

Craig321 said:
[..] can just be put in and then on post a would be added?[..]

If you just want a then you can use PHP's nl2br. If you want proper aragraphs, look at autop() (see wpautop() in the Wordpress source).

Craig321 · 16 Mar 2006 at 22:19

Hmm, that will still require me / the other administrator to enter atleast \n to make a new paragraph :S

Craig.

Augmented · 16 Mar 2006 at 22:31

Craig321 said:
Hmm, that will still require me / the other administrator to enter atleast \n to make a new paragraph :S

Craig.

Depends where the input's coming from. If you run nl2br() on content from a <textarea>, for example, then any regular new lines will be converted to - you don't need to add the literal \n.

Craig321 · 16 Mar 2006 at 22:50

Oh, OK, thanks