Is this code safe?

Ask about general coding issues or problems here.

Moderators: macek, egami, gesf

enigma86d
New php-forum User
New php-forum User
Posts: 3
Joined: Mon Feb 04, 2013 3:29 pm

Is this code safe?

Postby enigma86d » Fri Feb 08, 2013 3:23 pm

Hello,

I'm new to php programming and I wrote a code but I'm not sure whether or not I'm opening a door to hackers realy.
I use Prestashop, a open source cart to manage a online store. Because I want quickly access and alter its database, I created a .php file seems to do the trick. But how secure is it? In order to make it secure I used the login.php, and index.php files from the opensource cart; and if the e-mail and pass are ok, then it redirects to my own php file.
The user is redirected to a login.php file which suppose to be safe since is written by Prestashop; then the login.php redirects the user to index.php. The following is the index.php file

Code: Select all

<?php

define('_PS_ADMIN_DIR_', getcwd());
define('PS_ADMIN_DIR', _PS_ADMIN_DIR_); // Retro-compatibility
include(PS_ADMIN_DIR.'/../config/config.inc.php');
include(PS_ADMIN_DIR.'/functions.php');
include(PS_ADMIN_DIR.'/init.php');
@ini_set('display_errors', 'on');


//everything that is above suppose to be safe because is the login part of opensource cart
//next comes my own code


echo '
<!DOCTYPE html>
<html>
<head>
</head>
<body>';
?>

<!-- a form with a single input that takes a number (length is 13)-->
<form method="post" action="<?php echo $PHP_SELF;?>">
Scaneaza eticheta EAN13:<input type="text" id="my_input" size="20" maxlength="13" name="EAN"><br />
<input type="submit" value="submit" name="submit" style="display:none;">
</form>

<?php
$EAN = $_POST["EAN"];
if (!isset($_POST['submit'])) { // if page is not submitted to itself echo the form
//echo "<p>never submited<br /></p>";
} else {
$result = mysql_query("SELECT * FROM ps_product WHERE ean13=$EAN");
$attr=0;
$ean_valid=1;
if(!($row = mysql_fetch_array($result)))
  {
  $result = mysql_query("SELECT * FROM ps_product_attribute WHERE ean13=$EAN");
  if($row = mysql_fetch_array($result)) {
    $attr=1;
    }
    else
    {
    $ean_valid=0;
    }
  }
if ($ean_valid==1) {
  $qty = $row['quantity'];
  $id_prod = $row['id_product'];
  if ($qty>0) {
   $qty=$qty-1;
   if ($attr==0) {
      mysql_query("UPDATE ps_product SET quantity = $qty WHERE ean13 = $EAN");
      }
   else
      {
      mysql_query("UPDATE ps_product_attribute SET quantity = $qty WHERE ean13 = $EAN");
      $result = mysql_query("SELECT * FROM ps_product WHERE id_product=$id_prod");
      $row = mysql_fetch_array($result);
      $qty2=$row['quantity']-1;
      mysql_query("UPDATE ps_product SET quantity = $qty2 WHERE id_product = $id_prod");
      }


   echo $EAN." decremented, new quantity: ".$qty."<br />";
   $file='good_sound.wav'; echo "<embed src =\"$file\" hidden=\"true\" autostart=\"true\"></embed>";
   }
  else
   {
   echo "OUT OF STOCK! Quantity is: ".$qty."<br />";
   $file='bad_sound.wav'; echo "<embed src =\"$file\" hidden=\"true\" autostart=\"true\"></embed>";
   }
  }
  else
  {
  echo "EAN code not found!<br />";
  $file='bad_sound.wav'; echo "<embed src =\"$file\" hidden=\"true\" autostart=\"true\"></embed>";
  }
}

?>

<br />
<a href="index.php?logout">Logout</a>
</body> </html>



Does anybody think I'm inviting a hacker if I use this on my server?
Thanks

seandisanti
php-forum Fan User
php-forum Fan User
Posts: 838
Joined: Mon Oct 01, 2012 12:32 pm

Re: Is this code safe?

Postby seandisanti » Mon Feb 11, 2013 11:25 pm

right off the bat I'd remove the line

Code: Select all

@ini_set('display_errors', 'on');

you only want errors on in dev environment. in real world they can communicate info you do not want shared, including table names etc. if given the correct input. Also, this may sound nitpicky, but personally i believe that 'SELECT *' is bad form in general. I typically prefer explicitly named fields in select statements. In cases where you have multiple tables linked, it can really cut down on the size of the result set. You're also implicitly trusting the form posting your data by not verifying even as much as the referer or the post fields.

enigma86d
New php-forum User
New php-forum User
Posts: 3
Joined: Mon Feb 04, 2013 3:29 pm

Re: Is this code safe?

Postby enigma86d » Fri Feb 15, 2013 1:34 pm

1) Oh.. yap, I set display errors so I can debug it but forgot to deactivate it.
2) if I want to select a row in a table and I know the one element of that row (e.g. id_product then this is what I've been taught to write: mysql_query("SELECT * FROM ps_product WHERE id_product=$id_prod");
I don't understand how I can do it differently; can you give me a example?
3) Before being able to enter data in the form, the user has to log in. I was thinking this should prevent other people from accessing the form. Then I limit the input to 13 characters long. I'm not sure how else I should protect. Can you give me an example code?

Thanks allot!

seandisanti
php-forum Fan User
php-forum Fan User
Posts: 838
Joined: Mon Oct 01, 2012 12:32 pm

Re: Is this code safe?

Postby seandisanti » Fri Feb 15, 2013 3:18 pm

the asterisk is a wild card used in queries to say 'all fields'. It is better to be deliberate with your requests, like
SELECT field1,field2,field3 FROM atable WHERE criteria=true

you also want to escape your strings etc. think about if $EAN contained the value "0; DROP TABLE ps_product;" your query to return all rows just turned into 2 queries where the first returns no results, and the second discards the table.
If you're trying to be security minded, always assume that every user wants to break or compromise your database, pages, etc.

enigma86d
New php-forum User
New php-forum User
Posts: 3
Joined: Mon Feb 04, 2013 3:29 pm

Re: Is this code safe?

Postby enigma86d » Sun Feb 17, 2013 1:38 pm

Ooo, I see now. Ok, I will specify the fields I want.
I am limiting the input to 13 characters (using: maxlength="13") and I will also add a "is_numeric()" check to see that the entered value is a number.

I assume that if I do this (together with turning error reporting off) I shouldn't be afraid to put the script live.
If anybody does have any other suggestions please tell me know :)

Thanks.


Return to “PHP coding => General”

Who is online

Users browsing this forum: Google [Bot] and 1 guest