Re: REMINDER - stream wrappers in include_path
| From: | Gregory Beaver | Date: | Wed, 26 Mar 2008 03:46:45 +0000 |
| Subject: | Re: REMINDER - stream wrappers in include_path | ||
| References: | 1 2 3 4 5 6 7 8 9 | Groups: | php.internals |
| Request: | Send a blank email to internals+get-36525@lists.php.net to get a copy of this message | ||
Marcus Boerger wrote:
> 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)
<snip>
Hi Marcus,
It looks like I wasn't clear - the problem is not checking characters
before the :, but after. With the existing code, if the string ended
with ":" the code would attempt the check ":" +1 and ":" + 2, which
could result in out-of-bounds read (read after the terminating '\0'.
However, in answer to your other question, we do need stream wrappers
longer than 1 character, as we would otherwise could be unnecessarily
checking drive letters on windows thinking they're stream wrappers.
However, I took your excellent suggestions into account in the attached
patch, which fully works and prevents use of either . or .. as stream
wrapper names.
Otherwise, its identical to wrapper6.patch.txt
One last check by Dmitry before commit and we should be good to go.
Thanks for the catch on the logical problem Stas and the coding
suggestions Marcus - this is the kind of teamwork that I dream of. The
open source gods must be looking down on us kindly.
Greg