Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112348
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> <CAN2symii22mQknY=Y=zxJDF+EdZ0XcBBc8FsFTyEYR=PTY6k_g@mail.gmail.com>
In-Reply-To: <CAN2symii22mQknY=Y=zxJDF+EdZ0XcBBc8FsFTyEYR=PTY6k_g@mail.gmail.com>
Date: Tue, 1 Dec 2020 18:38:54 +0000
Message-ID: <CAFPFaMJFndsXqw2h8PE0DpAKU0S5yNE-6jG-wAkXRBYdDBrJ-A@mail.gmail.com>
To: Paul Crovella <paul.crovella@gmail.com>
Cc: "Christoph M. Becker" <cmbecker69@gmx.de>, "Aimeos | Norbert Sendetzky" <norbert@aimeos.com>, 
	PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000002d0cad05b56b712d"
Subject: Re: [PHP-DEV] Re: PHP 8 is_file/is_dir input handling
From: george.banyard@gmail.com ("G. P. B.")

--0000000000002d0cad05b56b712d
Content-Type: text/plain; charset="UTF-8"

On Tue, 1 Dec 2020 at 18:35, Paul Crovella <paul.crovella@gmail.com> wrote:

> On Tue, Dec 1, 2020 at 10:23 AM G. P. B. <george.banyard@gmail.com> wrote:
> >
> > On Tue, 1 Dec 2020 at 18:07, Paul Crovella <paul.crovella@gmail.com>
> wrote:
> >>
> >> On Tue, Dec 1, 2020 at 9:43 AM Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
> >> >
> >> > On 01.12.2020 at 18:35, Aimeos | Norbert Sendetzky wrote:
> >> >
> >> > > Am 01.12.20 um 18:24 schrieb Christoph M. Becker:
> >> > >>
> >> > >>> In PHP 7, this returns FALSE:
> >> > >>>
> >> > >>> php -r 'var_dump(is_file("ab\0c"));'
> >> > >>>
> >> > >>> In PHP 8, the same code throws a ValueException. Problem is now
> that
> >> > >>> it's not possible to check upfront if the passed argument is a
> valid
> >> > >>> path to avoid the exception being thrown.
> >> > >>
> >> > >> This is only about the NUL byte in the filename.  You can easily
> check
> >> > >> for that yourself. :)
> >> > >
> >> > > There may be other checks that will throw a ValueException. I'm not
> sure
> >> > > how it's implemented in detail because the filestat.c file doesn't
> >> > > thrown an exception at all:
> >> >
> >> > The exception is thrown from inside the parameter parsing routines
> >> > (zend_parse_parameters() and friends).  Internal function
> differenciate
> >> > between string and path, whereas the latter is an arbitrary string
> which
> >> > does not contain NUL bytes.
> >> >
> >> > It would likely make sense to document that.  OTOH, it's probably a
> good
> >> > idea to check (almost) all user input for NUL bytes.
> >>
> >> Would it not make more sense for something like is_file to have
> >> obvious sane behavior and simply return false itself? I don't
> >> understand the resistance to making it more difficult for a developer
> >> to screw something up.
> >>
> >> --
> >> PHP Internals - PHP Runtime Development Mailing List
> >> To unsubscribe, visit: https://www.php.net/unsub.php
> >
> >
> > 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.
>
> That's correct, it shouldn't have.
>
> > Would it have been fine if this would have been a TypeError as it was
> > originally intended?
>
> No, I imagine it would've been fixed sooner.
>

Incorrect, because using strict_types would make this exact case throw a
TypeError in PHP 7. See: https://3v4l.org/0P2O4


> > Is a warning fine because null bytes indicate a potential attack as in
> no sane
> > context should null bytes be passed around?
>
> Null bytes come from many places and do not necessarily indicate an
> "attack." There's plenty of UTF-16 in the world, for example, that's
> got oodles of them.
>
> > 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
>
> Because it didn't break userland code for 8 years. A ValueError is a
> much louder thing - that's the whole point of it in fact. It shouldn't
> be a surprise that it comes up now.
>

Because returning null instead of false is not a surprise?


George P. Banyard

--0000000000002d0cad05b56b712d--