Page 1 of 2

Is this code secure?

Posted: Thu Oct 25, 2007 8:52 pm
by Darkzero
I'm a super noob at php.. like, just started programming with it about a week ago. I had some help with this, but anyways.. All it is, an HTML form send's POST data to my php page, and the page stores it in mySQL database. The database then displays the user's message on a seperate page. The user's IP is also logged incase someone is to abuse the system.. Does the code properlyprotect form SQL injections?

***** PLEASE USE THE PHP TAG FOR PHP CODE AND DO NOT POST PASSWORDS *****

Code: Select all

<?php
if (!empty($_POST['name']) && !empty($_POST['email']) && !empty($_POST['message']))
{
	$con = mysql_connect("xxxx","xxxx","xxxx");
	if (!$con)
	{
		die('Could not connect: ' . mysql_error());
	}
	mysql_select_db('project_dz');
	$ip = $_SERVER['REMOTE_ADDR'];

	if (get_magic_quotes_gpc())
		$_POST = array_map('stripslashes', $_POST);

	$name = mysql_real_escape_string($_POST['name']);
	$email = mysql_real_escape_string($_POST['email']);
	$message = mysql_real_escape_string($_POST['message']);

	mysql_query("INSERT INTO users SET name = '$name', email = '$email', message = '$message', ip = '$ip'");

	if ( mysql_error() )
	{
		echo "There has been an error\n";
	}
	else
	{
		echo "Info submitted\n";
	}
}
else
{
    echo "Please fill in all the required forms\n";
}
mysql_close($con);
?>

Thanks :)

Posted: Thu Oct 25, 2007 9:25 pm
by Zoxive
Security is even worse now that you posted your Mysql Server and password.

Posted: Thu Oct 25, 2007 9:34 pm
by Darkzero
..Except for the fact that those are fake.

Posted: Fri Oct 26, 2007 12:00 am
by feyd
Path disclosure ability with regards to blindly stripslashing $_POST.

Posted: Sat Oct 27, 2007 12:38 am
by Darkzero
feyd wrote:Path disclosure ability with regards to blindly stripslashing $_POST.
what? o.o




And @ whoever edited my post; that was clearly a fake password and whatnot. The SQL server wasn't even correct.

Posted: Sat Oct 27, 2007 9:11 am
by feyd

Code: Select all

feyd:~ feyd$ php -r "stripslashes(array('foo','bar')); echo PHP_EOL;"
PHP Notice:  Array to string conversion in Command line code on line 1
<span style=color:red>
Notice: Array to string conversion in Command line code on line 1
</span>
The same will happen in your file if data other than scalars are given, except the full path will be disclosed where you see "Command line."

Posted: Sat Oct 27, 2007 11:52 am
by s.dot

Code: Select all

$ip = $_SERVER['REMOTE_ADDR'];
:arrow:

Code: Select all

