Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
DB_escape_string
03-10-2014, 07:13 PM, (This post was last modified: 03-11-2014, 06:14 PM by icedlava.)
#11
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).
Reply
03-10-2014, 10:52 PM,
#12
RE: DB_escape_string
Hi Jo, I agree that if there is a better solution (and there always is a better solution) we should use it. However randomly changing DB_escape_string() with either mysql_escape_string() or mysqli_escape_string() is wrong as you cannot guarantee that the mysqli functions will be available or that the mysql functions will still be available. That's why we have the ConnectDB_ abstractions. I agree with Phil that until we have that better solution we should continue with the code that works (albeit with what you describe as bandaids) rather than using one of mysql_escape_string or mysqli_escape_string either of which will break somebodies code.

Tim
Reply
03-11-2014, 05:03 AM,
#13
RE: DB_escape_string
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
Reply
03-11-2014, 01:26 PM,
#14
RE: DB_escape_string
(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.
Reply
03-11-2014, 05:43 PM,
#15
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
Reply
03-12-2014, 02:59 PM,
#16
RE: DB_escape_string
So what is the long term solution to this, accepting that we are not moving the code structure to a framework and want to avoid abstraction wherever possible.

We need to sanitise $_POST and $_GET in session.inc
Of course, we don't want piles of \\\\\\\\\\\\\\\\\\ in fields where we are escaping incorrectly - this is the only symptom I have seen and we have modified code wherever this has cropped up.

Do we just need to go through the scripts and DB_escape_string all SQL parameters and remove the code in session.inc - the snag with this solution is that there are a bugger of a lot of SQL parameters - but hey if that's where we are at ... let's divide and conquer?
Phil Daintree
webERP Admin
Logic Works Ltd
http://www.logicworks.co.nz
Reply
03-12-2014, 03:39 PM, (This post was last modified: 03-12-2014, 09:12 PM by icedlava.)
#17
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.
Reply
03-12-2014, 08:20 PM,
#18
RE: DB_escape_string
I agree that htmlspecialchars() should probably not be in DB_escape_string() function. Should we just take it out?

Tim
Reply
03-12-2014, 09:01 PM, (This post was last modified: 03-12-2014, 09:05 PM by serakfalcon.)
#19
RE: DB_escape_string
I think a good way of having it would be DB_escape_string() for input into DB and have a HTML_escape() function for output to the browser. Otherwise the data should not be escaped. This also gives us the flexibility to alter how HTML_escape() and DB_escape_string() work down the road.
I also think queries should be parameterized. mysqli_escape_string won't stop several types of clever SQL injections. Once they are parameterized we don't even need to worry about DB_escape_string.
Reply
03-12-2014, 09:08 PM, (This post was last modified: 03-12-2014, 09:13 PM by icedlava.)
#20
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.


Reply


Forum Jump:


Users browsing this thread: 1 Guest(s)