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

[C K Wu]

Hi Dave,

I hate to be a pest.

However, I think phpgwapi's nextmatchs class is
actually passing get values as
"arg1=val1&arg2=val2&.." .

In .../phpgwapi/inc/class.nextmatchs.inc.php,

>From line 744, within function show_sort_order, it
reads,

if (is_array($extra))
{
        $extra = $this->extras_to_string($extra);
}

$extravar =
'order='.$var.'&sort='.$sort.'&filter='.$filter.'&qfield='.$qfield.'&start='.$start.'&query='.urlencode(stripslashes($GLOBALS['query'])).$extra;

$link =
($this->action?$this->page($extravar):$GLOBALS['phpgw']->link($program,$extravar));


If I read it correctly, it actually turns $extra into
a string if it is an array.  Again, $filter and $query
are  probably most exposed to the '=' and '&' problems
that we've discussed.

To clarify further, I have done a "grep -R 'link(' *"
on the phpgwapi folder.  It shows that a lot of calls
to  $GLOBALS['phpgw']->link() are actually passing
extra get values as "arg=val" strings.  They don't
show up as problem only because no non-alphanumeric
data is involved.  However, there may yet be other
nextmatchs->show_sort_order's lurking behind the
scene.

Because show_sort_order is the primary function to
allow retrieved info to be re-sorted, I am going back
to double urlencode/decode.

I would suggest that a detail review of phpgwapi is
needed to ensure the change in session $extravar
handling is not creating hidden bugs waiting to be
discovered.

Cheers,
CK


 --- Dave Hall <address@hidden> :

C K Wu <address@hidden> wrote:
> 
> > Hi, Dave,
> > 
> > Thank you for spotting the security flaw.  I'll
> > certainly tighten up security further.  
> 
> No problem.  Good to hear :)
> 
> > At the same
> > time, '=' within the GET value string is a general
> > problem.  Any unstructured GET value string has
> the
> > potential of including a '=' char, thus causing
> > problem to the callee script.  Actually, this '='
> > problem could easily be rectified.  Instead of a
> > general 'split', class.sessions.inc.php could
> simply
> > pick on the 1st '=' char and assign the pre-'='
> string
> > to GET arg and the post-'=' string to GET value. 
> > However, the '&' char could be much more
> difficult.  A
> > double urlencode/decode seems to be the easiest
> > solution.
> > 
> > Any thoughts ?
> 
> Pass the $extravars arg of the link() method an
> array, not a string. 
> That is how it is designed to be used.  So then =
> and & are properly
> encoded, along with all other values.  I this is a
> little bit of work,
> but it will make it run faster, as no explode()ing
> is done on the
> $extravars variable within the link() method, it
> just url_encode()s it.
> 
> Hope this helps
> 
> Cheers
> 
> Dave


_________________________________________________________
最新鈴聲推介:遇見，亂世佳人，假如愛有天意...
http://ringtone.yahoo.com.hk