Post Reply 
 
Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
DB_escape_string
03-27-2014, 11:53 PM
Post: #81
RE: DB_escape_string
This is a very good discussion in my opinion. I come from a background where the 4GL itself prevented this sort of abuse so I appreciate all of the information and the work being done to make things better. My 2 cents: It must be clear to NON PHP experts how to modify the code so as to NOT introduce security vulnerabilities. Whether that solution is straightforward examples that can be copied or a function/layer of abstraction that is used that handles the nasty details is beyond me.
Can I assume right now that If i wish to add a new field to the database and I add the the existing INSERT or UPDATE statement something like comments = '". $_POST['WOComments'.$i] ."' that the posted variable has already been cleaned in includes/session.inc so I do not need to escape it?
Find all posts by this user
Quote this message in a reply
03-27-2014, 11:55 PM (This post was last modified: 03-28-2014 12:28 AM by icedlava.)
Post: #82
RE: DB_escape_string
(03-27-2014 11:44 PM)Uhuru Wrote:  Hi Jo, as I said in a previous post, PHP 6 was dropped a couple of years ago, and the decision to re-open 6.0 has not been taken yet.
Sorry, I edited the post - should have been 5.6 not 6 - 5.6 alpha is out for a while now.

Quote:Most projects still have 5.2 as the minimum requirement for reasons that I have said before. The end of life you talk about is from the core developers, but the distros and the hosing companies still maintain the older releases.

Most projects that I work with, or have worked with for some years have a minimum of 5.3 to base development on but in some case the code will work on php 5.2 but not all of it - for example some modules may fail.

These are just my thoughts on the matter in relation to what code we should use to fix the issue of this thread - which is off track now.

It is very time consuming going over and over the same thing when in fact this thread is about fixing the escaping of data in context (rather than with a broad brush as in session.inc now) and how to do that.

One method is to use bind vars for database data, and something else for output data.

Perhaps we can get back to deciding if anything now - is going to be fixed?

Thanks,

note that php 5.2 has already had end of life past some years ago.
Hi Agaluski,
(03-27-2014 11:53 PM)agaluski Wrote:  Can I assume right now that If i wish to add a new field to the database and I add the the existing INSERT or UPDATE statement something like comments = '". $_POST['WOComments'.$i] ."' that the posted variable has already been cleaned in includes/session.inc so I do not need to escape it?

In the current code, all variables including session, post and get variables, no matter what they are are escaped using DB_escape_string which applies mysqli_db_escape_string to the data.

So yes.

However this is the problem of this thread - the escaping is not done in context. This is what needs to be fixed and the thread is looking for a solution when the session.inc code if fixed by removal of that escaping.

This code worked at the time it was done as a quick fix to an important problem, but was applied across all data which is causing problems elsewhere and requires fixing.

Thanks for your comment and interest.

I started a thread on minimum php version here http://www.weberp.org/forum/showthread.php?tid=2181 if anyone is interested. This might be better than hijacking this thread with it's pros and cons.

Cheers,
Find all posts by this user
Quote this message in a reply
03-28-2014, 12:26 PM
Post: #83
RE: DB_escape_string
Hi, Tim,

Thank you for correct my errors and give more explanation on security.

(03-27-2014 06:50 PM)Uhuru Wrote:  Hi Exson, I can't take the credit for the DB_escape_string() function. I think it goes all the way back to Phil's original code. :-)

There is a lot of talk of security issues, but I know that many of the security companies regularly look at our code, and occasionally highlight vulnerabilities to Phil and me. Unless Phil knows of any outstanding then all these have been dealt with and withdrawn by the security companies. Of course nobody can say the code is 100% safe, but there are no known injection attack vulnerabilities to my knowledge. Hence the statement I have made before that the current code works from a security viewpoint. There are aspects where Phil's code has fallen behind, such as the quoted variables that serakfalcon mentioned, but those can be fixed in an hour or so. There are also occasions where some variables are not correctly 'unescaped' but again these are not hard to find and fix.

