PHP 8 is_file/is_dir input handling

4 years ago by Christoph M. Becker — view source — reply

unread

PHP 8 is stricter in checking input data then PHP 7. This is good but
has some side effects for is_file(), is_dir() and similar functions when
invalid paths are passed for checking.

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. :)

Regards,
Christoph

4 years ago by Aimeos | Norbert Sendetzky — view source — reply

unread

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:

https://github.com/php/php-src/blob/1e9db80d7264911fa4089cb7e4b3dc7f97b19c6e/ext/standard/filestat.c

Can you tell me how you would check for NULL bytes?

Best,

Norbert

4 years ago by Christoph M. Becker — view source — reply

unread

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.

https://github.com/php/php-src/blob/1e9db80d7264911fa4089cb7e4b3dc7f97b19c6e/ext/standard/filestat.c

Can you tell me how you would check for NULL bytes?

See e.g. https://3v4l.org/5WK3n.

Regards,
Christoph

4 years ago by Paul Crovella — view source — reply

unread

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.

4 years ago by G. P. B. — view source — reply

unread

On Tue, Dec 1, 2020 at 9:43 AM Christoph M. Becker cmbecker69@gmx.de
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.

--

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.
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.

Best regards,

George P. Banyard

4 years ago by Paul Crovella — view source — reply

unread

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.

--

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.

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.

4 years ago by G. P. B. — view source — reply

unread

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:

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.

--

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

4 years ago by Aimeos | Norbert Sendetzky — view source — reply

unread

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:

try
{
if( @is_file( $data ) === false ) {
throw new \Aimeos\MW\Exception( 'Invalid file' );
}
}
catch( \ValueException $e )
{
throw new \Aimeos\MW\Exception( $e->getMessage(), 0 , $e );
}

But it has two problems:

It's much more code for the same thing
The stack trace begins at the new exception, not the one at is_file()

We can't use the ValueException directly because this will lead to two
different exceptions depending on the used PHP version and our unittests
currently fails because of the different exceptions.

Norbert

4 years ago by Christoph M. Becker — view source — reply

unread

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

4 years ago by Nikita Popov — view source — reply

unread

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

4 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

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

For me as a user that would be a very clear "no". Now if I have any
usage of these functions in my existing code, I have to go and replace
them with safe wrapper to ensure it doesn't bail out in random places.
It may be the same for functions like fopen() where you have to
error-check anyway, but for functions like is_* it's strictly worse. In
fact, I am struggling to find a scenario where it's better for me as a
user from the code robustness PoV - it's either the same or worse.

is broken is a great illustration of that. PHP 8 makes it obvious that the
cited code is incorrect.

But it's not incorrect. if is_file("abc\0") returns false, it's correct

"abc\0" is not a correct filename, so I expect it to return false. It
does exactly what I need, so it's correct.

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.

This is a kind of philosophical question, like whether or not trying to
open a file that doesn't exist is an "error condition". Throw-happy
languages like Java think it is, other languages like C or PHP treat it
in different way. But I think in any case what we've got in 8 with is_*
is not an improvement.

--
Stas Malyshev
smalyshev@gmail.com

4 years ago by Reindl Harald privat — view source — reply

unread

Am 01.12.20 um 21:06 schrieb Stanislav Malyshev:

Hi!

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

For me as a user that would be a very clear "no". Now if I have any
usage of these functions in my existing code, I have to go and replace
them with safe wrapper to ensure it doesn't bail out in random places

yeah, you should think about external input before do anything with
it, always! if you pass a random path with NULL you did not do anything
to validate the input

millions of security issues in whatever programming language are the
result of "i throw the input somewhere and don't mind"

if you ever reach that exception you have a stacktrace up to the point
where you should have stopped proceed at all

4 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

yeah, you should think about external input before do anything with
it, always! if you pass a random path with NULL you did not do anything
to validate the input

Yes, and? is_file should be safe (as in, not exploding and breaking the
whole app) on any input (leaving typing discussion aside, any string
input). It should return true if the input is a name of an existing
file, false otherwise. It's simple, not?

millions of security issues in whatever programming language are the
result of "i throw the input somewhere and don't mind"