$ip = !empty($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : '';
REMOTE_ADDR might not always be set. And, since it's vulnerable to changing by the end user, running it through mysql_real_escape_string() probably wouldn't be a bad idea.

Posted: Sat Oct 27, 2007 8:28 pm
by Darkzero
scottayy wrote:

Code: Select all

$ip = $_SERVER['REMOTE_ADDR'];
:arrow:

Code: Select all

$ip = !empty($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : '';
REMOTE_ADDR might not always be set. And, since it's vulnerable to changing by the end user, running it through mysql_real_escape_string() probably wouldn't be a bad idea.
Thanks much :]

and @feyd, How exactly do i fix that..?

Here's my current code, I added a row to my table and fixed what scottayy pointed out.

Code: Select all

<?php
if (!empty($_POST['name']) && !empty($_POST['email']) && !empty($_POST['message']))
{
$con = mysql_connect("~~~~~~~~~~");
if (!$con)
  {
  die('Could not connect: ' . mysql_error());
  }
mysql_select_db('project_dz');
date_default_timezone_set('America/New_York');

$ip = !empty($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : '';
$date = date('D\, d M Y  h:i:s A \(\E\S\T\)');

if (get_magic_quotes_gpc())
$_POST = array_map('stripslashes', $_POST);

$name = mysql_real_escape_string($_POST['name']);
$email = mysql_real_escape_string($_POST['email']);
$message = mysql_real_escape_string($_POST['message']);
$ips = mysql_real_escape_string($ip);
$dates = mysql_real_escape_string($date);

mysql_query("INSERT INTO users SET name = '$name', email = '$email', message = '$message', ip = '$ips', date = '$dates'");

if ( mysql_error() )
{
                echo "There has been an error\n";
}
else
{
                echo "Info submitted\n";
}
}
else
{
    echo "Please fill in all the required forms\n";
}
mysql_close($con);
?>

Posted: Sat Oct 27, 2007 8:58 pm
by John Cartwright

Code: Select all

function stripslashes_recursive(array $source) {
   $stripped = array();
   foreach ($source as $key => $value) {
	  $stripped[$key] = is_array($value) ? stripslashes_recursive($value) : stripslashes($value);
   }
   
   return $stripped;
}
Here's a function I've written that will accomplish what you need. Probably is better to use a non-recursive function though.

Posted: Sat Oct 27, 2007 9:12 pm
by feyd
take a look through the various posts in this thread: viewtopic.php?t=74859&highlight=stripslashes

Posted: Sat Oct 27, 2007 10:39 pm
by Darkzero
Jcart wrote:

Code: Select all

function stripslashes_recursive(array $source) {
   $stripped = array();
   foreach ($source as $key => $value) {
	  $stripped[$key] = is_array($value) ? stripslashes_recursive($value) : stripslashes($value);
   }
   
   return $stripped;
}
Here's a function I've written that will accomplish what you need. Probably is better to use a non-recursive function though.
..how would I be able to manipulate that into my code? Do i just put that in exactly as it is, then replace "mysql_real_escape_string(" with "stripslashes_recursive("?


take a look through the various posts in this thread: viewtopic.php?t=74859&highlight=stripslashes
You two are telling me to do different things lol. What's the difference between what Jcart wants me to do and what you're telling me to do? They both do the same things and are both secure, right?

Posted: Sat Oct 27, 2007 11:05 pm
by John Cartwright
Darkzero wrote:
Jcart wrote:

Code: Select all

function stripslashes_recursive(array $source) {
   $stripped = array();
   foreach ($source as $key => $value) {
	  $stripped[$key] = is_array($value) ? stripslashes_recursive($value) : stripslashes($value);
   }
   
   return $stripped;
}
Here's a function I've written that will accomplish what you need. Probably is better to use a non-recursive function though.
..how would I be able to manipulate that into my code? Do i just put that in exactly as it is, then replace "mysql_real_escape_string(" with "stripslashes_recursive("?
You simply add the function to your script and change

Code: Select all

$_POST = array_map('stripslashes', $_POST);
to

Code: Select all

$_POST = stripslashes_recursive($_POST);

Posted: Sun Oct 28, 2007 12:27 am
by Darkzero
oh okay, thanks :]

Posted: Sun Oct 28, 2007 12:58 pm
by Mordred
scottayy wrote:REMOTE_ADDR might not always be set. And, since it's vulnerable to changing by the end user, running it through mysql_real_escape_string() probably wouldn't be a bad idea.
No, it's not. It always contains the IP address of the client/proxy. But passing it through escaping anyway is a good idea indeed ;)

Posted: Sun Oct 28, 2007 4:43 pm
by Christopher
feyd wrote:take a look through the various posts in this thread: viewtopic.php?t=74859&highlight=stripslashes
feyd, it would be good to have a definitive solution to point to for this -- as this question comes up often. I searched and couldn't find yours. Is yours or jmut's a better implementation?