Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112354
MIME-Version: 1.0
References: <b845d5bf-63da-d564-1f0f-cc934188d693@aimeos.com>
 <b546b5bf-27f8-62cc-cc4b-d859132998a4@gmx.de> <0774c293-afd7-d8b9-175f-217ed600d1ea@aimeos.com>
 <fad4cc49-0f40-53f1-400c-90ff01ffdccb@gmx.de> <CAN2symiHHJVGvaLYMD=Qes0Wy06dZRtzPeonNBUWOBRbiTdo5g@mail.gmail.com>
 <CAFPFaMLEdq0+hYkE49YVbcFvCt00rne6roApcoyY4aK=1ZHpuA@mail.gmail.com>
 <29529061-dc71-c759-590a-b4786936f8c5@aimeos.com> <96e40442-a649-f9af-a0cc-dd43cfd1bd0c@gmx.de>
In-Reply-To: <96e40442-a649-f9af-a0cc-dd43cfd1bd0c@gmx.de>
Date: Tue, 1 Dec 2020 20:53:59 +0100
Message-ID: <CAF+90c-xkV0PUBmOysrQYKoH2-muw0RHSamNJmM27HX1n+Px1Q@mail.gmail.com>
To: "Christoph M. Becker" <cmbecker69@gmx.de>
Cc: "Aimeos | Norbert Sendetzky" <norbert@aimeos.com>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000ce791905b56c7d89"
Subject: Re: [PHP-DEV] Re: PHP 8 is_file/is_dir input handling
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000ce791905b56c7d89
Content-Type: text/plain; charset="UTF-8"

On Tue, Dec 1, 2020 at 7:58 PM Christoph M. Becker <cmbecker69@gmx.de>
wrote:

> On 01.12.2020 at 19:38, Aimeos | Norbert Sendetzky wrote:
>
> > Am 01.12.20 um 19:23 schrieb G. P. B.:
> >
> >> So why having is_file()/is_dir() throw a warning for the past 8 years
> >> (since PHP 5.4) a non-issue? Because by that logic it shouldn't
> >> have been emitting warnings either.
> >> Would it have been fine if this would have been a TypeError as it was
> >> originally intended?
> >> Is a warning fine because null bytes indicate a potential attack as in
> no
> >> sane
> >> context should null bytes be passed around?
> >>
> >> I don't personally *care* that it throws a ValueError, but why is this
> >> issue only
> >> brought up *now* when it should have been shouting for 8 years and is
> >> either an
> >> indication of a bug or of something larger at play.
> >
> > Keep cool, the code we are currently using is similar to this one:
> >
> > if( @is_file( $data ) === false ) {
> >     throw new \Aimeos\MW\Exception( 'Invalid file' );
> > }
> >
> > We use the silence operator to suppress the warning so we can throw our
> > own exception in a clean way. Now, with support for PHP 8 it would be:
>
> However, if $data contains a NUL byte, no exception would be thrown,
> since is_file() returned NULL in that case.
>
> Regards,
> Christoph
>

So, there's two dimensions to this:

First, assuming that a null byte in a file name *is* an error condition, is
the PHP 8 behavior better than in PHP 7? I think the answer to this one is
very clearly "yes". The above code snippet and the subtle way in which it
is broken is a great illustration of that. PHP 8 makes it obvious that the
cited code is incorrect.

Second, should a null byte in a file name be an error condition in the
first place? This is a point I'm not sure about. It would certainly be
possible to treat paths containing null bytes as non-existing paths, and
abstract away the fact that paths cannot contain null bytes, and that this
is a common attack vector.

I'd personally lean towards not considering null bytes an error condition.
However, this is an entrenched notion, and undoing this long standing
assumption will be quite a bit of work. Just changing a few functions like
is_file() would be simpler though.

Regards,
Nikita

--000000000000ce791905b56c7d89--