webERP Forum

Full Version: DB_escape_string
You're currently viewing a stripped down version of our content. View the full version with proper formatting.
Pages: 1 2 3 4 5 6 7 8 9
(03-27-2014, 03:32 PM)serakfalcon Wrote: [ -> ]Is it a good idea to have a boolean option that drastically alters the output of the function? I'd almost prefer to use a different function name in that case.
Personally I prefer a separate function, as I would also for the new DB_query.

Quote:
however you still need the wrapper function if you pass the statement back at all,
Yep, assumed we would if this were the case, and as we do now with various webERP db functions.

Cheers,


Dear all,

Thank you for your efforts to find more new ways to improve webERP. I have some concern about this project, although it seems a little late. Are we really on the right track?

According to some sample codes committed, we have to add a variables array, which is total different from those styles we used before. We have to add lots of question mark? Did this really make it safer than Tim's simple mysqli_real_escape()?

It seems that we have to rewrote all those scripts about query? It will make webERP green code instead of a mature system. Does this effort really need? IMHO.

If we concern SQL injection attack? Why we can not pin out some model, then we can check those scripts which maybe not so security. I never heard that there is one method which is immutable to all SQL attack. Security alert never been disappear since there is always new method invented out.

I have also meet some extra slash problem, but I think it's solvable. Why we cannot solved it case by case?

Concerning to php version problem. I think webERP only use very common and based parts of php. It performance well in almost in every version great than 4.2. It's not like other projects which almost broken after php5.3. I hope this merit can also be kept. If there is new version feature only support in higher version, I suggest to provide a solution for lower version instead of push users to upgrade their operation environments.

Sorry for those opinion which seems very non harmony with all your efforts! But I still hope more consideration to this before we go further.

Best regards!

Exson
(03-27-2014, 06:00 PM)Exsonqu_Qu Wrote: [ -> ]Did this really make it safer than Tim's simple mysqli_real_escape()?

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
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.
(03-27-2014, 06:57 PM)phil Wrote: [ -> ]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.

Yes that's correct. It didn't absolutely fix SQL injection but good enough however did introduce all the other issues by not considering context (so we have multiplying slashes, entities appearing in output etc, corrupt data in some cases causing query failures) - symptoms of the root issue.

This also highlighted some inconsistencies that I think have come about through patching the code in various places to overcome this 'symptoms'.

Quote:the cost of doing this is very high indeed ignoring the sheer volume of work the risk of introducing bugs is 100% certain to.

Yes, it is a problem that all opensource projects face, especially in the light of more recent PHP versions, the fact that php 5.2 is no longer supported and php 5.3 nearing end of life, and latest php's deprecating some functionality we've been used to, and changing ideas on 'best practice' for what it's worth.

I've been through massive code changes in a few projects now, and this particular issue some years back. Drupal, joomla, moodle, wordpress, [codeigniter is younger], are just some higher profile php projects that have been around similar times to webERP and have had to bite the bullet and handle this (PDO, bind vars, contextual escaping or a mix of all). webERP would be one of the only higher profile projects I know that has yet to update their code for this issue - and php 5.3 (that doesn't mean much - could be a lot more but it's the only one I know of).

While we can put it off, I think it's inevitable that something will have to be done sooner or later to fix the missing context for output preparation (entities,slashes) and security issues. The longer it's left the less time we will have to do it.

The aim of course, is to continue to build an ever better webERP for everyone.

Cheers,



(03-27-2014, 09:06 PM)icedlava Wrote: [ -> ]and php 5.3

Which 5.3 issues haven't been addressed? Pretty sure I did that a long time ago, and I haven't seen any 5.3 issues since then.

Tim
(03-27-2014, 09:14 PM)Uhuru Wrote: [ -> ]Which 5.3 issues haven't been addressed? Pretty sure I did that a long time ago, and I haven't seen any 5.3 issues since then.

Hi Tim,

I was making a general point about php versions getting higher and older ones reaching end of life (meaning not supported), and our code remaining the same. This is specifically in relation to the problem at hand ie escaping of variables without context, the symptoms it produces, and security issues with database queries that we could solve at the same time. A big job, yes, depending on the method chosen - but I believe inevitable in some form or other at some point in the future.

