Re: [Phpgroupware-developers] Testing CK-Ledger v.0.7.1 against phpgroupware-0.9.16.RC1

[Dave Hall]

C=20K=20Wu <address@hidden> wrote:

> Hello, folks,
> 
> Me again.  There is some further complication with the
> urlencode/decode thing.
> 
> In .../phpgwapi/inc/class.sessions.inc.php, around
> line 1145, the code reads,
> 
> /* Now we process the extravars into a proper url
> format */
> /* if its not an array, then we turn it into one */
> /* We do this to help prevent any duplicates from
> being sent. */
> if (!is_array($extravars) && $extravars != '')
> {
>    $a = explode('&', $extravars);
>    $i = 0;
>    while ($i < count($a))
>    {
>        $b = split('=', $a[$i]);
>        $new_extravars[$b[0]] = $b[1];
>        $i++;
>    }
>    $extravars = $new_extravars;
>    unset($new_extravars);
> }
> 
> Apparently, '=' is used as the GET argument/value
> separator.  However, if there is a second '=' in
> $a[$i], the value part will be truncated.  This is the
> case when an SQL is passed from script to script as
> GET value.  In my case, the raw GET string looks like
> this,
> 
> "filter= WHERE substring(source,1,2)='GD'"
> 
> so the callee script only recovers $filter as "WHERE
> substring(source,1,2)"

Hmmmm.  I would argue that passing SQL via GET or even POST is very poor
security, and so it is good that this breaks.  Also the $extravars, as
shown in the code should be an array as it is faster for process, no
explode() needed :)

Without studying the CK Ledger code, anyone with access to the app,
could easily use mailicious SQL, for example by changing the URL from:
filter= WHERE substring(source,1,2)='GD'

to

?filter=WHERE+substring(source,1,2)='GD';+DROP+DATABASE+phpgroupware#
Note some (but not all) url_encode()ing is done

You should never send anything to the database without making sure it is
safe.  All it takes is one line of compromised code in one compromised
app, and you whole phpgw install could be lost.  As part of the 16
release we are removing a lot of these problems.

A safer way of handling the example above would be:

//Code Snippet for app/caller.php
$get_args = array(  'sub_start'   => 1,
                    'sub_end'     => 2,
                    'sub_field'   => 'source',
                    'filter_val'  => 'GD'
                 );

$url = $GLOBALS['phpgw']->link('/app/script.php', $get_args);


//Code Snippet for app/script.php
....
//some get_var var assignments here
....
$sql  = 'SELECT * FROM phpgw_app_tbl ';
$sql .= 'WHERE substring(' . $this->db->db_addslashes($sub_field) .','
intval($sub_start) . ',' . intval($sub_end) . ")="' . 
$this->db->db_addslashes($filter_val) . "'";

I know this is a little messy, it is designed to be an example.  The
easiest way to do this casting/escaping is when assigning the values
using get_var(), just wrap the call in the appropriate function.  This
may look like it is adding a lot of extra work to creating a query, but
really it must be done for every variable.  Best place to do it is in
the so layer.  

I hope this makes sense, if not ask questions and I will try to explain
it a little more.

Cheers

Dave

<snip />

