Page 1 of 1

security isues

Posted: Mon May 09, 2011 2:02 pm
by ta2shop
hy guys, i just finised the tutorial alex made about a mini shoping cart, and since the last one was.... well bad, i was hoping you can givet a lock, and mabye help my improve the security in it.
i want to add every posible thing but i want to make it to slow in the browser!
can you suggest somthing?
thanks
cart.php
<?php
session_start();

$page = "index.php";

mysql_connect("localhost","root","") or die(mysql_error());
mysql_select_db("ta2cart") or die(mysql_error());

if(isset($_GET['add'])) {
	$quantity = mysql_query('SELECT id, quantity FROM products WHERE id='.mysql_real_escape_string((int)$_GET['add'])) or die(mysql_error());
	while($quantity_row = mysql_fetch_assoc($quantity)) {
		if($quantity_row['quantity']!=$_SESSION['cart_'.(int)$_GET['add']]) {
			$_SESSION['cart_'.(int)$_GET['add']]+='1';
		}
	}
	header('Location: '.$page);
}

if(isset($_GET['remove'])) {
	$_SESSION['cart_'.(int)$_GET['remove']]--;
	header('Location: '.$page);
}
if(isset($_GET['delete'])) {
	$_SESSION['cart_'.(int)$_GET['delete']]='0';
	header('Location: '.$page);
}

function products() {
	$get = mysql_query('SELECT id, name, description, price FROM products WHERE quantity > 0 ORDER BY id DESC') or die(mysql_error());
	if(mysql_num_rows($get)==0) {
		echo "There are no products to display!";
	}
	else {
		while($get_row = mysql_fetch_assoc($get)) {
			echo'<p><b>'.$get_row['name'].'</b><br/>'.$get_row['description'].'<br/>'.number_format($get_row['price'],2).'€ <a href="cart.php?add='.$get_row['id'].'">Add</a><p/>';
		}
	}
}

function paypal_items() {
	$num = 0;
	foreach($_SESSION as $name => $value) {
		if($value!=0) {
			if(substr($name,0 ,5)=='cart_') {
				$id = substr($name, 5, strlen($name)-5);
				$get = mysql_query('SELECT id, name, price, shipping FROM products WHERE id='.mysql_real_escape_string((int)$id)) or die(mysql_error());
				while($get_row = mysql_fetch_assoc($get)) {
					$num++;
					echo '<input type="hidden" name="item_name_'.$num.'" value="'.$get_row['name'].'">';
					echo '<input type="hidden" name="item_number_'.$num.'" value="'.$id.'">';
					echo '<input type="hidden" name="amount_'.$num.'" value="'.$get_row['price'].'">';
					echo '<input type="hidden" name="shipping_'.$num.'" value="'.$get_row['shipping'].'">';
					echo '<input type="hidden" name="shipping2_'.$num.'" value="'.$get_row['shipping'].'">';
					echo '<input type="hidden" name="quantity_'.$num.'" value="'.$value.'">';
				}
			}
		}
	}
}

function cart() {
	foreach($_SESSION as $name => $value) {
		if($value>0) {
			if(substr($name, 0, 5)=='cart_') {
				$id = substr($name, 5, (strlen($name)-5));
				$get = mysql_query('SELECT id, name, price FROM products WHERE id='.mysql_real_escape_string((int)$id));
				while($get_row = mysql_fetch_assoc($get)) {
					$sub = $get_row['price']*$value;
					echo $get_row['name'].' x '.$value.' @ '.number_format($get_row['price'], 2).'€ = '.number_format($sub, 2).'€'.' 
					<a href=\'cart.php?remove='.$id.'\'>[-]</a> 
					<a href=\'cart.php?add='.$id.'\'>[+]</a> 
					<a href=\'cart.php?delete='.$id.'\'>[Delete]</a><br/>';
				}
			}
			$total += $sub;
		}
	}
	if($total ==0) {
		echo"Your cart is empty.";
	}
	else {
		echo'<br/> Total: '.number_format($total, 2).'€';
		?>
        <p>
        <form action="https://www.paypal.com/cgi-bin/webscr" method="post">
			<input type="hidden" name="cmd" value="_cart">
            <input type="hidden" name="upload" value="1">
			<input type="hidden" name="business" value="paypal@ta2shop.es">
            <?php paypal_items(); ?>
			<input type="hidden" name="currency_code" value="EUR">
			<input type="hidden" name="amount" value="<?php echo $total; ?>">
			<input type="image" src="http://www.paypal.com/es_XC/i/btn/x-click-but03.gif" name="submit" alt="Realice pagos con PayPal: es rápido, sin costo y seguro">
		</form>
        </p>
        <?php
	}
}

?>

Re: security isues

Posted: Mon May 09, 2011 2:08 pm
by jacek
moved to php security ;)

Nothing looks like it would be sql injectable to me

there is no need to use mysql_real_escape_string and (int) though so
mysql_real_escape_string((int)$_GET['add'])
can be
(int)$_GET['add']
Which applies to all the other similar cases.

Depending on who is allowed to set product descriptions you may want to use htmlentities on things like $get_row['description'].

Overall looks fine.

Re: security isues

Posted: Mon May 09, 2011 2:10 pm
by ta2shop
well i am the only one with back end permisions, and i just folowd the tutorial, if you say it is good to remouve the mysql_real_escape_string, i will ;)