Re: Directory traversal in source.php not fixed.

[Seth Alan Woolley <seth () tautology org>]

On Mon, Jul 04, 2005 at 01:17:50PM -0400, Kaf Oseo wrote:
> Thank you for contacting me.
>
> I've corrected, as well as further attempted to *harden*, my Quick & 
> Dirty PHPSource Printer (PHP script).
>
> The line referred below to is now:
>
> $file = (strstr($file_get, '..') == true) ? NULL : $file_get;

Actually, if I'm not mistaken again ;), it would be faster this way 
while still getting strpos speed advantages:

$file = (strpos($file_get, '..') === false) ? $file_get : NULL;

Note how we're testing for false and negating now -- I earlier made the 
mistake that (!(a === false)) is the same thing as (a === true), but 
they aren't because of the type munging going on (I'm really not used to 
php's type munging semantics).

I suggested === in my first proposed fix because php.net says to use it 
to test the return value, but they aren't explicit that this will only 
work on false.  strpos is also faster than strstr, according to 
php.net's manual of strstr.

I'll note that perl's index function is actually sane in that it uses a 
_different_ integer value for not found than one that overlaps with the 
valid set of found index positions:

       index STR,SUBSTR,POSITION
       index STR,SUBSTR
             The index function searches for one string within another, but
             without the wildcard-like behavior of a full regular-expression
             pattern match.  It returns the position of the first occurrence
             of SUBSTR in STR at or after POSITION.  If POSITION is omitted,
             starts searching from the beginning of the string.  The return
             value is based at 0 (or whatever you've set the $[ variable
             to--but don't do that).  *If the substring is not found, returns*
             *one less than the base, ordinarily "-1".*
                                                                                                                        
              
Just another reason why perl's so much easier to code secure software 
in.  I hate having to learn poorly-thought-through functions in php just 
because monkeys are taught to use it instead of a real scripting 
language. *sigh*

> Script available here:
> http://guff.szub.net/quick-and-dirty-phpsource-printer/
>
> Source can be viewed here:
> http://guff.szub.net/wp-content/sourceprt.php?file=source.php
>
> -Kaf Oseo
>
> Chew Keong Tan wrote:
> > Hi,
> >
> > I have taken a look at source.php and the vulnerability does not seem to
> > be fixed. This is due to an error in the strstr comparison in the
> > following line of code. Further, if your script is deployed in the
> > Windows platform, then "..\" sequences can also be used for directory
> > traversal.

Unfortunate if this is true from within php.

This is truly a security bug in windows or php itself since it should 
properly map directories to the posix way.  In any case, I don't really 
care if it works or does not work on windows.  Let them pay for their 
software and security; they aren't getting it for free from me.

> > $file = (strstr($file_get, '../') === true) ? '' : $file_get; // protect
> > from site traversing
> >
> > Do let us know when this has been fixed. 
> >
> > Thanks.

-- 
Seth Alan Woolley [seth at positivism.org], SPAM/UCE is unauthorized
Quality Assurance Team Leader & Security Team: Source Mage GNU/linux
Linux so advanced, it may as well be magic http://www.sourcemage.org
Secretary Pacific Green Party of Oregon http://www.pacificgreens.org
Key id 00BA3AF3 = 8BE0 A72E A47E A92A 0737  F2FF 7A3F 6D3C 00BA 3AF3

Attachment: _bin
Description:

_______________________________________________
Full-Disclosure - We believe in it.
Charter: http://lists.grok.org.uk/full-disclosure-charter.html
Hosted and sponsored by Secunia - http://secunia.com/