This is a general banality which is not applicable to this specific
functions. Sure, there are security issues that come from input
validation failure. It is not the case here. As somebody who added those
checks in most of the code personally, I can tell you not bailing out
but returning false on is_file would not make security of this function
worse in any way.

I know why it happens - because it has been treated as a type error
(which was a nice hack but in retrospect probably not the most correct
way) and then we decided to make type error throw and the fact that this
is not actually a type came to bite us in the butt. I think the solution
for this is to refactor this code and separate null checks from type
checks. It was a nice hack for the time, but its time has expired.

if you ever reach that exception you have a stacktrace up to the point
where you should have stopped proceed at all

Nope, there's no reason to stop processing when I check whether a random
string signifies valid files. There might be a reason to stop processing
later, after I discovered it is not, or continue processing, depending
on the code intent - e.g. use alternative filename, or the default, or
different code path. Exploding functions take this ability from me as an
author of the code. So I will be forced to take it back by replacing
every use of is_file with safe_is_file which would catch the exception
and return false. Which just adds work for me which I hadn't to do
before. That's not how the language should evolve - it shouldn't make
things that are now easy harder.

Stas Malyshev
smalyshev@gmail.com

4 years ago by Rowan Tommins — view source — reply

unread

is broken is a great illustration of that. PHP 8 makes it obvious
that the
cited code is incorrect.

But it's not incorrect. if is_file("abc\0") returns false, it's
correct - "abc\0" is not a correct filename, so I expect it to return
false. It does exactly what I need, so it's correct.