I appreciate there are certainly better ways to achieve what we already have, but I am concerned that it would as Exson has said be hugely invasive, and as was discussed yesterday be much less readable.

Thanks
Tim

Best regards!

Exson
Find all posts by this user
Quote this message in a reply
03-28-2014, 12:44 PM
Post: #84
RE: DB_escape_string
Hi, Phil,


(03-27-2014 06:57 PM)phil Wrote:  Yes a valid concern Exson.

The issue is with the code in session.inc that DB_escape_string() all $_POST and $_GET variables - which it should not do - this was a pragmatic solution to the problem that got us around the SQL injection in the short term but left a legacy of other issues. icedlava is right about this I think - but the cost of doing this is very high indeed ignoring the sheer volume of work the risk of introducing bugs is 100% certain to. I am very worried about it - that's why I started a new branch for this work. We only merge the branch if there is sufficient enthusiasm to see the job through and when it is fully tested.
Thanks for your reply and thank you the efforts from Jo and serakfalcon and other developers. I'll study those code in working branch and hope to contribute some efforts in the future.

Best regards!

Exson
Find all posts by this user
Quote this message in a reply
03-28-2014, 01:31 PM (This post was last modified: 03-28-2014 01:39 PM by serakfalcon.)
Post: #85
RE: DB_escape_string
Hi guys, I just took a look at what's in the working repository, for my two cents:

Phil, a true parametrized query sends the variables over separately from the query, which is why it protects against SQL injection since it's not even possible to affect the SQL query string, your 'pseudo-parametrized' query just concatenates them into the string, which is essentially the same as what we have now.

