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

danf_1979
Forum Commoner
Posts: 72
Joined: Sun Feb 20, 2005 9:46 pm

Is this code secure?

Post by danf_1979 »

Hi:

I'm beginning to read some security literature regarding PHP. I had never done it before, so you can say I'm pretty newbie in the subject. I want to hear some opinions for this code: Is this secure?

Code: Select all

if ( isset($_POST["do_save"]) ) {
	$clean = array();
	$config = array();
	$fields_length = array();
	
	$fields_length = array(
				"company_name"=> 50, 
				"catalog_title" => 50, 
				"contact_email" => 50,
				"use_cat_images" => 1,
				"categories_img_dir" => 50,
				"products_img_dir" => 50,
				"use_last_box" => 1,
				"front_message" => 255,
				"limit_per_page" => 2,
				);
	foreach ($fields_length as $field => $value) {
		if ( strlen($_POST[$field]) > $value) {
			$CONTENT = "
			<table >
			  <tbody>
			    <tr>
			      <td >ERROR</td>
			    </tr>
			  </tbody>
			</table>
			";	
			$ns -> tablerender(ADMIN_FRONT_00, $CONTENT);
			exit();
		}
	}
	$clean["company_name"] = mysql_real_escape_string($_POST['company_name']);
	$clean["catalog_title"] = mysql_real_escape_string($_POST["catalog_title"]);
	if ( is_email($_POST["contact_email"]) == true ) $clean["contact_email"] = $_POST["contact_email"];
	if ( is_numeric($_POST["use_cat_images"]) ) $clean["use_cat_images"] = $_POST["use_cat_images"];
	if(preg_match("/^[a-zA-Z0-9\-\.\/]{0,255}$/",$_POST["categories_img_dir"])) $clean["categories_img_dir"] = $_POST["categories_img_dir"];
	if(preg_match("/^[a-zA-Z0-9\-\.\/]{0,255}$/",$_POST["products_img_dir"])) $clean["products_img_dir"] = $_POST["products_img_dir"];
	if ( is_numeric($_POST["use_last_box"]) ) $clean["use_last_box"] = $_POST["use_last_box"];
	$clean["front_message"] = mysql_real_escape_string($_POST["front_message"]);
	if ( is_numeric($_POST["limit_per_page"]) ) $clean["limit_per_page"] = $_POST["limit_per_page"];

	$config = array(
				"company_name"=>$clean["company_name"],
				"catalog_title"=>$clean["catalog_title"],
				"contact_email"=>$clean["contact_email"],
				"use_cat_images"=>$clean["use_cat_images"],
				"categories_img_dir"=>$clean["categories_img_dir"],
				"products_img_dir"=>$clean["products_img_dir"],
				"use_last_box"=>$clean["use_last_box"],
				"front_message" => $clean["front_message"],
				"limit_per_page"=>$clean["limit_per_page"],
				);
	$mydb->update("e107_cart_config", $config, "id=1" );
Thanks to anyone who cares responding :)
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

although I don't care for the coding style, nothing security related jumps out. However, you could get some SQL errors if your database abstraction doesn't quote all the values given to it.
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Re: Is this code secure?

Post by timvw »

- I like the use of hashes with values 'cleaned' for a particular use.. That is i find $mysql and $html better choises, because they indicate in which context the values are 'clean'.

- It appears you're using a couple of variables in $_POST without explictely testing if they exists, eg: $_POST[$field].

- What happens if $_POST['limit_per_page'] is a negative number?
danf_1979
Forum Commoner
Posts: 72
Joined: Sun Feb 20, 2005 9:46 pm

Post by danf_1979 »

Yup, I will adopt the $mysql and $html style you suggest. Thanks...

PD: I hadn't thought about someone putting negative numbers =), I'll fix that.
AlecH
Forum Commoner
Posts: 27
Joined: Fri Feb 24, 2006 4:22 pm
Location: New Hampshire

Post by AlecH »

You also forgot to clean some of the $_POST variables that were set.
danf_1979
Forum Commoner
Posts: 72
Joined: Sun Feb 20, 2005 9:46 pm

Post by danf_1979 »

