Post Reply 
 
Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
DB_escape_string
03-06-2014, 05:42 PM
Post: #1
DB_escape_string
Just noting that I will be and have made fixes in the code base that do not use DB_escape_string but use mysql_real_escape_string instead.
Apparently we are only catering for mysql at the moment so this is not too affronting within the code and these fixes are marked 'temporary'.

The reason is because DB_escape_string is broken and using it will reintroduce htmlspecialchars into the database data again. The htmlspecialchars should not be called on data going into the database and when we use it it will cause issues elsewhere where entities are involved.

We can fix that function, but not before we evaluate what areas of the code rely on it currently to convert entities for displaying data, and also have other context relevant functions to prep the data for display or database use.

I will given time improve on my exising fixes and check to ensure mysqli_real_escape or mysql_real_escape is used as necessary but took safer option for quick fixes.

Cheers,
Find all posts by this user
Quote this message in a reply
03-06-2014, 06:26 PM
Post: #2
RE: DB_escape_string
Here's a quick grep of everywhere it is used FYI:
File | Lines used
ConfirmDispatch_Invoice.php
Contracts.php
DeliveryDetails.php
GoodsReceived.php
ImportBankTrans.php
PcAuthorizeExpenses.php
PDFTopItems.php
PO_Items.php
PurchaseByPrefSupplier.php
ReverseGRN.php
SelectOrderItems.php
ShopParameters.php
StockClone.php
StockLocTransferReceive.php
SupplierInvoice.php
TopItems.php
WorkOrderReceive.php
api_branches.php
api_customers.php
api_debtortransactions.php
api_glaccounts.php
api_glgroups.php
api_glsections.php
api_locations.php
api_purchdata.php
api_salesorders.php
api_stock.php
api_stockcategories.php
api_suppliers.php
api_workorders.php
ReportCreator.php
ConnectDB_mysql.inc
ConnectDB_mysqli.inc
ConnectDB_postgres.inc
session.inc
RCFunctions.inc
Find all posts by this user
Quote this message in a reply
03-06-2014, 06:26 PM
Post: #3
RE: DB_escape_string
Hi Jo,

mysql_real_escape_string() is deprecated as at PHP 5.5.

As the current DB_escape_string() function at least works it's probably best to keep that until your improvements come along.

I note Phil has changed your commits to use this function.

Tim
Visit this user's website Find all posts by this user
Quote this message in a reply
03-06-2014, 06:27 PM
Post: #4
RE: DB_escape_string
Also I would vote towards using PDO instead of MySQLi but that is for another day.
Find all posts by this user
Quote this message in a reply
03-09-2014, 10:19 PM (This post was last modified: 03-09-2014 10:25 PM by icedlava.)
Post: #5
RE: DB_escape_string
(03-06-2014 06:26 PM)Forums Wrote:  Hi Jo,
mysql_real_escape_string() is deprecated as at PHP 5.5.
Tim
Yes, if it is mysql_real_escape_string() in the code then it is a copy/paste error from a different branch. Should have been mysqli_real_escape_string().
Quote:I note Phil has changed your commits to use this function
Yes I emailed Phil and informed him of the problem with these 'corrections'. There should be a correction from mysql to the mysqli if mysql was used in error but that is all at the moment.

Unfortunatley, the DB_escape_string is broken and it continues to introduce problems into the database data.

There is no 'easy' solution as it is a code wide issue. There is no one easy function that can be provided, especially if we continue to use mysqli. This is a much bigger issue than just entities or unescaped charaters.

This is a change that has potential to affect how database security is handled (will we use bind parameters or not?), are we willing to change all the database inserts/updates in the codebase?, will we continue with mysqli or pdo?, how are we going to handle post vars through the code ? The latter of course is involved due to the current way the code is handled in session.inc and elsewhere with post, get, session vars.

What is the minimum level of php that we should code for? We will have to at some stage, make a break and perhaps keep a maintenance release for those that want php 5.2 or less.

For any future development I would think we have to go with at a minimum php 5.3 - even php 5.5 is the current release. There are incompatibilities wtih php versions so there has to be a decision on this.

So, it is not a quick fix with some 'improved functions'. It is something that should be actively discussed by webERP developers and planned for the code base.






In relation to a temp fix for DB_escape_string - as mentioned in my commit (it was temporary), we should in fact cater for mysql and mysqli which that quick fix didn't (but should have been mysqli).

There is the option of setting up another function such as DB_escape_string2 (as example) that corrects for the current DB_escape_string. I mentioned this in my email to Phil

Personally i'm not keen in introducing 'temp' functions into the api.
If you do want to have this as a solution then I can add one to cater for the 'temp' fixes so we do not have to use the current DB_escape_string function but still cater for mysql/mysqli.

Just let me know.

Cheers,
Find all posts by this user
Quote this message in a reply
03-10-2014, 04:44 AM
Post: #6
RE: DB_escape_string
(03-09-2014 10:19 PM)icedlava Wrote:  Yes, if it is mysql_real_escape_string() in the code then it is a copy/paste error from a different branch. Should have been mysqli_real_escape_string().

Hi Jo, the change Phil introduced was to revert to DB_escape_string(). Are you saying this introduces a security vulnerability? If so can you let me have an example so we can work on a fix?

If not, I think that Phil is right to revert to the DB_escape_string() function as it at least seems to work, even if there are better ways of achieving it

