Page 1 of 2

Is this code secure?

Posted: Wed Mar 22, 2006 4:26 pm
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 :)

Posted: Wed Mar 22, 2006 4:32 pm
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.

Re: Is this code secure?

Posted: Wed Mar 22, 2006 5:00 pm
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?

Posted: Wed Mar 22, 2006 5:12 pm
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.

Posted: Wed Mar 22, 2006 6:03 pm
by AlecH
You also forgot to clean some of the $_POST variables that were set.

Posted: Wed Mar 22, 2006 9:59 pm
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.

Posted: Wed Mar 22, 2006 10:01 pm
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; 
}

Posted: Thu Mar 23, 2006 3:47 am
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.

Posted: Thu Mar 23, 2006 7:12 pm
by danf_1979
Ok Maugrim, thanks for that info. I am using characters like á, ó, í, ¿, etc (spanish), so I guess i need to use htmlentities...

Posted: Thu Mar 23, 2006 10:51 pm
by Ambush Commander
If that's the case, I suggest looking into the possibility of UTF-8.

Posted: Fri Mar 24, 2006 2:56 am
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.

Posted: Fri Mar 24, 2006 7:21 pm
by danf_1979
So I should use something like this before entering data to mysql?

$mysql['name'] = htmlentities($clean['name'], ENT_QUOTES, 'UTF-8');

Posted: Fri Mar 24, 2006 7:30 pm
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?

Posted: Sat Mar 25, 2006 1:38 am
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']}'";
?>

Posted: Sat Mar 25, 2006 2:33 am
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...