Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:36515
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 85.214.94.56 as permitted sender)
Date: Tue, 25 Mar 2008 20:17:29 +0100
Reply-To: Marcus Boerger <helly@php.net>
Message-ID: <1891676371.20080325201729@marcus-boerger.de>
To: Gregory Beaver <greg@chiaraquartet.net>
CC: Stanislav Malyshev <stas@zend.com>, Dmitry Stogov <dmitry@zend.com>, 
	Stefan Walk <et@php.net>, <internals@lists.php.net>, 
	Marcus Boerger <helly@php.net>
In-Reply-To: <47E94C24.2030400@chiaraquartet.net>
References: <47E2F8FA.20107@chiaraquartet.net> <47E7FEFD.8080504@chiaraquartet.net> <47E800F1.5090101@zend.com> <200803251613.20245.et@php.net> <47E91898.5050503@zend.com> <47E92D3B.7010500@chiaraquartet.net> <47E930C0.6000201@zend.com> <47E94C24.2030400@chiaraquartet.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] REMINDER - stream wrappers in include_path
From: helly@php.net (Marcus Boerger)

Hello Gregory,

Tuesday, March 25, 2008, 8:01:56 PM, you wrote:

> Stanislav Malyshev wrote:
>>> stream wrapper.  Here is an example:
>>>
>>> oops.broken://UNC/path
>>
>> I wonder if .://UNC/path is treated as "."+//UNC/path (and the same
>> for ..). It should anyway :) However I'm not too worried without
>> pathes like foo.bar - not likely to have path without any slashes
>> unless it's . or .., and if you do, you always can say ./foo.bar
>>
> That's a great question.  In attempting to answer, I think I may have
> unfortunately found a severe flaw in the patch, allowing reading past
> the end of the filename and the include_path.

> If we pass a file named "hello:" to php_resolve_path, this code:

> if ((*p == ':') && (p - filename > 1) && (p[1] == '/') && (p[2] == '/')) {

> would look past the end of the filename by 1 (p[1] = '\0', but p[2] is
> an off by one read).  Instead, we should be checking to see if p -
> filename < filename_length - 2 so we have enough room for the stream
> wrapper and 1 character (i.e. blah://a, not blah://).

How so?

Just reorder and put (p - filename > 1 first)
Here also note that this checks for two chars not just one. One woudl be
simplier as it could be done with: (p > filename)
The remainder is: p[0] == ':' && p[1] == '/' && p[2] == '/'
Now thanks to C rules p[1] is never accessed if p[0] is not ':' which for
instance is true for '\0'. So unless p can be of or filename is not zero
terminated we are fine.

Now p is set to filename initially and checks for some chars. So it can
only whatever but must be equal to filename or greater filename.

So in the end you can do:
if (*p == ':' && ++p > filename && *p == '/' && p[1] == '/')
Only a bit harder to read. The only question that still remains to me is
why you require two chars in front of the ':'. Also to deal with '.' and
'..' you can simply add a check for p[-1] != '.' or p[-2] != '.' after the
++p.

> To get around the problem Stas raises, we need to disallow "." or ".."
> as stream wrapper names in php_stream_locate_url_wrapper and check
> for them explicitly in php_resolve_path.

> The attached patch is identical to Dmitry's wrapper6.patch.txt and fixes
> these two issues.

> Note that the check for "." and ".." is only needed when scanning
> include_path, and the part at the end that checks dirname(__FILE__) does
> not need it because there is no way __FILE__ could be
> .://path/to/something or ..:/path/to/something if . and .. are
> disallowed as stream wrapper names.

> Unfortunately, I don't have quite enough time to get the attached patch
> working, but I'm including it for the smarties to figure out how to
> handle, I've put /* XXX FIXME */ where things need work.

> Thanks,
> Greg

marcus

Best regards,
 Marcus