MySQL Security Concerns

[AnimeCYC]

There are some serious issues with the changes in 1.2.1 I have not had the chance to run through it completely but I see some major flaws. In the MySQL class for instance you are using the die() method explicitly without regard to the security risk involved with showing people the queries. I would understand it if you had a debug flag in there for developers to see there SQL errors but as it is this is not something for production. I would highly recommend replacing the die() methods with that of a more secure and intuitive method. It looks as though you just rushed this release and did not properly take a look at the code that was written. Just from this one file it makes me wonder what other bad practices have taken place in this "update". I don't intend for this message to be offensive at all, but the fact that PrestaShop is widely used by people concerns me. Please fix this ASAP.

[Paul C]

In the MySQL class for instance you are using the die() method explicitly without regard to the security risk involved with showing people the queries.

Er, just where exactly are you seeing the queries? The only lines I see in the MySQL class that use die are in the connect() function (called way before you're in a position to run any queries at all):

22. die(Tools::displayError('The database selection cannot be made.'));
25. die(Tools::displayError('Link to database cannot be established.'));
28. die(Tools::displayError('PrestaShop Fatal error: no utf-8 support. Please check your server configuration.'));

If you've properly configured your SERVER then you shouldn't be displaying PHP errors (although you may want to silently log them to a log file) either. You should just show a blank screen. Prestashop will try and impose this on your store, although on some servers it will not be allowed to turn error display off.

Paul

[AnimeCYC]

In the MySQL class for instance you are using the die() method explicitly without regard to the security risk involved with showing people the queries.

Er, just where exactly are you seeing the queries? The only lines I see in the MySQL class that use die are in the connect() function (called way before you're in a position to run any queries at all):

22. die(Tools::displayError('The database selection cannot be made.'));
25. die(Tools::displayError('Link to database cannot be established.'));
28. die(Tools::displayError('PrestaShop Fatal error: no utf-8 support. Please check your server configuration.'));

If you've properly configured your SERVER then you shouldn't be displaying PHP errors (although you may want to silently log them to a log file) either. You should just show a blank screen. Prestashop will try and impose this on your store, although on some servers it will not be allowed to turn error display off.

Paul

I have attahced the file bundled with 1.2.1 please look at the functions that execute queries. Im not sure if your looking at in house version of your software but the die() methods are still being called:

   public function    getRow($query)
   {
       $this->_result = false;
       if ($this->_link)
           if ($this->_result = mysql_query($query.' LIMIT 1', $this->_link))
           {
               if (mysql_errno())
                   die(Tools::displayError($this->getMsgError($query)));
               return mysql_fetch_assoc($this->_result);
           }
       if (mysql_errno())
           die(Tools::displayError($this->getMsgError($query)));
       return false;
   }
   public function    Execute($query)
   {
       $this->_result = false;
       if ($this->_link)
       {
           $this->_result = mysql_query($query, $this->_link);
           if (mysql_errno())
               die(Tools::displayError($this->getMsgError($query)));
           return $this->_result;
       }
       if (mysql_errno())
           die(Tools::displayError($this->getMsgError($query)));
       return false;
   }
   public function    ExecuteS($query, $array = true)
   {
       $this->_result = false;
       if ($this->_link && $this->_result = mysql_query($query, $this->_link))
       {
           if (mysql_errno())
               die(Tools::displayError($this->getMsgError($query)));
           if (!$array)
               return $this->_result;
           $resultArray = array();
           while ($row = mysql_fetch_assoc($this->_result))
               $resultArray[] = $row;
           return $resultArray;
       }
       if (mysql_errno())
           die(Tools::displayError($this->getMsgError($query)));
       return false;
   }

I have downloaded the file just now to make sure I was not going crazy and the functions above still contain the aforementioned methods.

MySQL.php

[Paul C]

My utmost apologies - you are indeed correct.

My mistake - I was looking at an older version, and you are indeed correct that they have left what looks to me -- like their debugging code in there. (Note that I have nothing to do with writing the core PrestaShop code). I upgraded the test store to 1.2.1.0 via a shell login and hadn't sync'd the local copy on my machine.

I'm happy that us (3rd party) developers could use this to see what's going wrong with our code, but as you say it's a BIG no for production code.

I would be happy if those were replaces using trigger_error() -- that way the error condition can be logged correctly and we're all happy

I've attached a version of the mysql class file that uses this technique, for anyone who is concerned about using the original.

Paul

MySQL.php