The posted code is correct under a theoretical version of is_file() that
returns false for that input, but it is incorrect for all versions of
PHP after PHP 5.4, since they return null (as shown by the sample posted
by George: https://3v4l.org/7E2mv) so the ===false condition will not be
met.

I think we can all agree that the previous behaviour was less than ideal

the NULL return value was undocumented and unexpected, and the wording
of the Warning was pretty vague.

If I'd never seen this discussion, I think my assumption would be that
invalid input would simply return false in this particular case - if the
path is logically impossible, then it's not a file. However, I wonder if
there other functions also use this undocumented "file path" input type,
where there isn't such an obvious safe return value, and throwing an
exception is more justifiable.

Regards,

--
Rowan Tommins (né Collins)
[IMSoP]

4 years ago by come.chilliet@fusiondirectory.org — view source — reply

unread

Le Tue, 1 Dec 2020 12:06:22 -0800,
Stanislav Malyshev smalyshev@gmail.com a écrit :

But it's not incorrect. if is_file("abc\0") returns false, it's correct

"abc\0" is not a correct filename, so I expect it to return false. It
does exactly what I need, so it's correct.

Hear hear. I think this is the best summary in this discussion.

is_file and is_dir should just return false on values which are not files and
dirs.

Can we please fix this for future PHP versions?
Does it need an RFC?

4 years ago by Christian Schneider — view source — reply

unread

Am 03.12.2020 um 09:27 schrieb Côme Chilliet come.chilliet@fusiondirectory.org:

Le Tue, 1 Dec 2020 12:06:22 -0800,
Stanislav Malyshev smalyshev@gmail.com a écrit :

But it's not incorrect. if is_file("abc\0") returns false, it's correct

"abc\0" is not a correct filename, so I expect it to return false. It
does exactly what I need, so it's correct.

Hear hear. I think this is the best summary in this discussion.

is_file and is_dir should just return false on values which are not files and
dirs.

Can we please fix this for future PHP versions?
Does it need an RFC?

There is a PR for this which was approved by Nikita. But I'm not sure what the plan is on merging into master or other branches:
https://github.com/php/php-src/pull/6478 https://github.com/php/php-src/pull/6478

Chris

4 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

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

Warning is a debugging functionality. Throwing is breaking the app and
stopping the whole process. There's a fundamental difference between the
two.

Would it have been fine if this would have been a TypeError as it was
originally intended?

It's not a type error. PHP does not support such types. "string that is
a valid filename" is not a type in PHP, thus TypeError would be misleading.

Is a warning fine because null bytes indicate a potential attack as in no
sane
context should null bytes be passed around?

A warning is fine because it does what it's supposed to do - fails the
is_file check (which is literally only there to check if this string
specifies a valid filename) while not breaking the app. Exception breaks
the app.

So what we'll be seeing very soon is people creating userspace safe_is_*
wrappers that would work around this "functionality", working against
the language instead of being helped by it. This is not how it should be.

Stas Malyshev
smalyshev@gmail.com

4 years ago by bjorn.x.larsson@telia.com — view source — reply

unread

Den 2020-12-01 kl. 20:57, skrev Stanislav Malyshev:

Hi!

Is a warning fine because null bytes indicate a potential attack as
in no
sane context should null bytes be passed around?

A warning is fine because it does what it's supposed to do - fails the
is_file check (which is literally only there to check if this string
specifies a valid filename) while not breaking the app. Exception
breaks the app.

So what we'll be seeing very soon is people creating userspace
safe_is_* wrappers that would work around this "functionality",
working against the language instead of being helped by it. This is
not how it should be.

One could add that here the PHP programmer need to do work that basically
replicate how the code worked earlier for little gain. Maybe one also
need to
take into account how likely it is that \0 is part of a filename.

So I wonder how much of a hurdle it is for PHP 8 migration? Especially
of one
has an application that needs to run on both PHP 7.x and PHP 8.

Think it would be good if a solution / conclusion is found for PHP 8.0.1.

r//Björn Larsson

4 years ago by Rowan Tommins — view source — reply

unread

One could add that here the PHP programmer need to do work that
basically
replicate how the code worked earlier for little gain.

Just to reiterate, the previous implementation was also bad - it returned an entirely undocumented and unexpected null value.

In many cases, that will have worked because it was implicitly cast to boolean, but in some cases it will already have led to a TypeError being thrown, e.g. in a wrapper like this:

<?php declare(strict_types=1);
class Dir {
private $path;
// ...
public function hasFile(string $name): bool {
return is_file("$path/$name");
}
}

Regards,

--
Rowan Tommins
[IMSoP]

4 years ago by Christoph M. Becker — view source — reply

unread

One could add that here the PHP programmer need to do work that
basically
replicate how the code worked earlier for little gain.

Just to reiterate, the previous implementation was also bad - it returned an entirely undocumented and unexpected null value.

Just to reiterate, the former behavior was documented[1], and the return
value was undefined. Instead of returning NULL, the erroneous
function call could have returned anything, even TRUE. Relying on
undefined behavior is bad.

That said, I'm not against changing the behavior of is_file() and
is_dir() as requested, and also the behavior of is_readable(),
is_writable() and file_exists(). There are likely more
functions/methods whose behavior should be changed accordingly. It
might be worthwhile to identify these functions.

And we should also explicitly document what these functions returned for
paths containing NUL bytes prior to PHP 8.0.0.

[1] https://www.php.net/manual/en/functions.internal.php

Regards,
Christoph

4 years ago by Rowan Tommins — view source — reply

unread

PHP functions always returned null on bad parameters, so it's not
exactly unexpected.

and

Just to reiterate, the former behavior was documented[1], and the return
value wasundefined. Instead of returning NULL, the erroneous
function call could have returned anything, even TRUE. Relying on
undefined behavior is bad.

The note on https://www.php.net/manual/en/functions.internal.php says:

If the parameters given to a function are not what it expects...

But the prototype on https://www.php.net/manual/en/function.is-file.php is:

is_file ( string $filename ) : bool

And the description says only:

filename: Path to the file.

While it's clear that passing e.g. an array falls into the scope of that
general note, it doesn't say anywhere on that page that a string value
which contains "\0" is "not what it expects", and I don't think I would
ever have guessed that before reading this thread.

So I stand by my assertion that this behaviour was both undocumented and
unexpected.

Regards,

--
Rowan Tommins (né Collins)
[IMSoP]

4 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

While it's clear that passing e.g. an array falls into the scope of that
general note, it doesn't say anywhere on that page that a string value
which contains "\0" is "not what it expects", and I don't think I would
ever have guessed that before reading this thread.

So I stand by my assertion that this behaviour was both undocumented and
unexpected.

You may assert anything, but it's a fact that PHP functions have
returned nulls on bad values since forever. The manual may tell you not
to rely on that, but that's still what they did. It's not like it
suddenly happened out of nowhere. It has been the case for ages.

--
Stas Malyshev
smalyshev@gmail.com

4 years ago by Rowan Tommins — view source — reply

unread

You may assert anything, but it's a fact that PHP functions have
returned nulls on bad values since forever.

I'm not disputing that. I'm disputing that anyone looking at the name or
documentation of this particular function would guess that "foo\0bar" is
a "bad value". Clearly, somebody thought so and accounted for it as such
in the implementation, but it would never have occurred to me if I
hadn't read this thread.

Regards,

--
Rowan Tommins (né Collins)
[IMSoP]

4 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

Just to reiterate, the previous implementation was also bad - it
returned an entirely undocumented and unexpected null value.

PHP functions always returned null on bad parameters, so it's not
exactly unexpected.

--
Stas Malyshev
smalyshev@gmail.com

4 years ago by Paul Crovella — view source — reply

unread

PHP 8 is stricter in checking input data then PHP 7. This is good but
has some side effects for is_file(), is_dir() and similar functions when
invalid paths are passed for checking.

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. :)