Hi Exson,
(03-27-2014, 06:00 PM)Exsonqu_Qu Wrote: [ -> ]According to some sample codes committed, we have to add a variables array, which is total different from those styles we used before. We have to add lots of question mark? Did this really make it safer than Tim's simple mysqli_real_escape()?
Yes, as mysqli_real_escape is not sufficient.

Quote: It seems that we have to rewrote all those scripts about query? It will make webERP green code instead of a mature system. Does this effort really need?
I believe some rewrite is inevitable - has to be done and I hope sooner than later so we can get it out the way. I think it will help make webERP a hardened, and up to date system.

Quote: I have also meet some extra slash problem, but I think it's solvable. Why we cannot solved it case by case?
I think part of the problem is the fact there is patching case by case - some cases which should not exist but due to incorrect escaping of data out of context (eg in session.inc or the old db_escape_string with the entities encoding in it). A more consistent system approach might help.

Quote: It's not like other projects which almost broken after php5.3. I hope this merit can also be kept.
I believe this is a symptom of those projects not acting in time to cater for php 5.3. But php 5.3 is already near end of life - we need to be looking at php 5.5 and 5.6 (php 5.6 alpha is out for a while now).
Quote:If there is new version feature only support in higher version, I suggest to provide a solution for lower version instead of push users to upgrade their operation environments.
I really understand this problem and the fact that some people cannot easily upgrade to a higher php.

However, at some point we cannot continue to code and support php 5.2 as we only loose the benefits of higher php versions and our code becomes more and more out of date.

Does anyone remember coding for php 3? or php 4? These debates to upgrade minimum php version are not new and there was much angst and discussion in some projects to do the work to stop support for php4!

But what would happen if we didn't upgrade our code since php3?

Sorry for being a broken record, but I believe we should be maintaining a branch for our webERP with php 5.2 support and making a higher minimum say php 5.3 for development and stable branch. That's one suggestion anyway.

I do think you have valid concerns Exson, but I believe we also need to move on at some point.

Cheers,
[Edit - 6 updated to 5.6]
Hi Jo, I think it's a bit harsh to say PHP moves forward and the webERP code doesn't. We were very early on in sorting the BC issues of PHP 5.3 and 5.4 and have always endeavoured to stay ahead of the game. However many hosting companies are still running highly patched versions of PHP 5.2 so we cannot just use the latest functionality PHP offers. I hear WordPress still has 5.3 issues in their code.

It is true we have never slavishly followed the latest fashions in PHP coding but I see that as an advantage. I have seen several projects die because it is hard to maintain developer interest when you have to constantly change large amounts of code because of fashion changes. For a project with a small number of developers like webERP large scale disruptive changes such as this can be very damaging.

All this still feels to me like a solution in search of a problem. I haven't seen any examples of where our code is vulnerable (send me it privately if you have one so I can look into it).

Thanks
Tim
(03-27-2014, 09:21 PM)icedlava Wrote: [ -> ](php 6 alpha is out for a while now).

That was dumped, and the branch was deleted some time ago. Discussions have recently (in the past few weeks) started up again as to whether to create a new 6.0 branch and start again. There were fundamental flaws with the first attempt at 6.0.

Many hosting companies are still at 5.2 because changing to 5.3 will break a lot of web sites that they host.

Thanks
Tim
(03-27-2014, 10:48 PM)Uhuru Wrote: [ -> ]Hi Jo, I think it's a bit harsh to say PHP moves forward and the webERP code doesn't.
Sorry you interpreted it that way, Tim. Perhaps I could have worded it better but if you have read prior threads and the last in context you should see my meaning and hopefully not take the most negative interpretation.

ie In terms of php moving forward - my meaning is that the PHP versions are getting higher with php 6 alpha out and php 5.3 reaching end of life. However our minimum php version is still php 5.2. Thus my suggestion to keep a maintenance version for php 5.2 and base webERP on a minimum of php 5.3. Hope it's clear.

I'm positive about webERP else I wouldn't be using it. Nevertheless I believe we can make webERP better for everyone - that is the goal .
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.

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.

Thanks
Tim
Pages: 1 2 3 4 5 6 7 8 9