DB_escape_string - Printable Version +- webERP Forum (http://www.weberp.org/forum) +-- Forum: webERP Discussion (http://www.weberp.org/forum/forumdisplay.php?fid=1) +--- Forum: Development Discussion & Specification (http://www.weberp.org/forum/forumdisplay.php?fid=10) +--- Thread: DB_escape_string (/showthread.php?tid=2143) |
DB_escape_string - icedlava - 03-06-2014 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, RE: DB_escape_string - serakfalcon - 03-06-2014 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 RE: DB_escape_string - Forums - 03-06-2014 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 RE: DB_escape_string - serakfalcon - 03-06-2014 Also I would vote towards using PDO instead of MySQLi but that is for another day. RE: DB_escape_string - icedlava - 03-09-2014 (03-06-2014, 06:26 PM)Forums Wrote: Hi Jo,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 functionYes 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, RE: DB_escape_string - Forums - 03-10-2014 (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 RE: DB_escape_string - icedlava - 03-10-2014 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, RE: DB_escape_string - serakfalcon - 03-10-2014 (03-10-2014, 10:31 AM)icedlava Wrote: Hi Tim, 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. RE: DB_escape_string - icedlava - 03-10-2014 (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:.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, RE: DB_escape_string - Forums - 03-10-2014 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 |