Regards,
Christoph

Or is_file could check for it, handle it gracefully, and be a safe
function to call without worrying about this undocumented edge case.

4 years ago by G. P. B. — view source — reply

unread

On Tue, Dec 1, 2020 at 9:25 AM Christoph M. Becker cmbecker69@gmx.de
wrote:

PHP 8 is stricter in checking input data then PHP 7. This is good but
has some side effects for is_file(), is_dir() and similar functions
when
invalid paths are passed for checking.

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. :)

Regards,
Christoph

Or is_file could check for it, handle it gracefully, and be a safe
function to call without worrying about this undocumented edge case.

--

To unsubscribe, visit: https://www.php.net/unsub.php

4 years ago by G. P. B. — view source — reply

unread

On Tue, 1 Dec 2020 at 17:45, Paul Crovella paul.crovella@gmail.com
wrote:

On Tue, Dec 1, 2020 at 9:25 AM Christoph M. Becker cmbecker69@gmx.de
wrote:

PHP 8 is stricter in checking input data then PHP 7. This is good but
has some side effects for is_file(), is_dir() and similar functions
when
invalid paths are passed for checking.

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. :)

Regards,
Christoph

Or is_file could check for it, handle it gracefully, and be a safe
function to call without worrying about this undocumented edge case.

--

To unsubscribe, visit: https://www.php.net/unsub.php

Apologize my email client sent by mistake:
But this has always generated a warning see:
https://3v4l.org/7E2mv

So this is not new behaviour.

George P. Banyard

4 years ago by Paul Crovella — view source — reply

unread

PHP 8 is stricter in checking input data then PHP 7. This is good but
has some side effects for is_file(), is_dir() and similar functions when
invalid paths are passed for checking.

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. :)

Regards,
Christoph

Or is_file could check for it, handle it gracefully, and be a safe
function to call without worrying about this undocumented edge case.

--

To unsubscribe, visit: https://www.php.net/unsub.php

Apologize my email client sent by mistake:
But this has always generated a warning see:
https://3v4l.org/7E2mv

So this is not new behaviour.

George P. Banyard

The result is in fact new. An uncaught ValueError isn't a warning.

4 years ago by Aimeos | Norbert Sendetzky — view source — reply

unread

Am 01.12.20 um 18:47 schrieb G. P. B.:

Or is_file could check for it, handle it gracefully, and be a safe
function to call without worrying about this undocumented edge case.

Apologize my email client sent by mistake:
But this has always generated a warning see:
https://3v4l.org/7E2mv

So this is not new behaviour.

A warning is OK but now an exception is now thrown so it's very
different from versions before PHP 8.

Furthermore, Christoph's code uses str_contains() which is only
available in PHP 8 so it's not a good workaround if PHP 7 and 8 must be
both supported.