Alech, what do you mean?

Feyd, I thought about what you said about my coding style. After a looking at the code I began to dislike the way I was controling the length of the $_POST[$vars]. I came up with this, that should be better:

Code: Select all

if ( isset($_POST["do_save"]) ) {

	$mysql = array();

	$mysql["company_name"] = clean_input($_POST['company_name']);
	$mysql["catalog_title"] = clean_input($_POST["catalog_title"]);
	$mysql["front_message"] = clean_input($_POST["front_message"]);

	if (isset($_POST["use_cat_images"]) && is_numeric($_POST["use_cat_images"])) {
		$mysql["use_cat_images"] = $_POST["use_cat_images"];
	}
	else {
		e107_fast_error(ERROR_00, "config.php");
	}
	if ( isset($_POST["use_last_box"]) && is_numeric($_POST["use_last_box"])) {
		$mysql["use_last_box"] = $_POST["use_last_box"];
	}
	else {
		e107_fast_error(ERROR_00, "config.php");
	}
	if ( isset($_POST["limit_per_page"]) && is_numeric($_POST["limit_per_page"]) && $_POST["limit_per_page"] > 0) {
		$mysql["limit_per_page"] = $_POST["limit_per_page"];
	}
	else {
		e107_fast_error(ERROR_01, "config.php");
	}
	if(isset($_POST["categories_img_dir"]) && preg_match("/^[a-zA-Z0-9\-\.\/]{0,255}$/",$_POST["categories_img_dir"]) && strlen($_POST["categories_img_dir"]) <= 50) {
		$mysql["categories_img_dir"] = $_POST["categories_img_dir"];
	}
	else {
		e107_fast_error(ERROR_02, "config.php");
	}
	if(isset($_POST["products_img_dir"]) && preg_match("/^[a-zA-Z0-9\-\.\/]{0,255}$/",$_POST["products_img_dir"]) && strlen($_POST["products_img_dir"]) <= 50) {
		$mysql["products_img_dir"] = $_POST["products_img_dir"];
	}
	else {
		e107_fast_error(ERROR_02,"config.php");
	}
	if ( isset($_POST["contact_email"]) && is_email($_POST["contact_email"]) && strlen($_POST["contact_email"] <= 50) {
		$mysql["contact_email"] = $_POST["contact_email"];
	}
	else {
		e107_fast_error(ERROR_03, "config.php");
	}

	$config = array();
	$config = array(
				"company_name"=>$mysql["company_name"],
				"catalog_title"=>$mysql["catalog_title"],
				"contact_email"=>$mysql["contact_email"],
				"use_cat_images"=>$mysql["use_cat_images"],
				"categories_img_dir"=>$mysql["categories_img_dir"],
				"products_img_dir"=>$mysql["products_img_dir"],
				"use_last_box"=>$mysql["use_last_box"],
				"front_message" => $mysql["front_message"],
				"limit_per_page"=>$mysql["limit_per_page"],
				);
	$mydb->update("e107_cart_config", $config, "id=1" );
	$CONTENT .= "
	<table class='fborder' style='text-align: left; width: 90%; margin-top:20px;margin-left:auto;margin-right:auto;' border='1' cellpadding='0' cellspacing='0'>
	  <tbody>
	    <tr>
	      <td colspan='1' rowspan='1' class='forumheader' style='text-align: center;'>".ADMIN_FRONT_07."</td>
	    </tr>
	  </tbody>
	</table>
	";	
}
Thanks for your comments, this is a very interesting topic for me, I'm having fun.
Last edited by danf_1979 on Wed Mar 22, 2006 10:04 pm, edited 1 time in total.
danf_1979
Forum Commoner
Posts: 72
Joined: Sun Feb 20, 2005 9:46 pm

Post by danf_1979 »

This is function clean_input:

Code: Select all

function clean_input($data) {
    $data = htmlspecialchars($data);
    $data = mysql_real_escape_string($data);
    return $data; 
}
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

Be careful of context - do you need to entitise text being sent to MySQL? Or does it only need escaping? Don't entitise database values unless it's required for a specific reason.

e.g.

Code: Select all

$_POST['name'] = 'Pádraic';

$clean = array();
$html = array();
$sql = array();

if(isset($_POST['name']) && !empty($_POST['name'])) //asuming you don't want empty values (0, false, null)
{
   // do other checks, maxlength/value range/string structure before this; then
   $clean['name'] = $_POST['name'];
   $html['name'] = htmlentities($clean['name'], ENT_QUOTES, 'UTF-8');
   $sql['name'] = mysql_real_escape_string($clean['name']);
}

echo 'Name is <strong>' . $html['name'] . '</strong>!';
$rs = do_query("insert into users (name, timestamp) values ('" . $sql['name'] . "', " . time() . ")");
Just some sample code showing the split between contexts - you can just create some functions to do this automatically, or use a function accepting rules to automatically cover standard input filtering.
danf_1979
Forum Commoner
Posts: 72
Joined: Sun Feb 20, 2005 9:46 pm

Post by danf_1979 »

Ok Maugrim, thanks for that info. I am using characters like á, ó, í, ¿, etc (spanish), so I guess i need to use htmlentities...
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

If that's the case, I suggest looking into the possibility of UTF-8.
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

$html['name'] = htmlentities($clean['name'], ENT_QUOTES, 'UTF-8');
;). Default presumption since PHP is UTF-8 ignorant. It also encompasses most contingencies such as lower level UTF encodings among other things. Character encoding is important for entitising in PHP.
danf_1979
Forum Commoner
Posts: 72
Joined: Sun Feb 20, 2005 9:46 pm

