[RFC][DISCUSSION] Remove allow_url_include INI View as Markdown

Hey:

Hi Xinchen,

hmm, does that means, if this RFC won't pass, then script only include
RFC should also be rejected?

if yes, then maybe you should put them together?

Sorry I just sent previous mail before your mail.

We need to fix this regardless of
https://wiki.php.net/rfc/script_only_include
If we have both, we close the door for "arbitrarily script execution".
(I mean almost the same as other language level)

Sorry, but I am confused by the point, do you want to disable include
a remote php file or not?

if yes, how about with allow_url_fopen?

eval(file_get_contents(http://xxxxxx/));

thanks

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Hi all,

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.

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
yohgaki@ohgaki.net

Hi!

I have no intention to change current include/require syntax, except adding
2nd parameter.

This is a bit misleading since include is not a function, so there's no
1st parameter. Instead, it's a syntax construct. Of course, syntax
construct's grammar can be changed, though I'm not sure if that wouldn't
lead to unexpected effects in the parser. But maybe not.

Bigger issue is that if somebody is knowledgeable enough in security to
go and change their code to use this option, why they aren't
knowledgeable enough to fix their includes so this option is not required?

--
Stas Malyshev
smalyshev@gmail.com

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]