Personally, I would agree with Paul because the additional code that's
necessary for the workaround complicates the user code base.

Best,

Norbert

4 years ago by Reindl Harald privat — view source — reply

unread

Am 01.12.20 um 18:56 schrieb Aimeos | Norbert Sendetzky:

Am 01.12.20 um 18:47 schrieb G. P. B.:

Or is_file could check for it, handle it gracefully, and be a safe
function to call without worrying about this undocumented edge case.

Apologize my email client sent by mistake:
But this has always generated a warning see:
https://3v4l.org/7E2mv

So this is not new behaviour.

A warning is OK but now an exception is now thrown so it's very
different from versions before PHP 8

a warning is not OK and never was

we are running error_reporting E_ALL for 17 years now and don't distinct
between notice / warning / error, it has to be fixed - period

4 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

we are running error_reporting E_ALL for 17 years now and don't
distinct between notice / warning / error, it has to be fixed -
period

Surely you do. Your code continues to run after warning/notice but stops
after the error. It's impossible to ignore that. Unless you have an
error handler that does exit() after a notice (which I have hard time
believing, honestly, but who knows), there is a very major distinction.

It's not about what "has to be fixed" - it's not about the contents of
your bug tracking database - it's about the code that run one way and
suddenly now runs (or, rather, fails) in a fundamentally different way.

--
Stas Malyshev
smalyshev@gmail.com

4 years ago by Reindl Harald privat — view source — reply

unread

Am 01.12.20 um 21:09 schrieb Stanislav Malyshev:

we are running error_reporting E_ALL for 17 years now and don't
distinct between notice / warning / error, it has to be fixed -
period

Surely you do. Your code continues to run after warning/notice but stops
after the error. It's impossible to ignore that. Unless you have an
error handler that does exit() after a notice (which I have hard time
believing, honestly, but who knows), there is a very major distinction.

my server would trigger a mail every 15 minutes wioth all warnings and
notices to enforce fixing the issue

It's not about what "has to be fixed" - it's not about the contents of
your bug tracking database - it's about the code that run one way and
suddenly now runs (or, rather, fails) in a fundamentally different way
it should fail and it should have done that for 20 years because it
points out missing input validation which is a much bigger probkem than
a random exception seems to be

but yeah, you are the guy closing security bugs all the time with no
understanding what "fail eraly and fail safe" means in the context of
security

4 years ago by Christian Schneider — view source — reply

unread

Am 01.12.2020 um 21:13 schrieb Reindl Harald (privat) harry@rhsoft.net:

Am 01.12.20 um 21:09 schrieb Stanislav Malyshev:

we are running error_reporting E_ALL for 17 years now and don't
distinct between notice / warning / error, it has to be fixed -
period
Surely you do. Your code continues to run after warning/notice but stops
after the error. It's impossible to ignore that. Unless you have an
error handler that does exit() after a notice (which I have hard time
believing, honestly, but who knows), there is a very major distinction.

my server would trigger a mail every 15 minutes wioth all warnings and notices to enforce fixing the issue

Out of curiosity: What is your fix?

Because we are running into this issue with fuzzers bombarding our website with all types of illegal parameters, string containing 0-bytes amongst them.

Our solution was to basically throw away all user input containing 0-bytes (except $_FILES) which feels awkward but was the only way to avoid these messages (and in some cases exceptions) consistently.

Concerning the original question:
My personal preference in this specific case is Stas’ way: is_file() is a low-level function and should simply return false for anything which is not a valid, existing filename. Having everything involving paths warn/throw an exception when 0-bytes are involved is an overly broad generalization.

I challenge everybody to show me how changing is_file() to simply return false (while keeping more high-level functions like, say, popen throwing an exception) leads to a security hole.

