On Tue, Dec 1, 2020 at 7:58 PM Christoph M. Becker <cmbecke...@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