Also, if we're going to re-write the queries we have an opportunity to remove &$Conn from all the functions, so please take it! $db is a global that can be referenced inside the functions and doesn't need to be exposed outside of the ConnectDB files.
Also, Jo, it's impossible for gettype to return blob, so it's unnecessary to have it in the switch, it's possible for the parametrized query to fail on prepare() or on execute() so you should use the error handlers I wrote, also if a datatype not covered by the switch (array,object) is passed, it will still be added to the $BindParams array but will not be added to params[0] so not only will it cause the code to mess up but it will also cause all the parameters after the messed up one to be inserted into the wrong places. (I hate that mysqli requires the type to be passed to it, so much room for errors)
Find all posts by this user
Quote this message in a reply
03-28-2014, 03:34 PM
Post: #86
RE: DB_escape_string
Hi serakfalcon,
(03-28-2014 01:31 PM)serakfalcon Wrote:  Hi guys, I just took a look at what's in the working repository, for my two cents:
Phil, a true parametrized query sends the variables over separately from the query, which is why it protects against SQL injection since it's not even possible to affect the SQL query string, your 'pseudo-parametrized' query just concatenates them into the string, which is essentially the same as what we have now.
I brought that up on this forum discussion when Phil committed it. It was also brought up separately via email to Phil which he's requested to forward to the dev list and which I've done (although I can't see any of my posts to the dev list of late).

Quote:Also, if we're going to re-write the queries we have an opportunity to remove &$Conn from all the functions
At the moment the 'draft' function kept the signature intact for compatibility as I was asked to use the current DB_query function. As mentioned previously, I'd prefer to use a new function that has a more efficient signature as well.

Quote:Also, Jo, it's impossible for gettype to return blob, so it's unnecessary to have it in the switch, it's possible for the parametrized query to fail on prepare() or on execute() so you should use the error handlers I wrote, also if a datatype not covered by the switch (array,object) is passed, it will still be added to the $BindParams array but will not be added to params[0] so not only will it cause the code to mess up but it will also cause all the parameters after the messed up one to be inserted into the wrong places. (I hate that mysqli requires the type to be passed to it, so much room for errors)

As previously discussed on this thread, I thought it was agreed not to include array/object - there are a few posts on this very issue.

Feel free to add it serakfalcon but imho I prefer to have an error and fix it at source as was discussed previously. I would still question why we are passing full arrays/objects to that function and inserting to the db. They should be handled according to individual script requirements prior to going to DB_query rather than have that gettype do the job which was my point about letting that gettype loop do it's job not something that should be done elsewhere (nothing to do with misunderstanding what it does as implied in an earlier thread).

As to the blob you are quite right. Both you and I seem to have missed it when we reviewed it a few posts back. I copied it from an opensource snippet and I'd say it was possibly a hangover from some old code reworked eg mysql_field_type. Feel free to correct it.

Cheers,
Find all posts by this user
Quote this message in a reply
03-28-2014, 07:33 PM
Post: #87
RE: DB_escape_string
Quote:Hi there,
just to put my two cents in-- Phil, the main advantage security wise of parametrized queries is that the data is sent over in a distinct 'package' from the query. You 'pseudo-parametrized' query is sent over as a string, however the true parametrized queries sends the query along separately from the parameters, which is why it protects against SQL injection, since the parameters are never a part of the SQL query string at all.

There are a few posts on this issue on the mailing list but as my posts there are rejected I will reply to the above here.

As Phil has pointed out he and I regularly get security reports from various security companies and they are always acted upon, and to my knowledge there are none outstanding. This leads me to believe that we are fairly tight on security, and that what we currently have works. Does it really make sense to go through all the scripts changing every DB_query() call, and inevitably introducing new bugs along the way?

I think a major change like this has to give us a benefit to make it worth while and as of yet I have not seen a convincing argument for this.

Thanks
Tim
Visit this user's website Find all posts by this user
Quote this message in a reply
03-28-2014, 08:05 PM
Post: #88
RE: DB_escape_string
What we have is a pragmatic solution which is not the best method of securing against sql injection attacks. My reading corroberates Jo's and serakfalcon's views. This amount of work has the potential to destroy webERP so we keep the stable branch till we resurface with a bulletproof version 5

Phil Daintree
webERP Admin
Logic Works Ltd
http://www.logicworks.co.nz
Visit this user's website Find all posts by this user
Quote this message in a reply
03-31-2014, 04:27 PM
Post: #89
RE: DB_escape_string
serakfalcon Wrote:I figured out a work around for the DB_PreparedQuery issues, I made a class that has most of the functions of mysqli_result, and DB_PreparedQuery passes that back. That way, the DB functions can handle it as if it was a mysqli_result object. I've updated the working branch accordingly.
I also moved $Parameters to the second position, so you don't have to specify values for everything (since DB_Query is now different $Parameters doesn't need to be last anymore), and reduced reliance on passing $db around, although legacy code will still work. I recommend removing $db from code outside of the connection includes wherever possible.

It seems to me that we are starting to re-invent pdo here. If this is the way we are going to go, aren't we better off using PDO?

Thanks
Tim
Visit this user's website Find all posts by this user
Quote this message in a reply
03-31-2014, 05:47 PM
Post: #90
RE: DB_escape_string
(03-31-2014 04:27 PM)Uhuru Wrote:  
serakfalcon Wrote:I figured out a work around for the DB_PreparedQuery issues, I made a class that has most of the functions of mysqli_result, and DB_PreparedQuery passes that back. That way, the DB functions can handle it as if it was a mysqli_result object. I've updated the working branch accordingly.
I also moved $Parameters to the second position, so you don't have to specify values for everything (since DB_Query is now different $Parameters doesn't need to be last anymore), and reduced reliance on passing $db around, although legacy code will still work. I recommend removing $db from code outside of the connection includes wherever possible.

It seems to me that we are starting to re-invent pdo here. If this is the way we are going to go, aren't we better off using PDO?

Thanks
Tim

in brief: yes. I was definitely thinking about it when I wrote the code. why on earth does mysqli return a completely different object for prepared statements as normal queries, anyways?
In any case, the code works now.
Find all posts by this user
Quote this message in a reply
Post Reply 


Forum Jump:


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