In my preferred world the following code would be both safe and guaranteed to report „File not found“ on any invalid input:
If (is_file($filename))
do_something($filename);
else
message("File not found“);

I’ll try to put a PR together for FileFunction() because I think those functions should all handle 0-bytes like non-existing files.

Chris

4 years ago by Reindl Harald privat — view source — reply

unread

Am 02.12.20 um 09:18 schrieb Christian Schneider:

Am 01.12.2020 um 21:13 schrieb Reindl Harald (privat) harry@rhsoft.net:

Am 01.12.20 um 21:09 schrieb Stanislav Malyshev:

we are running error_reporting E_ALL for 17 years now and don't
distinct between notice / warning / error, it has to be fixed -
period
Surely you do. Your code continues to run after warning/notice but stops
after the error. It's impossible to ignore that. Unless you have an
error handler that does exit() after a notice (which I have hard time
believing, honestly, but who knows), there is a very major distinction.

my server would trigger a mail every 15 minutes wioth all warnings and notices to enforce fixing the issue

Out of curiosity: What is your fix?

https://en.wikipedia.org/wiki/Web_application_firewall and sanitize
userinput while i don't see much legit usecases where userinout from
outside makes it to is_file/is_dir without calling that a security issue
anyways

4 years ago by Larry Garfield — view source — reply

unread

Am 01.12.2020 um 21:13 schrieb Reindl Harald (privat) harry@rhsoft.net:

Am 01.12.20 um 21:09 schrieb Stanislav Malyshev:

we are running error_reporting E_ALL for 17 years now and don't
distinct between notice / warning / error, it has to be fixed -
period
Surely you do. Your code continues to run after warning/notice but stops
after the error. It's impossible to ignore that. Unless you have an
error handler that does exit() after a notice (which I have hard time
believing, honestly, but who knows), there is a very major distinction.

my server would trigger a mail every 15 minutes wioth all warnings and notices to enforce fixing the issue

Out of curiosity: What is your fix?

Because we are running into this issue with fuzzers bombarding our
website with all types of illegal parameters, string containing 0-bytes
amongst them.

Our solution was to basically throw away all user input containing
0-bytes (except $_FILES) which feels awkward but was the only way to
avoid these messages (and in some cases exceptions) consistently.

Concerning the original question:
My personal preference in this specific case is Stas’ way: is_file() is
a low-level function and should simply return false for anything
which is not a valid, existing filename. Having everything involving
paths warn/throw an exception when 0-bytes are involved is an overly
broad generalization.

I challenge everybody to show me how changing is_file() to simply
return false (while keeping more high-level functions like, say, popen
throwing an exception) leads to a security hole.

In my preferred world the following code would be both safe and
guaranteed to report „File not found“ on any invalid input:
If (is_file($filename))
do_something($filename);
else
message("File not found“);

I’ll try to put a PR together for FileFunction() because I think those
functions should all handle 0-bytes like non-existing files.

Chris

I'm inclined to agree here. An is_*() function isn't asking why something isn't a legit file; "missing" and "it's a stupid name anyway and it should feel bad" and "the disk is missing" are all the same thing as far as it's concerned. All it's asking is "if I try to open this file, should I expect it to work?"

Having it throw an exception in some cases, it's understandable how that ends up happening, but from a user/API point of view it's still incorrect.

--Larry Garfield

4 years ago by Aimeos | Norbert Sendetzky — view source — reply

unread

Am 01.12.20 um 18:24 schrieb Christoph M. Becker:

PHP 8 is stricter in checking input data then PHP 7. This is good but
has some side effects for is_file(), is_dir() and similar functions when
invalid paths are passed for checking.

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. :)

If it's the only check that would throw a ValueException, then yes -
even if I think that is_file() should only return true/false to avoid
blown up code for checks that should be done by is_file().

Now have a look at GD imagecreatefromstring() which has almost the same
issue. If you use:

php -r 'var_dump(imagecreatefromstring('some data'));'

you will get in PHP 7:

PHP Warning: imagecreatefromstring(): Empty string or invalid image in
Command line code on line 1
PHP Stack trace:
PHP 1. {main}() Command line code:0
PHP 2. imagecreatefromstring() Command line code:1
Command line code:1:
bool(false)

and in PHP 8:

PHP Fatal error: Uncaught ValueError: imagecreatefromstring(): Argument
#1 ($data) cannot be empty in Command line code:1
Stack trace:
#0 Command line code(1): imagecreatefromstring()
#1 {main}
thrown in Command line code on line 1

How would you check the string upfront to be a valid image to avoid the
ValueException there?

Also, the error in PHP 8 is wrong because the string isn't empty but not
a valid image or not supported by GD.

Norbert

4 years ago by Nikita Popov — view source — reply

unread

On Wed, Dec 2, 2020 at 10:21 AM Aimeos | Norbert Sendetzky <
norbert@aimeos.com> wrote:

Am 01.12.20 um 18:24 schrieb Christoph M. Becker:

PHP 8 is stricter in checking input data then PHP 7. This is good but
has some side effects for is_file(), is_dir() and similar functions when
invalid paths are passed for checking.

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. :)