> I am not sure if '&' may similarly be a problem.  I'll
> leave it to the sessions maintainer to check it out
> and perhaps rectify the situation.  Again, this
> problem will affect other addon scripts.
> 
> Cheers,
> CK
> 
> --- Dave Hall <address@hidden>:
> Hey CK,
> > 
> > C=20K=20Wu <address@hidden> wrote:
> > 
> > > Hello, Dave,
> > > 
> > > I think I've found what's going on.
> > > 
> > > With 0.9.14.006,
> > > 
> > > ../phpgwapi/inc/class.sessions_php4.inc.php (line
> > 951)
> > > and ../phpgwapi/inc/class.sessions_db.inc.php
> > (line
> > > 977) read,
> > > 
> > > $new_extravars .= "$key=$value" ;
> > > 
> > > With 0.9.16RC1,
> > > 
> > > ../phpgwapi/inc/class.sessions.inc.php (line 1194)
> > > reads,
> > > 
> > > $new_extravars .= $key.'='.urlencode($value) ;
> > > 
> > > So, apparently, with earlier versions, it is the
> > > application script's responsibility to url_encode
> > GET
> > > variables before sending it on.  However, with
> > > 0.9.16RC1, the sessions facility handles the
> > > url_encode-ing when it receives the GET variables
> > from
> > > the application script.
> > > 
> > > With CK-Ledger v.0.7.1 running against phpgw
> > > 0.9.16RC1, it means double url_encoding and
> > therefore
> > > the callee scripts need to url_decode the GET
> > variable
> > > one more time to recover the correct value.
> > > 
> > > I think this will break a lot of the addon module
> > > codes.  However, if the GET variable passed
> > contains
> > > pure alphanumeric chars, no error will be
> > detected,
> > > since urlencode/urldecode in these cases do not
> > alter
> > > the GET variables.  So, there may be quite a fair
> > bit
> > > of  spurious 0.9.16RC1 errors being the result of
> > the
> > > above.
> > 
> > Ok, now I follow what is going on.  I didn't make
> > this change, but I can
> > understand (and agree with) the logic behind it. 
> > This is my logic with
> > it, others may disaagree, it is easier to url_encode
> > the variables, once
> > in the api, than each app maintainer having to
> > remember to encode them.
> >  It also means that if we ever have to do anything
> > else to the GET args
> > it can be changed once in the API and all apps
> > automatically get the
> > benefit.
> > 
> > I understand this will cause some problems with CK
> > Ledger, this is
> > unfortunate, but I doubt the change will be backed
> > out.  As will all new
> > versions of the API there are changes.  The 16 API
> > has quite a few
> > changes, some of which I think you app could benefit
> > from.  
> > 
> > I would suggest that you continue testing with the
> > 16RCs with regular
> > CVS updates, and keep in touch with us.  I am
> > willing to assist you get
> > your app to run properly on 16.  Please be aware
> > that I do not use CK
> > Ledger, but am happy to answer any questions you may
> > have.
> > 
> > Cheers
> > 
> > Dave
> > 
> > > 
> > > Cheers,
> > > CK
> > > 
> > > 
> > > 
> > > Dave Hall:
> > > 
> > > >CK Wu <address@hidden> wrote:
> > > >
> > > >>Hello, folks,
> > > >>
> > > >>While testing CK-Ledger v.0.7.1 against
> > > >>phpgroupware-0.9.16.RC1,
> > > >>I came across the following,
> > > >>
> > > >>When calling,
> > > >>
> > > >>
> > >
> >
>
>http://localhost/.../loglist.php?filter=%2BWHERE%2B1%253D1%2B&sessionid=...&kp3=...&domain=default&click_history=...
> > > >
> > > >Is this
> > >
> >
>
>http://localhost/phpgroupware/loglist.php?filter=%2BWHERE%2B1%253D1%2B&;...
> > > >
> > > >or
> > > >
> > > >http://localhost/ck-
> > >
> > ledger/loglist.php?filter=%2BWHERE%2B1%253D1%2B&...>
> > > >Looking at that code ... there are several
> > problems
> > > ....
> > > >
> > > >firstly the $_POST/$_GET hack won't work with
> > > register_globals = off
> > > >
> > > >Also phpgroupware has never processed the
> > external
> > > variables, I think it
> > > >is a PHP problem.  IIRC php will url_decode all
> > $_GET
> > > vars for you.
> > > >
> > > >Bit more info about where this code is will
> > probably
> > > help us track this
> > > >down.
> > > >
> > > >Cheers
> > > >
> > > >Dave
> > > >

dave.hall.vcf
Description: Card for <dave.hall@mbox.com.au>