Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:84082
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.52 as permitted sender)
Message-ID: <54F20D17.9050506@gmail.com>
Date: Sat, 28 Feb 2015 18:46:47 +0000
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0
MIME-Version: 1.0
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
CC: "internals@lists.php.net" <internals@lists.php.net>
References: <CAGa2bXYf7Bi1QPya96t0Z3cQkTWBCOG2p2gCLZSUTY=_gNp_kw@mail.gmail.com> <54F0540C.1040807@gmail.com> <CAGa2bXbgBT6A3_-ihQyNrgE-ayVX+6UtdtJOX4=xQYYz+WFFwQ@mail.gmail.com>
In-Reply-To: <CAGa2bXbgBT6A3_-ihQyNrgE-ayVX+6UtdtJOX4=xQYYz+WFFwQ@mail.gmail.com>
Content-Type: multipart/alternative;
 boundary="------------040602060104080904090608"
Subject: Re: [PHP-DEV] [RFC][DISCUSSION] Remove allow_url_include INI
From: rowan.collins@gmail.com (Rowan Collins)

--------------040602060104080904090608
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

On 28/02/2015 02:37, Yasuo Ohgaki wrote:
> Hi Rowan,
>
> On Fri, Feb 27, 2015 at 8:25 PM, Rowan Collins 
> <rowan.collins@gmail.com <mailto: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.

Yes, the global vs local setting issue is distinct from the question of 
"is phar://blah a URL?"


> 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.

I agree with this part. Global settings are always harder to work with, 
and in this case more dangerous, than something more local.


> 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');

I'm not sure most users would look at that and think "that's a URL" or 
"that's a URI". They'd think "that's some funky syntax for including a 
file from inside an archive".


> 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.

I'm not really clear how this is a "back door". At worst, it seems to be 
a form of obfuscation, like the classic eval(base64_decode(....)) etc, 
since it makes the intention of the uploaded file less obvious. But 
you've still got to activate the stream wrapper, upload the file, and 
convince the script to include it.


> 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.

Relying on the fact that no uploaded file can have a name ending ".php", 
and that therefore any file ending ".php" is safe to include, seems like 
a very naive check, best remedied with user education. It's also only 
exploitable if they rely on the include_path setting, rather than 
prefixing the input with an expected directory (since 
'/var/www/my_app/modules/phar://foo.phar/bob.php' would not be loadable).

Similarly, the only sensible way I  know to protect against ".." is to 
call realpath() or similar, which would simply return false for anything 
beginning "phar://", regardless of what path it then pointed to.


>     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.

What I don't understand is who is the "caller" and who is the "callee". 
All I can see in your examples is "global setting enabled at point X, 
and exploited at point Y"; there is no caller/callee relationship 
between those points.


> 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.

Why does the user have to pass flags around? I would have thought the 
vast majority of cases will be replacing "require 
'phar://foo.phar/bar.php';" with either "require 
'phar://foo.phar/bar.php', 'phar://'; or "require 
'phar://foo.phar/bar.php', PHP_STREAM_LOCAL". The burden of upgrading is 
basically identical.

The only difference would seem to be the fairly rare scenario of 
implementing a custom stream wrapper, and then using it for script 
includes, and that would require only a single implementation of 
getType() on the stream wrapper, which doesn't seem that complex.


> 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.

No, we can't; we can use () for string expressions right now, and 
include/require expect a single string expression. This is an important 
distinction, not just a matter of terminology.

I actually encountered a bug in real code which looked something like this:

@include('/path/to/some/library.php') && call_some_library_function();

This looks like it executes include(...) in a boolean context, and calls 
the library function if it succeeds; but it actually evaluates 
'/path/to/some/library.php' in a boolean context (non-empty string => 
true), and always attempts to call the library function, all *before* 
attempting to include anything. It is equivalent to this:

@include ( '/path/to/some/library.php' && call_some_library_function() );


> I have no intention to change current include/require syntax, except 
> adding 2nd parameter.

Then you need to remove the parentheses from your examples, since they 
are incompatible with the current syntax, unless you only treat the 
parentheses specially if there is also a comma, which seems likely to 
cause confusion:

include ('foo') . '.php';  // existing syntax; includes 'foo.php'
include ('foo', PHP_STREAM_LOCAL) . '.php';  // new syntax; includes 'foo'?


In case you think all my examples are rather unlikely, consider this 
perfectly reasonable use of parentheses:

include ($debug_mode ? 'debug_' : 'production_') . $class_name . '.php';

If the parser starts treating the parentheses as part of the include 
syntax, this will break completely.


Regards,

-- 
Rowan Collins
[IMSoP]


--------------040602060104080904090608--