Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:103199
MIME-Version: 1.0
In-Reply-To: <CANZOY=C6Zmh4QdxQvyzkErqPrJ0R-Xwq8ye9MYQmE085=n71cQ@mail.gmail.com>
References: <CANZOY=C6Zmh4QdxQvyzkErqPrJ0R-Xwq8ye9MYQmE085=n71cQ@mail.gmail.com>
Date: Thu, 20 Sep 2018 14:11:23 +0300
Message-ID: <CAPhkiZyvaEg19WS5wXTU=1MwaKHu1=w0qYPfpV1Ex9nv_9LEpA@mail.gmail.com>
To: Arnold Daniels <arnold.adaniels.nl@gmail.com>
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 1:37 PM, Arnold Daniels
<arnold.adaniels.nl@gmail.com> wrote:
> There are many security issues that arise from not sanitizing a variable
> before using it in an include (eg `include $script;`).
>
> The filter extension is intended to prevent this kind of security issues. A
> validation filter would make it easier and could be the defacto standard
> when using variable includes.
>
> When a static code analyzer is used, it can check if the filter has been
> used and the variable is safe to be used in include.
>
> The options could be "base_path, allowed_streams".
>
> The base_path option defines the path where the file should be in. Dots
> like `..` are resolved. Home paths, like `~/foo` and `~arnold/` are not
> allowed (or resolved). Symlinks are not considered.
>
> The `allowed_streams` option would set which streams are allowed. By
> default none. I feel this is a better option than relying on
> 'allow_url_include' or RFC: Precise URL include control (
> https://wiki.php.net/rfc/allow_url_include).
>
>     include filter_var($script, FILTER_VALIDATE_INCLUDE, ["base_path" =>
> "path/to/project/", "allowed_streams" => ["phar", "zip"]]);
>
> What do you think?
>
> Also, does this require an RFC or should I just create a PR?
>

Those security issues arise from using user inputs as includes at all,
not just from lack of sanitization. Sanitization is more often than
not imperfect and there's always the potential to bypass it.

The only acceptable way of doing something similar to variable
includes is the following:

    if ($input === 'foo') {
        require 'path/to/foo.php';
    }

... which is not in fact a variable include.

Adding the filter that you're proposing would imply that doing input
variable includes is ok, and I am very strongly against this.

Cheers,
Andrey.