[RFC] [FINAL DISCUSSION] Script only include/require

Pádraic Brady

11 years ago

Does this have any impact on allow_url_include or has that setting
been retained?

Yes, folk do indeed try to do this, for example hitting up Google:
http://www.quora.com/Why-do-include-and-require_once-not-work-with-remote-files

Paddy

On 21 February 2015 at 01:06, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi all,
>
> I think this will be the final discussion before vote.
> This RFC is to make PHP stronger against script inclusion attacks just like
> other languages.
>
> https://wiki.php.net/rfc/script_only_include
>
> I hope everyone will like this proposal.
> Thank you all who have participated to discussions.
>
> Those who are not involved, this is the time to check this RFC.
>
> Thank you.
>
> --
> Yasuo Ohgaki
> yohgaki@ohgaki.net

--

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com
Zend Framework Community Review Team
Zend Framework PHP-FIG Representative

Yasuo Ohgaki

11 years ago

Hi Padraic,

On Sat, Feb 21, 2015 at 5:18 PM, Pádraic Brady <padraic.brady@gmail.com>
wrote:

> Does this have any impact on allow_url_include or has that setting
> been retained?
>
> Yes, folk do indeed try to do this, for example hitting up Google:
>
> http://www.quora.com/Why-do-include-and-require_once-not-work-with-remote-files
>

allow_url_include=Off is kept.

Attacker can easily place *.php files on remote servers.
I guess PHP also allows php://input without it, doesn't it?
php://input allows script execution via post.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Jan Ehrhardt

11 years ago

Yasuo Ohgaki in php.internals (Sat, 21 Feb 2015 10:06:24 +0900):

>I think this will be the final discussion before vote.
>This RFC is to make PHP stronger against script inclusion attacks just like
>other languages.
>
>https://wiki.php.net/rfc/script_only_include
>
>I hope everyone will like this proposal.

Am I correct in assuning that files with extensions like .inc, .install,
.module will not be included by default anymore if this RFC is accepted?
And that only a change in the php.ini can make those files includable?

Drupal uses files like these, even in Drupal 8 beta. Would not that be a
big obstacle for installing Drupal, especially at shared hosters where
the user has no control over php.ini?

Jan

Yasuo Ohgaki

11 years ago

Hi Jan,

On Sun, Feb 22, 2015 at 12:33 AM, Jan Ehrhardt <phpdev@ehrhardt.nl> wrote:

> Yasuo Ohgaki in php.internals (Sat, 21 Feb 2015 10:06:24 +0900):
> >I think this will be the final discussion before vote.
> >This RFC is to make PHP stronger against script inclusion attacks just
> like
> >other languages.
> >
> >https://wiki.php.net/rfc/script_only_include
> >
> >I hope everyone will like this proposal.
>
> Am I correct in assuning that files with extensions like .inc, .install,
> .module will not be included by default anymore if this RFC is accepted?
> And that only a change in the php.ini can make those files includable?
>
> Drupal uses files like these, even in Drupal 8 beta. Would not that be a
> big obstacle for installing Drupal, especially at shared hosters where
> the user has no control over php.ini?
>

I forgot to write INI type.
The INI is INI_ALL, so users may use any filename extensions.

I added.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Dan Ackroyd

11 years ago

From the RFC:

> Patches and Tests
> Will be prepared before vote.

The implementation details may determine how some people vote. Is the
patch still coming before the voting is opened?

cheers
Dan

Yasuo Ohgaki

11 years ago

Hi Dan,

On Sun, Feb 22, 2015 at 12:40 AM, Dan Ackroyd <danack@basereality.com>
wrote:

> From the RFC:
> > Patches and Tests
> > Will be prepared before vote.
>
> The implementation details may determine how some people vote. Is the
> patch still coming before the voting is opened?
>

Yes. The patch will be simple one.
I'll write it in a week.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi all,

On Sat, Feb 21, 2015 at 10:06 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> I think this will be the final discussion before vote.
> This RFC is to make PHP stronger against script inclusion attacks just
> like other languages.
>
> https://wiki.php.net/rfc/script_only_include
>
> I hope everyone will like this proposal.
> Thank you all who have participated to discussions.
>
> Those who are not involved, this is the time to check this RFC.
>
> Thank you.
>

