Hi Rowan,
On Sun, Mar 1, 2015 at 3:46 AM, Rowan Collins <rowan.coll...@gmail.com>
wrote:
> On 28/02/2015 02:37, Yasuo Ohgaki wrote:
>
> Hi Rowan,
>
> 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.
>
>
> Yes, the global vs local setting issue is distinct from the question of
> "is phar://blah a URL?"
I think your question is about remote/local resource.
Is phar:// local resource? It's yes.
Is phar:// URL/URI? I think it depends how people understand URL/URI.
file:// is considered URI/URL even if it's local resource.
> 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".
>
PHP allows too many wrappers for include/require. We may be better to
disable almost all wrappers.
Phar remains as problematic wrapper. Solution would be adding way to load
script precisely.
e.g. Restrict filename extension, specify URL/URI prefix.
Note: We need to have either, specific file name for scripts or script
loader must be specific for wrapper to be used.
> 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.
Nice attribute of attack with wrapper is actual attack code can be hidden
in other data files.
The data file may have any kind of form. This makes detection a lot harder.
> 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).
>
Right. '/var/www/my_app/modules/phar://foo.phar/bob.php' is invalid.
However, relative path is allowed...
Using Stas' exploit example
php chump.php phar://../cuteponies.gif/pwnd.php
This is executes attack code.
>
> 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.
This is interesting idea for mitigation. Thank you.
> 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.
>
>
The include() in the child script's condition (allow_url_include) must be
controlled by the caller.
i.e. ini_set('allow_url_inlcude', 'Off') is needed before include/require
if it works.
Current PHP does not allow this by making "allow_url_include" INI_SYSTEM.
Almost all PHP
codes assume "allow_url_include=Off". If caller needs to set
"allow_url_include=On" for some
script, all of child scripts are affected since there is no proper
caller/callee responsibility segregation.
There are 2 possible solutions
- Make allow_url_include flag a specific flag for each include/require.
- Make sure allow_url_include is desired setting before calling
include/require. i.e. ini_set('allow_url_include', 'On/Off') for every
include/require.
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.
>
It's just more complex than 'URL prefix' check.
I don't mind adopting/implementing this method.
> 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() );
Interesting code example. Thank you.
We can use return value from script. i.e. We can use script file as a
function call with include/require. I didn't think of this. I'll keep this
in my mind.
>
> 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.
include ('foo') . '.php'; // existing syntax; includes 'foo.php'
This one is another good example, too. Thank you.
I think I'm better to merge this RFC to "Script only inclusion" RFC.
I'll do this so that there is no confusions.
Regards,
--
Yasuo Ohgaki
yohg...@ohgaki.net
