Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
DB_escape_string
03-13-2014, 12:39 AM,
#21
RE: DB_escape_string
(03-12-2014, 09:08 PM)icedlava Wrote: 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.

My ideal query function for implementing with parameters would be to pass the query string and the array of variables, it would be trivial to report back a string that fills in each of the variables...
Reply
03-13-2014, 11:55 AM,
#22
RE: DB_escape_string
(03-13-2014, 12:39 AM)serakfalcon Wrote: My ideal query function for implementing with parameters would be to pass the query string and the array of variables, it would be trivial to report back a string that fills in each of the variables...
Yes agree. Whatever way, to provide a suitable string or not if that audit method were to continue, there are areas in webERP that are affected, and have to be added to the list of work to do. There may be more. There may be other areas.

In terms of how I thought it might work, using a very simple example, my little test this would do it this way - where something like DB_bindquery would handle the actual work involved instead of the current DB_query:
Code:
$bindvars= array($_POST['ContactName'],
    $_POST['ContactRole'],
    $_POST['ContactPhone'],
    $_POST['ContactNotes'],
    $_POST['ContactEmail']
);
$sql = "INSERT INTO custcontacts (debtorno,
                        contactname,
                        role,
                        phoneno,
                                  notes,
                        email)
                VALUES (?,?,?,?,?,?)";
$msg = _('The contact record has been added');
$result = DB_bindquery($sql,$db,$bindvars);
Reply
03-13-2014, 12:49 PM, (This post was last modified: 03-13-2014, 12:49 PM by phil.)
#23
RE: DB_escape_string
Well maybe we could do this inside the DB_query function - just sending an array parameter to DB_query and mysql_real_escape each parameter inside it.

It is a MASSIVE job - but between us we could break it down into bite size chunks?
Phil Daintree
webERP Admin
Logic Works Ltd
http://www.logicworks.co.nz
Reply
03-13-2014, 01:03 PM, (This post was last modified: 03-13-2014, 01:20 PM by icedlava.)
#24
RE: DB_escape_string
Hi Phil,
(03-13-2014, 12:49 PM)phil Wrote: Well maybe we could do this inside the DB_query function - just sending an array parameter to DB_query and mysql_real_escape each parameter inside it.
The bind parameter method is just one option and it is a big job. I set up a new function eg DB_bindquery in my example because of that. It allows one to got through each script one by one, leaving the rest of the codebase in place while each script is addressed.

With bind variables and an array, there needs to be different processing than in DB_query. A rough example of one way of handling for webERP :

