Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:103215
MIME-Version: 1.0
In-Reply-To: <CANZOY=BKsrVnKCO66JfXewFfBxMUOZQgkaSiQObFSvOofPeA9w@mail.gmail.com>
References: <CANZOY=C6Zmh4QdxQvyzkErqPrJ0R-Xwq8ye9MYQmE085=n71cQ@mail.gmail.com>
 <CAPhkiZyvaEg19WS5wXTU=1MwaKHu1=w0qYPfpV1Ex9nv_9LEpA@mail.gmail.com>
 <CANZOY=B-2igFBVCYGDrZT2a11B0xMXRNeWPXh6jxT_EZ_O2LUQ@mail.gmail.com>
 <CAPhkiZwDYEjG9wz0exMQcv3dAXApHEa6BJ-AULqq_kYt=ur-VQ@mail.gmail.com> <CANZOY=BKsrVnKCO66JfXewFfBxMUOZQgkaSiQObFSvOofPeA9w@mail.gmail.com>
Date: Fri, 21 Sep 2018 14:36:38 +0300
Message-ID: <CAPhkiZzCNONNa=eKD+7+bthaPrj8fbBFY_T+uZ1X1wsPku3L7g@mail.gmail.com>
To: Arnold Daniels <arnold@jasny.net>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] Add FILTER_VALIDATE_INCLUDE validation filter for
 variable includes
From: narf@devilix.net (Andrey Andreev)

Hi,

On Thu, Sep 20, 2018 at 11:30 PM, Arnold Daniels <arnold@jasny.net> wrote:
>
>
> On Thu, Sep 20, 2018 at 8:50 PM Andrey Andreev <narf@devilix.net> wrote:
>>
>> Hi again,
>>
>>
>> On Thu, Sep 20, 2018 at 5:29 PM, Arnold Daniels <arnold@jasny.net> wrote:
>> >
>> > Variable includes have proper purposes, like for a (PSR-4) autoloader.
>> > This
>> > can't be simply replaced with an 'if' statement. Other reasons are
>> > module
>> > inclusion and generated code.
>> >
>>
>> Of course, there are a few valid applications even for the most
>> discouraged practices - in that you are correct. But you should know I
>> meant *user input* variable inclusion in particular.
>> Either way, I don't see how a *user input filter* validator would help
>> PSR-4, generated code and/or whatever you meant by module inclusion.
>
>
> With module inclusion, I mean the way Symfony works with Bundles and
> Wordpress works with plugins.
>

I'm familiar with neither, sorry.

> To be honest, few applications seem to have static include statements at
> all. It's all dynamic.
>

Again, talking about user inputs. Yes, most includes are dynamic, but
still controlled by the developer and not with user-supplied strings.
In fact, since you mentioned PSR-4 which is used for pretty much
everything nowadays, you should know that almost nobody writes
"include" anymore and it's all handled by the autoloader in a
bullet-proof manner.

>>
>>
>> Just to be clear, in everything I'm saying, I assume you want to solve
>> the following problem:
>>
>>     include $_GET['page'].".php"; // <-- vulnerability
>>
>> (simplified, of course)
>
>
> With all the abstraction you see in modern applications, it's difficult to
> tell if a variable could have been influenced via the HTTP request. It's
> better always to validate the variable before using it in the include.
>

... and I noted that was a simplified example.

But while it is true that many variables are influenced by the HTTP
request, I fail to recall a valid case where they're not already
validated by a router mapping variables to pre-defined classes,
already handled by an autoloader.

This is getting kind of hard to follow, we may not even be talking
about the same thing at this point. Can you provide an example of a
case where your proposed filter would solve a problem?

>>
>>
>> > Variable inclusion is already done very often. I don't think this filter
>> > will persuade people to do it that would otherwise not. This is a common
>> > security issue. So if variable inclusion isn't disabled in full, having
>> > a
>> > common way to prevent such issues seem like a good idea to me.
>> >
>>
>> Well, I've got two things to say about this:
>>
>> 1. The developers who intruduce such vulnerabilities are the ones who
>> don't do validation in the first place, so the way I see it, the few
>> poor souls who might benefit from your proposal wouldn't care for it
>> anyway.
>
>
> If there is a proper way to do it, they'll probably be more inclined to do
> it. It's the same as for SQL injection. There is a right and a wrong way to
> inject variables.
>

That's just wishful thinking; I've never seen a developer who doesn't
do validation all of a sudden go "oh, there's this validator, I should
use it!". They don't do it because they don't care about it, not
because tools don't exist ... we already have tools for everything
that the average developer needs. And not that it matters, but SQLi
prevention is quite different.

>>
>> 2. Reiterating from my previous reply, this would only serve as an
>> excuse for some to say that user input variable inclusion is an OK
>> thing to do, because "see, PHP has a tool specifically for it, so it
>> must be good".
>
>
> There are probably not many scripts where you see direct user input being
> used for `include`. It's always an indirect hack.
>
> If you link the 'filter' extension to request data, perhaps the function
> should not be in this extension.
>

I don't understand what you're saying here ... I thought you were
trying to solve a security problem, now all of a sudden that's not
related to request data? I'll wait for that example I asked for.

>>
>>
>> >> Sanitization is more often than not imperfect and there's always the
>> >> potential to bypass it.
>> >
>> > This would be a validation filter and not a sanization filter. Can you
>> > give
>> > an example on how you could bypass it?
>> >
>>
>> Sorry, you started the discussion by mentioning sanitization and I
>> just went with it without noticing the details.
>>
>> Still, even as a validator this can be bypassed depending on the
>> application architecture - sure, you specify a base path, but who is
>> to say I'm not trying to RCE via something within that base path?
>
>
> You should not allow uploading files with `base_path`. Current RFCs aren't
> solving this issue and I don't see how to solved this, short of disalowing
> variable for includes completely. But that's unacceptable since most
> applications rely on it.
>

"Should not" doesn't mean people don't do it, and I wasn't talking
about uploaded files anyway. Either way, that's a moot point ... my
position is that there's no value in the proposed feature, regardless
of whether it can be bypassed or not.

Cheers,
Andrey.