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

Post by danf_1979 »

Ok, thanks for all your comments. I really appreciate them. This is how the code looks now:
(I have implemented your ideas, and I feel very comfortable with them)

Code: Select all

<?php

// Header
include_once("../class.php");

$lan_file = BASE."languages/Spanish.php";
if (file_exists($lan_file)) require_once($lan_file);
// End Header

$mysql = array();
$clean = array();

// Common to add or edit
if (isset($_POST["do_add"]) || isset($_POST["do_edit"])) {
	// Parent name
	if (isset($_POST["parent"]) && !empty($_POST["parent"]) && strlen($_POST["parent"]) <= 50) {
		if ($_POST["parent"] == CAT_FIRST_PARENT) {
			$mysql["parent_id"] = 0;
			$msg = CAT_ACTION_ADD_06;
			$clean["parent"] = "";
		}
		else {
			$parent = array();
			$clean["parent"] = mysql_real_escape_string($_POST["parent"]);
			$parent= $mydb->getOne("SELECT id FROM e107_cart_categories WHERE name='".$clean["parent"]."' ");
			$mysql["parent_id"] = $parent["id"];
			$msg = CAT_ACTION_ADD_07;
		}
	}
	else exit();
	// category name
	if (isset($_POST["name"]) && !empty($_POST["name"]) && strlen($_POST["name"]) <= 50) {
		$mysql["name"] = mysql_real_escape_string($_POST["name"]);
	}
	else exit();
	// description, not mandatory
	if (isset($_POST["description"]) && strlen($_POST["description"]) <= 255) {
		$mysql["description"] = mysql_real_escape_string($_POST["description"]);
	}
	else exit();
	// image, not mandatory
	if (isset($_POST["image"]) && preg_match("/^.*(\.(gif|jpg|png))$/",$_POST["image"]) && strlen($_POST["image"]) <= 20) {
		$mysql["image"] = mysql_real_escape_string($_POST["image"]);
	}
	else exit();
	// image alt tag, not mandatory
	if (isset($_POST["image_alt"]) && strlen($_POST["image_alt"]) <= 50) {
		$mysql["image_alt"] = $_POST["image_alt"];
	}
	else exit();
}

// Do add result
if ( isset($_POST["do_add"]) ) {
	$html['parent'] = htmlentities($clean['parent'], ENT_QUOTES, 'UTF-8'); 
	$mydb->insert("e107_cart_categories", $mysql);
	$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;'>".CAT_ACTION_ADD_05.$msg.$html["parent"]."</td>
	    </tr>
	  </tbody>
	</table>
	";
}

// End do add action result

// Do edit result
if ( isset($_POST["do_edit"]) ) {
	if (isset($_POST["category_selected_id"]) && !empty($_POST["category_selected_id"]) && is_numeric($_POST["category_selected_id"])) {
		$clean["category_selected_id"] = mysql_real_escape_string($_POST["category_selected_id"]);
	}
	else exit();
	$mydb->update("e107_cart_categories", $mysql, "id='".$clean["category_selected_id"]."' " );
	$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;'>".CAT_ACTION_EDIT_07."</td>
	    </tr>
	  </tbody>
	</table>
	";
}
// End do edit result

// Do delete result
if ( isset($_POST["do_delete"]) ) {
	// category name, again
	if (isset($_POST["name"]) && !empty($_POST["name"]) && strlen($_POST["name"]) <= 50) {
		$clean["name"] = mysql_real_escape_string($_POST["name"]);
	}
	else exit();
	$category = $mydb->getOne("SELECT * FROM e107_cart_categories WHERE name='".$clean["name"]."' "); // Category to get wasted
	$mydb->query("DELETE FROM e107_cart_categories WHERE id='".$category["id"]."' ");
	if (isset($_POST["transfer"]) && !empty($_POST["transfer"]) && strlen($_POST["tranfer"]) < 50) {
		$clean["transfer"] = mysql_real_escape_string($_POST["transfer"]);
		if ($clean["transfer"] == CAT_ACTION_DELETE_06) { // No transfer, products are going to be deleted too.
			$products2purge =$mydb->getAll("SELECT * FROM e107_cart_product_categories WHERE category_id='".$category["id"]."' ");
			foreach ($products2purge as $product2purge) {
				$mydb->query("DELETE FROM e107_cart_products WHERE id='".$product2purge["product_id"]."' ");
			}
			$mydb->query("DELETE FROM e107_cart_product_categories WHERE category_id=".$category["id"]." ");
		}
		else { // Tranfer products to another category
			$category2get_products = $mydb->getOne("SELECT * FROM e107_cart_categories WHERE name='".$clean["transfer"]."' ");
			$mydb->update("e107_cart_product_categories",array("category_id"=>$category2get_products["id"]), "category_id='".$category["id"]."' ");
		}
		$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;'>".CAT_ACTION_DELETE_04."</td>
		    </tr>
		  </tbody>
		</table>
		";
		}
	}
// End do delete result

$template = new Template(BASE."templates/simple");
$template->set_file("file","main.html");
$template->set_var(array(
					"STYLESHEET" => BASE."templates/simple/admin.css",
					"TITLE" => $main["catalog_title"],
					"CATEGORIES" => $CATEGORIES,
					"CONTENT" => $CONTENT,
					"FOOTER" => $FOOTER,
					));
$template->parse("output","file");
$template->p("output");
?>
I think its ok, but I really dont know for sure if it has holes... :(
Thanks :)
HubGoblin
Forum Newbie
Posts: 7
Joined: Fri Apr 14, 2006 9:27 am

Post by HubGoblin »

Just have in mind that in some specific cases addslashes(), addcslashes() or mysql_real_escape_string() will NOT prevent SQL injection, but will do exactly the opposite action - TRIGGER SQL injection. If you really want to secure that code you have to make some more effort to escape some characters.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

HubGoblin wrote:Just have in mind that in some specific cases addslashes(), addcslashes() or mysql_real_escape_string() will NOT prevent SQL injection, but will do exactly the opposite action - TRIGGER SQL injection. If you really want to secure that code you have to make some more effort to escape some characters.
Eh? :? (now, it's not as bad as if you had claimed that base64_encode() could cause injections, but it's pretty close).
Post Reply