Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:84064
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.174 as permitted sender)
MIME-Version: 1.0
Sender: yohgaki@gmail.com
In-Reply-To: <54F0540C.1040807@gmail.com>
References: <CAGa2bXYf7Bi1QPya96t0Z3cQkTWBCOG2p2gCLZSUTY=_gNp_kw@mail.gmail.com>
 <54F0540C.1040807@gmail.com>
Date: Sat, 28 Feb 2015 11:37:04 +0900
Message-ID: <CAGa2bXbgBT6A3_-ihQyNrgE-ayVX+6UtdtJOX4=xQYYz+WFFwQ@mail.gmail.com>
To: Rowan Collins <rowan.collins@gmail.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a113ac678aad23a05101cde64
Subject: Re: [PHP-DEV] [RFC][DISCUSSION] Remove allow_url_include INI
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

--001a113ac678aad23a05101cde64
Content-Type: text/plain; charset=UTF-8

Hi Rowan,

On Fri, Feb 27, 2015 at 8:25 PM, Rowan Collins <rowan.collins@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.

Regards,

>
--
Yasuo Ohgaki
yohgaki@ohgaki.net

--001a113ac678aad23a05101cde64--