Tim
Visit this user's website Find all posts by this user
Quote this message in a reply
03-10-2014, 10:31 AM
Post: #7
RE: DB_escape_string
Hi Tim,
(03-10-2014 04:44 AM)Forums Wrote:  the change Phil introduced was to revert to DB_escape_string().
Yes, I understand he changed my mysqli_real_escape_string fix to DB_escape_string().

It is not a security issue but I purposely did not use DB_escape_string() as it is broken and will introduce HTML entities into the database data which causes problems. As we only cater for mysql at the moment in webERP and the fact that mysql_real_escape_string is deprecated at php 5.5 i did not think it will be a big issue at the moment, and marked it a temp fix.

However, I may have inadvertently used mysql_real_escape_string and that is wrong - it should have been mysqli_real_escape_string.

Cheers,
Find all posts by this user
Quote this message in a reply
03-10-2014, 04:16 PM
Post: #8
RE: DB_escape_string
(03-10-2014 10:31 AM)icedlava Wrote:  Hi Tim,
(03-10-2014 04:44 AM)Forums Wrote:  the change Phil introduced was to revert to DB_escape_string().
Yes, I understand he changed my mysqli_real_escape_string fix to DB_escape_string().

It is not a security issue but I purposely did not use DB_escape_string() as it is broken and will introduce HTML entities into the database data which causes problems. As we only cater for mysql at the moment in webERP and the fact that mysql_real_escape_string is deprecated at php 5.5 i did not think it will be a big issue at the moment, and marked it a temp fix.

I agree with Phil's descision here, temp fixes are the enemy of the good.
A better approach would be to fix up DB_escape_string() and where it is being used so it only happens when it needs to. That might be more work now but we'd have to do it eventually, and it is better in the long run, and saves us from digging around for where mysqli_escape_string got used if we switch to PDO or someone wants to use PostgreSQL or SQLite or even MSSQL down the road.
Find all posts by this user
Quote this message in a reply
03-10-2014, 04:38 PM
Post: #9
RE: DB_escape_string
(03-10-2014 04:16 PM)serakfalcon Wrote:  I agree with Phil's descision here, temp fixes are the enemy of the good.
I would usually agree with you too, however the DB_escape function is causing a lot of issues in a client database so it's the lesser of two evils in this case at least for them.

Quote:.
A better approach would be to fix up DB_escape_string() and where it is being used so it only happens when it needs to.
That is the intent - however we cannot just 'fix DB_escape_string' without fixing many other areas in the code (see session.inc). It is related to security as you would appreciate and will take some time to go through the code base and put in place a suitable solution.

Quote: saves us from digging around for where mysqli_escape_string got used if we switch to PDO or someone wants to use PostgreSQL or SQLite or even MSSQL down the road.

Of course that is the reason we use APIs and usually --never-- call mysqli directly. Nevertheless, as it is now we have to go digging around to find where DB_escape_string is used, and should be used when we fix it. It's just as easy to do a grep and find the mysqli_escape_string imho, and meanwhile ensure we do not continue to introduce bad data into the database. IF we continue to use DB_escape_string then people are going to have to go through and find/fix their database data - that's an even more worrisome proposal.

Of course, there is the option as previously mentioned to setup yet another 'alternative' DB_escape_string function to deal with mysqli and mysql however i find introducing 'temp' functions might be easy, but in the long run it can prove more work to remove them.

I believe that a solution for this is very much intertwined with security related to database insert/update and user data in HTML, AJAX etc. There has been a big bandaid applied (session.inc) and then inappropriate fixes (in DB_escape_string) to cope with the issues it causes.

It would be wondeful if this can be discussed and a proper, agreed solution applied. Either way it will be a lot of work.

Eg My preferred method would be to:
1. Remove the incorrect code from session.inc (prepping the post vars, get vars and session vars)

2. Put in place a proper solution for processing data for database insert/updates eg use bindvars (if using mysqli) and appropriate api function to handle that across all database insert/updates, or go to PDO (won't discuss pros/cons here).

3. Put in place a solution for processing post/get vars for use in HTML or AJAX etc

While it is relatively straighforward to write the above code, it is time consuming to implement across the codebase but happy to chip in with it.

Whatever solution is chosen, should be one agreed, and taken in conjunction with other possible areas that might impact it or be impacted by it.

Also, current version of php is 5.5. For those that are using that version we need to ensure the code base remains compatible. I think the time may be coming to retain a maintenance version for php <= 5.2 and move to a current version for php >= php 5.3 (or something of that ilk)

Hopefully this might start further discussion. Thanks for the feedback so far.

Cheers,
Find all posts by this user
Quote this message in a reply
03-10-2014, 06:56 PM
Post: #10
RE: DB_escape_string
I'm still not sure I fully understand the problem. Yes there are a few places in Phil's branch that don't deal with html entities correctly but they are not hard to fix up. I did it in my branch quite easily. All my tests use random mixes of so called illegal characters and html special characters and everything seems to work fine. I'm sure there are better ways of dealing with it (there is no such thing as a "perfect" solution) but the current code at least works. It always worries me when we are taking out working code (and DB_escape_string() has been in a very very long time, so cannot be causing that much damage) and not replacing it with something better.

Tim
Visit this user's website Find all posts by this user
Quote this message in a reply
Post Reply 


Forum Jump:


User(s) browsing this thread: 1 Guest(s)