If it's the only check that would throw a ValueException, then yes -
even if I think that is_file() should only return true/false to avoid
blown up code for checks that should be done by is_file().

Now have a look at GD imagecreatefromstring() which has almost the same
issue. If you use:

php -r 'var_dump(imagecreatefromstring('some data'));'

you will get in PHP 7:

PHP Warning: imagecreatefromstring(): Empty string or invalid image in
Command line code on line 1
PHP Stack trace:
PHP 1. {main}() Command line code:0
PHP 2. imagecreatefromstring() Command line code:1
Command line code:1:
bool(false)

and in PHP 8:

PHP Fatal error: Uncaught ValueError: imagecreatefromstring(): Argument
#1 ($data) cannot be empty in Command line code:1
Stack trace:
#0 Command line code(1): imagecreatefromstring()
#1 {main}
thrown in Command line code on line 1

How would you check the string upfront to be a valid image to avoid the
ValueException there?

Also, the error in PHP 8 is wrong because the string isn't empty but not
a valid image or not supported by GD.

This was an implementation error, fixed in
https://github.com/php/php-src/commit/a89aaf6c386679492e814cfbb5790142e29692fe.
Thanks for the report!

Nikita

4 years ago by Hans Henrik Bergan — view source — reply

unread

so.. will the imagecreatefromstring() thing be fixed in 8.0.1 or 8.1.0 ?

On Wed, Dec 2, 2020 at 10:21 AM Aimeos | Norbert Sendetzky <
norbert@aimeos.com> wrote:

Am 01.12.20 um 18:24 schrieb Christoph M. Becker:

PHP 8 is stricter in checking input data then PHP 7. This is good but
has some side effects for is_file(), is_dir() and similar functions when
invalid paths are passed for checking.

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. :)

If it's the only check that would throw a ValueException, then yes -
even if I think that is_file() should only return true/false to avoid
blown up code for checks that should be done by is_file().

Now have a look at GD imagecreatefromstring() which has almost the same
issue. If you use:

php -r 'var_dump(imagecreatefromstring('some data'));'

you will get in PHP 7:

PHP Warning: imagecreatefromstring(): Empty string or invalid image in
Command line code on line 1
PHP Stack trace:
PHP 1. {main}() Command line code:0
PHP 2. imagecreatefromstring() Command line code:1
Command line code:1:
bool(false)

and in PHP 8:

PHP Fatal error: Uncaught ValueError: imagecreatefromstring(): Argument
#1 ($data) cannot be empty in Command line code:1
Stack trace:
#0 Command line code(1): imagecreatefromstring()
#1 {main}
thrown in Command line code on line 1

How would you check the string upfront to be a valid image to avoid the
ValueException there?

Also, the error in PHP 8 is wrong because the string isn't empty but not
a valid image or not supported by GD.

This was an implementation error, fixed in
https://github.com/php/php-src/commit/a89aaf6c386679492e814cfbb5790142e29692fe.
Thanks for the report!

Nikita

4 years ago by Christoph M. Becker — view source — reply

unread

so.. will the imagecreatefromstring() thing be fixed in 8.0.1 or 8.1.0 ?

This will be fixed in 8.0.1.

Christoph

4 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

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