Code:
function DB_bindquery($query,
                    &$Conn,
                    $bindvars = null,
                    $ErrorMessage='',
                    $DebugMessage= '',
                    $Transaction=false,
                    $TrapErrors=true)
{

    global $debug;
    global $PathPref;
    global $db;

    $query = filter_var($query, FILTER_SANITIZE_STRING); //sanitize the string
    $stmt = $Conn->prepare($query);

    if (is_array($bindvars) === true) {
        $params = array(''); // Create the empty 0 index
        foreach ($bindvars as $prop => $val) {

            switch (gettype($val)) {
                case 'NULL':
                case 'string':
                    $params[0] .= 's';
                    break;

                case 'integer':
                    $params[0] .= 'i';
                    break;

                case 'blob':
                    $params[0] .= 'b';
                    break;

                case 'double':
                    $params[0] .= 'd';
                    break;
            }

            array_push($params, $bindvars[$prop]);
        }
        call_user_func_array(array($stmt, 'bind_param'), refValues($params));
    }

    $stmt->execute();
    $result = bindResults($stmt);

$stmt->close();
    return $result;
That is not doing the error handling yet and please excuse any errors in above but it's the idea of it.
[edit - added] Some small functions refValues and bindResults called also not posted here.

Quote:It is a MASSIVE job - but between us we could break it down into bite size chunks?
I've done similar before in other projects some years back, and at some point I think it has to be done for webERP - this method or another - and better now than later.

It isn't --that-- much work once you have a few people get into it.
If you can have the old DB_query still functioning while you update to whatever the new function name is in scripts, it enables you to have them both working concurrently until the whole code base is processed.

Because it is massive, other areas of the code like the audit trail would need to be considered at the same time.

Also, you still need to look at display of variables -will you use an api to process get/post/session vars or will you treat each variable individually? Sometimes this type of work is best done at the same time as other massive jobs as we have to go through a script anyway.

Forgot to mention - this is where it gets difficult with PHP compatibilities.

Some decision would also need to be taken here about how to handle differences eg php >= PHP 5.3 and whether to continue to support a php 5.2 branch. What would be minimum PHP level (php 5.3 is no longer supported) given php 5.5.10 is the current release and php 5.6 alpha is out, or will we have a maintenance branch for older PHPs?
Reply
03-13-2014, 05:27 PM, (This post was last modified: 03-13-2014, 05:44 PM by phil.)
#25
RE: DB_escape_string
Quote:I set up a new function eg DB_bindquery in my example because of that. It allows one to got through each script one by one, leaving the rest of the codebase in place while each script is addressed.

Yes I see the logic.... but the very worst outcome for me would be we retain the DB_query function for some scripts and have another function with parameters in an array sent in some other scripts. Consistency throughout the application is critical for me - often in these changes there is only one person left standing at the end of the exercise .... I am keen for it not to be me - Gettextification, lowercasing of sql fields, utf-8, locale_number_format are 4 such examples. Actually I exaggerate there was actually a good team on gettextification _('') around strings and Exson and Uldis were there for the project for the utf-8 changes.

Also, as soon as we pull out the session.inc $_POST and $_GET sanitisation loops we need this code in place

I am not really sure how the audit trail functionality is affected? We just use the concatenated sanitised SQL string - where is the catch here?

Any suggestions with display of variables.

Maybe start a thread on PHP 5.5.10 to spell out the issues we need to consider.

When I look at the extent of the problem i.e. it really doesn't seem to be major to me - I am asking myself is this mission really of great importance and worth the effort required?

Once we have a plan in place I will make us up a new branch. If you could do a demo script documenting all the changes required and perhaps you would take the lead Jo and start the ball rolling.. divi out the scripts to those who put their hand up to help and lead the charge?
Of course you can count me in for 20 or so scripts. We could list the scripts in a forum thread and allocate to interested developers ... and update as we progress?
Phil Daintree
webERP Admin
Logic Works Ltd
http://www.logicworks.co.nz
Reply
03-13-2014, 05:52 PM,
#26
RE: DB_escape_string
(03-13-2014, 05:27 PM)phil Wrote: Yes I see the logic.... but the very worst outcome for me would be we retain the DB_query function for some scripts and have another function with parameters in an array sent in some other scripts. Consistency throughout the application is critical for me
I agree on consistency, but it's a time thing. Do you want to hold up a release until the whole code base is converted? It is a time consuming job.

Code:
Also, as soon as we pull out the session.inc $_POST and $_GET sanitisation loops we need this code in place
What do you mean by sanitisation?
There is no need to do anything to data held in $_POST, $_GET, or $_SESSION vars unless
1. They are saved to the database (processing by the DB_* function proposed)

2. Output to the browser (HTML in forms etc)

3. Used in AJAX or other output

If they are not output or saved to the databse, there is no need, and better off that they are not 'sanitised'.

If you mean validated - then that's something else - eg ensure we have a string 8 characters long that are all alphanumeric.

Quote:I am not really sure the audit trail functionality is affected? We just use the concatenated sanitised SQL string - where is the catch here?
Well, depends on what method you use for the fix. An sql statement using bind parameters is processed by the database server. There is no completed sql statement - it contains ? characters instead of any variables. Thus, serakfalcon's comment on it in reference to audit trail.

In the least the audit trail existing function would have to adapt to deal with the new method, convert the database data returned.

Quote:Someone else can take the lead and start the ball rolling and divi out the scripts to those who put their hand up to help! Of course you can count me in for 20 or so scripts. We could list the scripts in a forum thread and allocate to interested developers ... and update as we progress.
Happy to help once there is an agreed way forward and all areas are addressed.

Quote:Maybe start a thread on PHP 5.5.10 to spell out the issues we need to consider.
Perhaps it could be a thread on what minimum version to require for webERP, given that php 5.3 is no longer supported and things really start to change once we get to php 5.3 - eg harder to continue to maintain compatibility for php 5.2.x systems when you are writing for effective php 5.3, 5.4 and 5.5 too. I know we still need to maintain something for php 5.2 apparently - but for how much longer? Do we need a maintenance version? I'll start a new topic for it.
Quote:When I look at the extent of the problem i.e. it really seems to be minimal to me - I am asking myself is this mission really of great importance and worth the effort required?
I think it is very much worth the effort as it's both a security and validation issue - both important to an accounting package. We:

1. Ensure webERP has secure and efficient database transactions - mysqli_real_escape_string is insufficient to endure this and is not even required when using the bind parameter statement method.

2. Ensure webERP is securely validating and preparing variables, when required, for output to HTML/browser.

3. Prevent bad data getting into the database causing other issues down the line.

PHP has moved along a lot since webERP was written. I think that we will get further and further behind and a bigger and bigger job to catch up if we do not start upgrading the code now.

Quote:Any suggestions with display of variables.
.. thinking on it - trying to think of an efficient way, without too much abstraction.

Many thanks for the input

Cheers,
Reply
03-13-2014, 06:01 PM,
#27
RE: DB_escape_string
Quote:What do you mean by sanitisation?
There is no need to do anything to data held in $_POST, $_GET, or $_SESSION vars unless
1. They are saved to the database (processing by the DB_* function proposed)

Exactly, if we do nothing with $_POST, $_GET then we must have the DB_parmeterised() function going. If we have the DB_parameterised() function going we should remove the session.inc loops and existing DB_query() code is exposed.


Phil Daintree
webERP Admin
Logic Works Ltd
http://www.logicworks.co.nz
Reply
03-13-2014, 06:39 PM,
#28
RE: DB_escape_string
(03-13-2014, 06:01 PM)phil Wrote: Exactly, if we do nothing with $_POST, $_GET then we must have the DB_parmeterised() function going. If we have the DB_parameterised() function going we should remove the session.inc loops and existing DB_query() code is exposed.

Yes, agree - thus we need a logical step by step implementation that can handle all situations over the implementation time - not overnight - throughout the codebase. Needs to be thought out, written down, and then hopefully as many of us as possible can jump in and do it Smile
Reply
03-14-2014, 08:25 PM,
#29
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

I notice Phil has committed my suggestion today. Thanks Phil!

Tim
Reply
03-14-2014, 08:33 PM,
#30
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

Yes, that is something that has to be done, in conjunction with other fixes. Doing it now will ensure entities are not going into the database which is good. But there will be code around that assumes entities are already taken care of when displaying to the browser, so will in fact not be once this is changed unless each is treated individually.
Reply


Forum Jump:


Users browsing this thread: 2 Guest(s)