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

[Dave Hall]

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

> Hi Dave,
> 
> I hate to be a pest.

No you are not a pest.  I think this thread will provide some good
adivce for other developers.

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

Yeah, not everything is done as it should be in phpGroupWare.  The link
method is designed so $extravars can be an array or a string, but for it
to function properly is $extravars should be an array.  


> 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.

Ok that looks like it is creating a lot of work, which doesn't need to
be done.


> 
> 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.

Yeah, we will check it, and alter the behavoir of the offending calls.

> 
> 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
> 
> 
> _________________________________________________________
> ³Ì·s¹aÁn±À¤¶:address@hidden
> http://ringtone.yahoo.com.hk
> 
>

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