Hi all,
On Sat, Feb 28, 2015 at 11:37 AM, Yasuo Ohgaki <yohg...@ohgaki.net> wrote:
> On Fri, Feb 27, 2015 at 8:25 PM, Rowan Collins <rowan.coll...@gmail.com>
> wrote:
>
>> Yasuo Ohgaki wrote on 27/02/2015 03:44:
>>
>>> Hi all,
>>>
>>> This is RFC for removing "allow_url_include" INI option. [1]
>>>
>>> During "Script only include" RFC[2] discussion, stream wrapper issue is
>>> raised.
>>> I was thinking this issue as a separate issue, but it seems others are
>>> not.
>>>
>>
>> I'm not convinced by the argument that because "phar://blah" looks like a
>> URL, allowing it makes allow_url_include broken. Perhaps it would be better
>> named allow_remote_include, but it corresponds to masking out your
>> PHP_STREAM_REMOTE flag further down, which is the more important
>> protection. If you want to be able to disable phar:// access, you could add
>> something like allow_local_stream_include for that case without breaking BC.
>>
>
> allow_local_stream_include is one possible solution. It has the same
> problem with allow_url_include.
> Since allor_url_include is global INI, it applies to child scripts. e.g.
>
> A.php
> -----
> <?php
> ini_set('allow_url_include', 'On');
> include('B.php');
> -----
>
> B.php
> -----
> <?php
> include($_GET['var']); // Not only local script execution vulnerable, but
> also remote script execution is possible because "allow_url_include=On"
> here.
> -----
>
> We need to control these flags more precisely. That's the reason why I
> proposed 2nd parameter for include.
> If flags are globals, we cannot control them as it should.
>
> URL and URI is the same for most users, I think. (It differs, but their
> usage is mostly the same) Not
> many users expect to work something like
>
>
> include('new_wrapper://some_file_with_special_content/some_how_php_script_is_accecible_suddenly');
>
> Attackers can write malicious user stream wrapper as back door. Then
> upload attack file and takeover
> server. The wrapper will be hard to distinguish if it's legitimate or not.
> Attack file can be any kind of
> format. It's impossible to detect as malicious file. URL formed script
> (stream wrapper) is very flexible/handy/
> hard to be detected tool for attackers.
>
> Remote resource is extremely danger. Local resource is very dangerous
> also. Current phar:// wrapper
> implementation spoils file extension and ".." (double dot) protection that
> is deployed by many PHP codes
> already. User wrapper seems allowed for include(), so malicious user
> wrapper can help attackers to
> exploit PHP servers.
>
>
> I'm also not at all clear what you mean by "caller" and "callee"
>> responsibilities; surely the difference is just between a global option
>> (ini_set()) and a local one (extra argument)? And in what way does Option
>> #2 require more changes than Option #1, since they both require the
>> argument to be present whenever a stream wrapper is used?
>>
>
> First question is answered in previous comment. INI cannot control flags
> precisely...
>
> Option #1 is simple string comparison for filename is passed. It does not
> care what's the filter at all nor even filter name is valid or not. 2nd
> parameter is
> used to make sure filename passed to include() is user's intension. e.g.
>
> -----
> // somewhere deep in code
> $module_name = substr($_GET['module'], -4, 4) === '.php' ?
> $_GET['module'] : ''; // Do not care to much, we have safe .php files.
>
> // somewhere far from above code
> include($module_name); // phar://evil_phar/evil_script.php can be executed.
> -----
>
> With this RFC, if 2nd parameter is omitted
>
> ------
> include($module_name); // E_RROR, URL formed include(stream wrapper) is
> not allowed.
> ------
>
> to use phar. User must be explicit.
>
> -----
> include($module_name, 'phar://');
> -----
>
> Option #2 will be much more complex than Option #1, since it introduces
> types of wrapper, force users to have class method return wrapper type and
> pass flags around functions to handle wrapper types correctly. There may be
> more.
>
>
> I do think local options are better than global ini settings in many
>> cases, but include/require/etc are statements, not functions, so giving
>> them extra arguments is awkward - some of your examples are "wrong" in this
>> regard:
>>
>> // Redundant brackets make this look like a function, but it's not:
>> include('phar://phar_file/script.php');
>>
> // I can add as many as I like, the parser is just resolving them to a
>> single string expression:
>> include(((('phar://phar_file/script.php'))));
>> // This is the actual syntax:
>> include'phar://phar_file/script.php';
>> // Implying this for arguments:
>> include'phar://phar_file/script.php', 'phar://';
>> // You could explicitly allow a "function form" of the statements, so you
>> could parse this:
>> include(('phar://phar_file/' . $script_name), 'phar://');
>> // But then you've got a subtle BC break, because the interpretation of
>> this changes:
>> include ($foo) . ('.php');
>>
>
> I used include('phar://phar_file/script.php'), but it can be include
> 'phar://phar_file/script.php'.
> I'm using () since it seemed harder to distinguish parameters and other
> text as we can
> use () for include/require now.
>
> I have no intention to change current include/require syntax, except
> adding 2nd parameter.
>
> Thank you for your feedback. I hope I explained well enough.
> I'm thinking merging this RFC to "Script only include" RFC. I have to
> start over, but it seems
> I must do this.
>
> I'll use Option #1 since it's much easier/simpler/less BC/more precise.
> However,
> ideas are appreciated always.
>
In short, I'm saying we have serious security problem in our current stream
wrapper
implementation and it's friends including include/require.
Regards,
--
Yasuo Ohgaki
yohg...@ohgaki.net