I think this is a mistake. Exceptions should not be thrown on such
values, it only breeds boilerplate code (now you'd have to wrap each
call to is_* into try/catch or add another pre-call validator just to
ensure it doesn't throw). PHP type system is not robust enough to
support fine-grained types like "string that has certain format", and
pretending we do have this as a type and throwing typing error on it
only makes the consumer work harder for no additional gain. In almost
every situation I can imagine, string that is not the name of the file
is equivalent to string that is not the name of the file and also has \0
in it. However, this setup forces me to treat them differently and add
additional validation step to it. While to some functions it may be a
useful functionality - specifically to constructor functions expected to
return an object, so they can't really tell "you gave me invalid data"
other than throwing - for something like is_file it will only be annoying.

Stas Malyshev
smalyshev@gmail.com

4 years ago by Mike Schinkel — view source — reply

unread

Hi internals

PHP 8 is stricter in checking input data then PHP 7. This is good but
has some side effects for is_file(), is_dir() and similar functions when
invalid paths are passed for checking.

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.

My suggestion for simpler client side code would be to return FALSE in
this situation for PHP 8 too instead of throwing the ValueException.
Otherwise, it's not possible to use is_file() and related functions
without adding a try/catch block around in any web application.

This is a general case of throwing exceptions in PHP whenever an unwanted condition occurs, especially where code previously did not throw an exception.

The two schools of thought for error handling could be classified as:

1.) Throw for every unwanted condition and then handle later in a catch block, and
2.) Handle every unwanted condition at the point it is discovered.

Some believe only one strategy is valid but others disagree, so I argue there is no settled best practice.

When a PHP function throws an error if forces developers to wrap in try/catch, and the more functions throwing errors the more wrapping is needed. Especially when the function could just have returned a false.

It is extremely easy to throw an Exception for is_file() when try/catch is needed, which is rare that an exception is needed for \0.

When following best practices, having is_file() throw adds complexity to every is_file() call, or developers to create their own safe_is_file() as mentioned in the thread.

In summary:

Please consider making is_file() return false for an embedded \0 and no longer throw an exception or generate a warning.
Beyond is_file(), please consider allowing PHP to support both types of error handling strategies without forcing complexity just to use the 2nd strategy.

-Mike

4 years ago by Christian Schneider — view source — reply

unread

Am 08.12.2020 um 17:16 schrieb Mike Schinkel mike@newclarity.net:

Please consider making is_file() return false for an embedded \0 and no longer throw an exception or generate a warning.

The PR implementing this has already been merged for inclusion in PHP 8.0.1: https://github.com/php/php-src/pull/6478

Beyond is_file(), please consider allowing PHP to support both types of error handling strategies without forcing complexity just to use the 2nd strategy.

This is a contentious topic and will have to be handled on a case by case basis I guess.
E.g. for security-related stuff a fail-early-strategy throwing an exception might be preferable, for basic lower-level functions like is_file() returning false was chosen to be preferable.
There is no silver bullet for all cases and having a global switch would only make things worse.

Chris

4 years ago by Mike Schinkel — view source — reply

unread

Am 08.12.2020 um 17:16 schrieb Mike Schinkel <mike@newclarity.net mailto:mike@newclarity.net>:

Please consider making is_file() return false for an embedded \0 and no longer throw an exception or generate a warning.

The PR implementing this has already been merged for inclusion in PHP 8.0.1: https://github.com/php/php-src/pull/6478 https://github.com/php/php-src/pull/6478
Thanks.

Beyond is_file(), please consider allowing PHP to support both types of error handling strategies without forcing complexity just to use the 2nd strategy.

This is a contentious topic

Exactly.

and will have to be handled on a case by case basis I guess.

Which is really all I was asking for; that it be considered on a case-by-case basis instead of just assuming that exceptions are always preferable.

E.g. for security-related stuff a fail-early-strategy throwing an exception might be preferable, for basic lower-level functions like is_file() returning false was chosen to be preferable.
There is no silver bullet for all cases and having a global switch would only make things worse.

-Mike

PHP 8 is_file/is_dir input handling

So what we'll be seeing very soon is people creating userspace safe_is_* wrappers that would work around this "functionality", working against the language instead of being helped by it. This is not how it should be.

So what we'll be seeing very soon is people creating userspace safe_is_*
wrappers that would work around this "functionality", working against
the language instead of being helped by it. This is not how it should be.