I've wrote initial patch for this RFC

https://github.com/php/php-src/pull/1111

There are some issues to resolve (error, the way comparing extension), but
it works as it supposed. Comments are appreciated.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Stas Malyshev

11 years ago

Hi!

> I think this will be the final discussion before vote.
> This RFC is to make PHP stronger against script inclusion attacks just like
> other languages.
>
> https://wiki.php.net/rfc/script_only_include

I still think this RFC takes a wrong road for the following reasons:

1. Having any code in your app that allows to run include on
user-controlled files (I'm not talking about filtered cases but user
data controlling the path) is insecure and can not be made secure. It
should just never be done. Trying to find workarounds for this is like
safe_mode - good idea in theory, leads to worse security in practice.

2. Default configuration would break tons of PHP scripts with extensions
other than .php (very frequent case). The BC break potential of this is
very big as it modifies core functionality.

3. Prohibiting phar uploads would also be a bc break, but more
importantly, there still probably are ways to work around this by using
phar files with extension different than .phar and then asking to
include files within that phar file. As long as the eventual path would
end in .php, your code would allow it.

Also, the claim that move_upload_file() is obsolete is not based on
anything as far as I can see. Why is it "obsolete"?

--
Stas Malyshev
smalyshev@gmail.com

Yasuo Ohgaki

11 years ago

Hi Stas,

On Mon, Feb 23, 2015 at 7:00 AM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> > I think this will be the final discussion before vote.
> > This RFC is to make PHP stronger against script inclusion attacks just
> like
> > other languages.
> >
> > https://wiki.php.net/rfc/script_only_include
>
> I still think this RFC takes a wrong road for the following reasons:
>
> 1. Having any code in your app that allows to run include on
> user-controlled files (I'm not talking about filtered cases but user
> data controlling the path) is insecure and can not be made secure. It
> should just never be done. Trying to find workarounds for this is like
> safe_mode - good idea in theory, leads to worse security in practice.
>

This is mitigation proposal against script inclusions. The difference is
clear
by statistics.

Because this is mitigation, it does not aims to be a perfect solution. It
aims
to make PHP as secure as other languages.

I think system admins feel more comfortable with this change, too.
They know PHP programs are very weak against script inclusion attacks
compare to other languages.

>
> 2. Default configuration would break tons of PHP scripts with extensions
> other than .php (very frequent case). The BC break potential of this is
> very big as it modifies core functionality.
>

Compatibility can be provided by one liner.

ini_set('zend.script_extensions', '.php .phar .inc .phtml .php4 .php5');

ini_set() does not emit any errors for non existing INIs.

3. Prohibiting phar uploads would also be a bc break, but more

> importantly, there still probably are ways to work around this by using
> phar files with extension different than .phar and then asking to
> include files within that phar file. As long as the eventual path would
> end in .php, your code would allow it.
>

Security is trade off relation, so I think this change acceptable trade off
to disable "script inclusion" (executing attacker programs).

Users can move uploaded files safely without move_uploaded_file() now.
I just made use of it to provide another mitigation, since script only
include
cannot be mitigation for uploading script files under docroot.

Also, the claim that move_upload_file() is obsolete is not based on

> anything as far as I can see. Why is it "obsolete"?

move_uplaoded_file() is needed for "register_globals". Attacker could
specify source files (i.e. in $_FILES) other than uploaded files with
"register_globals".

Current move_uploaded_file() checks source filename is really a
uploaded file's filename. It prevents moving other files, so it's not
completely useless but there is not real protections now because
values in $_FILES is safe now.

I know your point of view, but I hope you like this RFC.
Thank you for your comment. Your comments are very helpful to
come up with this RFC.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Jan Ehrhardt

11 years ago

Stanislav Malyshev in php.internals (Sun, 22 Feb 2015 14:00:02 -0800):

>2. Default configuration would break tons of PHP scripts with extensions
>other than .php (very frequent case). The BC break potential of this is
>very big as it modifies core functionality.

Exactly my point with the Drupal example. Drupal will break without
changes.

Jan

Yasuo Ohgaki

11 years ago

Hi Jan,

On Mon, Feb 23, 2015 at 6:32 PM, Jan Ehrhardt <phpdev@ehrhardt.nl> wrote:

> Stanislav Malyshev in php.internals (Sun, 22 Feb 2015 14:00:02 -0800):
> >2. Default configuration would break tons of PHP scripts with extensions
> >other than .php (very frequent case). The BC break potential of this is
> >very big as it modifies core functionality.
>
> Exactly my point with the Drupal example. Drupal will break without
> changes.
>

They can adopt PHP7 by one liner.

ini_set('.php .phar .module .etc');

Since ini_set() does not raise any errors, it works all PHP versions.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

On Mon, Feb 23, 2015 at 6:52 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> ini_set('.php .phar .module .etc');

ini_set('zend.script_extensions', '.php .phar .module .etc');

to be correct.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Jan Ehrhardt

11 years ago

Yasuo Ohgaki in php.internals (Mon, 23 Feb 2015 18:53:10 +0900):

>On Mon, Feb 23, 2015 at 6:52 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>
>> ini_set('.php .phar .module .etc');
>
>ini_set('zend.script_extensions', '.php .phar .module .etc');
>
>to be correct.

Quote from a Drupal 7 .htaccess:

# Protect files and directories from prying eyes.
<FilesMatch
"\.(engine|inc|info|install|make|module|profile|test|po|sh|.*sql|theme|tpl(\.php)?|xtmpl)$|^(\..*|Entries.*|Repository|Root|Tag|Template)$">
Order allow,deny
</FilesMatch>

A lot of these, even .test, are used as include scripts. How would the
corresponding .htaccess entry for zend.script_extensions be formulated?

Jan

Yasuo Ohgaki

11 years ago

Hi Jan,

On Tue, Feb 24, 2015 at 12:51 AM, Jan Ehrhardt <phpdev@ehrhardt.nl> wrote:

> Yasuo Ohgaki in php.internals (Mon, 23 Feb 2015 18:53:10 +0900):
> >On Mon, Feb 23, 2015 at 6:52 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> >
> >> ini_set('.php .phar .module .etc');
> >
> >ini_set('zend.script_extensions', '.php .phar .module .etc');
> >
> >to be correct.
>
> Quote from a Drupal 7 .htaccess:
>
> # Protect files and directories from prying eyes.
> <FilesMatch
>
> "\.(engine|inc|info|install|make|module|profile|test|po|sh|.*sql|theme|tpl(\.php)?|xtmpl)$|^(\..*|Entries.*|Repository|Root|Tag|Template)$">
>   Order allow,deny
> </FilesMatch>
>
> A lot of these, even .test, are used as include scripts. How would the
> corresponding .htaccess entry for zend.script_extensions be formulated?
>

My patch allows up to 32 extensions or disable the protection, so all of
these
can be used as PHP scripts by

ini_set('zend.script_extensions', ''); // The same as now. No mitigations.

I don't recommend disabling the protection nor adding too many extensions,
but it's up to developers.

By the way, the configuration is not for PHP script, but file access
control from
external clients. I suppose Drupal uses much less filename extensions for
PHP
scripts. Otherwise, it will be too weak against script inclusion (or direct
PHP
script access via uploaded files).

I checked Drupal8 files briefly. I see a few .module/.inc (there may be
others)
They may either rename to .module.php/.inc.php or

ini_set('zend.script_extensions', '.php .module .inc .phar');

I suggest developers to use standard extension for PHP scripts, use only a
few
extensions as PHP scripts if it's ever needed. Leaving
zend.script_extensions by
default and using '.php' as PHP script would be the best practice. I think
most
apps/scripts follow this already.

Regards,

P.S. My patch does not protect "direct access". i.e. It allows to execute
PHP
scripts specified by web server configurations when PHP is a module of web
server. e.g.

<FilesMatch \.png$>
SetHandler application/x-http-php
</FilesMatch>

allows ".png" files to be executed as PHP script regardless of
"zend.script_extensions".
PHP script files is opened by Web server and PHP will not check file
extension for
this case. The patch checks only when script file is loaded by PHP/Zend.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi all,

I would like to start voting. The last open issue is vote type

50%+1 or 2/3?

I feel this RFC is somewhere between 50%+1 and 2/3.
Any comment?

I'll start the vote by using "2/3" rule from tomorrow morning
(JST, About 12 hours later from now) if there is no comment
for vote type.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

On Sat, Feb 21, 2015 at 10:06 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: