Is this code secure?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Darkzero
Forum Newbie
Posts: 18
Joined: Thu Oct 25, 2007 8:48 pm

Is this code secure?

Post 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 :)
User avatar
Zoxive
Forum Regular
Posts: 974
Joined: Fri Apr 01, 2005 4:37 pm
Location: Bay City, Michigan

Post by Zoxive »

Security is even worse now that you posted your Mysql Server and password.
Darkzero
Forum Newbie
Posts: 18
Joined: Thu Oct 25, 2007 8:48 pm

Post by Darkzero »

..Except for the fact that those are fake.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Path disclosure ability with regards to blindly stripslashing $_POST.
Darkzero
Forum Newbie
Posts: 18
Joined: Thu Oct 25, 2007 8:48 pm

Post 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.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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."
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post 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.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
Darkzero
Forum Newbie
Posts: 18
Joined: Thu Oct 25, 2007 8:48 pm

Post 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);
?>
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post 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.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

take a look through the various posts in this thread: viewtopic.php?t=74859&highlight=stripslashes
Darkzero
Forum Newbie
Posts: 18
Joined: Thu Oct 25, 2007 8:48 pm

Post 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?
Last edited by Darkzero on Sat Oct 27, 2007 10:41 pm, edited 1 time in total.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post 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);
Darkzero
Forum Newbie
Posts: 18
Joined: Thu Oct 25, 2007 8:48 pm

Post by Darkzero »

oh okay, thanks :]
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post 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 ;)
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post 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?
(#10850)
Post Reply