Post by danf_1979 »

So I should use something like this before entering data to mysql?

$mysql['name'] = htmlentities($clean['name'], ENT_QUOTES, 'UTF-8');
danf_1979
Forum Commoner
Posts: 72
Joined: Sun Feb 20, 2005 9:46 pm

Post by danf_1979 »

For example, I'm using this to validate the name, before getting the data to mysql:

Code: Select all

if (isset($_POST["name"]) && !empty($_POST["name"]) && ctype_alnum($_POST["name"]) && strlen($_POST["name"]) < 50) {
	$mysql["name"] = clean_input($_POST["name"]);
}

function clean_input($data) {
    $data = htmlspecialchars($data);
    $data = mysql_real_escape_string($data);
    return $data; 
}
Is that ok?
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

No, use htmlentities to escape data when outputting to html. When you want to output data to mysql, use mysql_real_escape_string.

What's important to realise is that it's all about 2 processes: filtering input and escaping output.

Input is what comes from the POST variables in this case. Other times it could be input from GET, SERVER or other variables.
Output is what goes out. That's output to a database, or output to a browser. That's two different things. You can read more about this in Chris Shifletts talks here or in his book http://phpsecurity.org/ or take a look at the Security guide from phpsec. And there have been a lot of good threads here of course :)

Here's the process in short:

Code: Select all

<?php
// start by declaring empty arrays
$clean = array();
$html  = array();
$mysql = array();


// filter input
if (isset($_POST["name"]) && !empty($_POST["name"]) && ctype_alnum($_POST["name"]) && strlen($_POST["name"]) < 50) {
   $clean['name'] = $_POST['name'];
}   


// logic stuff here...


// escape output for browser
$html['name'] = htmlentities($clean['name'], ENT_QUOTES, 'UTF-8');
// now it's safe to output to html
echo 'Welcome back ' . $html['name'];


// escape for mysql
$mysql['name'] = mysql_real_escape_string($clean['name']);
$sql = "SELECT *
       FROM users
       WHERE name = '{$mysql['name']}'";
?>
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

We're getting somewhere... Still, $clean still doesn't indicate in which it's clean (to me).

What about the following approach?

(1) Validate input (from the $_* arrays) and add the accepted input in $input
(2) Prepare variables for use in a query and store them in $mysql
(3) Prepare variables for use in html and store them in $html

When you're using this approach, it seems obvious that we'll evolve to three classes that getter/setter methods for key,value pairs.
In case of the input class we'll probably want to append rules to the setter..
In case of the mysql and html class we'll probably want some processing in the getter...
Post Reply