03-10-2014, 07:13 PM,
(This post was last modified: 03-11-2014, 06:14 PM by icedlava.)
|
|
icedlava
Senior Member
|
Posts: 80
Threads: 9
Joined: Sep 2012
|
|
RE: DB_escape_string
(03-10-2014, 06:56 PM)Forums Wrote: I'm still not sure I fully understand the problem. Put simply the code in webERP in session.inc and the DB_escape_string are causing all sorts of problems.
They look like a quick fix that was for a critical security issue throughout webERP code - one that is time consuming to fix properly. It is put in place supposedly to be one quick solution to all the security probs and issues related to slashes, htmlentities, and unescaped data that one finds in databases, display of HTML, etc. So yes - it does handle the security issue (eg XXS and so on) in a big bandaid way.
It should not be the final solution. This must be done in a context sensitive manner and cannot be handled in one solution as in webERP that can result in multiple problems with database data corruption and display issues.
Quote:It always worries me when we are taking out working code (and DB_escape_string() has been in a very very long ti
If something is obviously broken, and causing problems like DB_escape_string then it should be fixed.
It does not worry me as the code problem and solution is obvious. It cannot be fixed alone as it has been put in place in conjunction with the code in session.inc to handled security in a blanket manner. Unfortunately it creates other problems.
The code in these two places is obviously a quick fix at the time that did its immediate job - eg one of those 'temporary' fixes that everyone seems to dislike - but it has been left in place for too long and the problems it causes elsewhere has not been addressed.
Quote:so cannot be causing that much damage
Well - that is because there are many, many bandaids being put into the code (some incorrect) over time to 'fix' the issues. They are still cropping up and i've been working with webERP for quite some time. Now we have a code base with bandaids all through it for these problems and some are wrong, and we still are getting data corruption to this day as one symptom (eg the ReverseGRN function prob for example).
|
|
03-11-2014, 05:43 PM,
|
|
Forums
Senior Member
|
Posts: 155
Threads: 9
Joined: Jan 2014
|
|
RE: DB_escape_string
(03-11-2014, 01:26 PM)serakfalcon Wrote: (03-11-2014, 05:03 AM)Forums Wrote: Would it be best to change the relevant ConnectDB file to just use the correct escape_string() function. That way whichever driver the user is using they will get the correct function?
Thanks
Tim
It's a two-fold thing: code should only be escaped as htmlentities if it needs to be (on input to the browser, this should be decided by the view layer) and should only be escaped for the db on input to the db (which should be decided by the db abstraction layer). The issue right now is all fields are being escaped on $_POST or $_GET,in session.inc which is inappropriate if they are not used for insertion into the DB, and in some cases, the form itself escapes before sending the values off to $_POST, or htmlescapes values that will be posted into the DB, resulting in values being escaped twice in some forms.
Yes I am aware of the issue, most of the double escaped fields have long since been found and dealt with. My concern was that we might be removing this before having a solution to replace it with.
Tim
|
|
03-12-2014, 03:39 PM,
(This post was last modified: 03-12-2014, 09:12 PM by icedlava.)
|
|
icedlava
Senior Member
|
Posts: 80
Threads: 9
Joined: Sep 2012
|
|
RE: DB_escape_string
Hi Phil,
I can't say how to fix it without discussion. A general synopsis to start? Hopefully people will fill in anything I've missed.
A. What happens now in the code
session.inc does the following (leaving aside the get_magic_quotes_gpc()) treatments which are ok) and does so to all session, get and post vars:
1. escapes the data for database using mysqli_real_escape_string
2. prepares variables for output in HTML using htmlspecialchars
These are both done now by way of the DB_escape_string function which applies both mysqli_real_escape_string and hmtlspecialchars to the variable/string in question.
3. 'fixes' for symptoms:
We have patched some data in the code because due to past treatments and some current ones, databases contain either data with entities or too many slashes. So unfortunately in some places we have to remove slashes, unencode it for entities (just in case it is encoded) and then htmlspecialchar encode it to make sure it is ok before displaying it.
Similarly we have to html unencode it, and remove slashes before mysqli_real_escape_string it back for database insert/update.
B. What should happen?
1. all data going into the database (insert, update) should have strings treated with mysqli_real_escape_string equivalent (not htmlspecialchars).
2. All data going into display views (forms etc) should be treated for html entities using htmlspecialchars.
3. Data input by the user should also be validated before it is output/saved but that is another issue but one we might want to be address at the same time
4. We should not be using stripslashes or addslashes anywhere in the code (except in session.inc where we test for get_magic_quotes_gpc).
5. Because there is 'bad' data in the database now, we may have to 'fix' data in or retrieved from the database as mentioned above in A so it can be handled properly for display or resaving to the database - fixed for slashes or entities that may be in it when retrieved from the database.
6. If we have variables that also need to be used in AJAX or other output then they needed to be treated appropriately.
7. imho - we should also add additional security to all data going into the database by means of bind parameter use with mysqli (or use PDO). Either way it's a lot of work but might be done at the same time as fixing for 1. above. mysqli_real_escape_string is not enough to avert all security issues.
[edited/added]8. We need to remove all the 'patches' that have been added in the code base to date to deal with slashes, entities, and escaping for database.
[edited - took out 5. and renumbered - as 5 was a duplicate of 2.]
C. How do we fix it?
That would be good discussion on method to use, what to include or not, and logical steps for implementing it throughout the code.
|
|
03-12-2014, 09:08 PM,
(This post was last modified: 03-12-2014, 09:13 PM by icedlava.)
|
|
icedlava
Senior Member
|
Posts: 80
Threads: 9
Joined: Sep 2012
|
|
RE: DB_escape_string
(03-12-2014, 09:01 PM)serakfalcon Wrote: Once they are parameterized we don't even need to worry about DB_escape_string.
My thoughts too.
I put in place a little function for handling this in a test branch of my local webERP as an experiment a while back. The function took the place of DB_query in scripts I was processing (leaving in place DB_query in other scripts until they were 'fixed').
One of the issues is of course used parameterized vars is that you can never capture the SQL query itself in full as webERP does now in some scripts.This would have impact in other areas such as the current way the audit trail script works.
|
|
|