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 <mailto: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?"
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]
