# PHP 8 is_file/is_dir input handling

- Thread: https://externals.io/message/112333
- Original archive: https://news-web.php.net/php.internals/112333
- Started by Aimeos | Norbert Sendetzky on 2020-12-01T17:18:17+00:00
- Messages: 44
- Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL)
- Next page: https://externals.io/message/112333.md?page=2
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #112333 — Aimeos | Norbert Sendetzky — 2020-12-01T17:18:17+00:00

URL: https://externals.io/message/112333#112333

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.

Best,


Norbert

---

## #112334 — Christoph M. Becker — 2020-12-01T17:24:35+00:00

URL: https://externals.io/message/112333#112334 · In reply to: #112333

> 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

---

## #112335 — Aimeos | Norbert Sendetzky — 2020-12-01T17:35:01+00:00

URL: https://externals.io/message/112333#112335 · In reply to: #112334

Am 01.12.20 um 18:24 schrieb Christoph M. Becker:
> > […]
> 
> 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

---

## #112336 — Christoph M. Becker — 2020-12-01T17:43:24+00:00

URL: https://externals.io/message/112333#112336 · In reply to: #112335

> Am 01.12.20 um 18:24 schrieb Christoph M. Becker:
> > […]
>
> 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

---

## #112337 — Paul Crovella — 2020-12-01T17:45:16+00:00

URL: https://externals.io/message/112333#112337 · In reply to: #112334

>
> 
>
> > […]
>
> 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.

---

## #112338 — G. P. B. — 2020-12-01T17:46:29+00:00

URL: https://externals.io/message/112333#112338 · In reply to: #112337

> On Tue, Dec 1, 2020 at 9:25 AM Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
> > […]
> when
> > […]
>
> 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

---

## #112339 — G. P. B. — 2020-12-01T17:47:47+00:00

URL: https://externals.io/message/112333#112339 · In reply to: #112338

> On Tue, 1 Dec 2020 at 17:45, Paul Crovella <paul.crovella@gmail.com>
> wrote:
>
> > […]
>
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

---

## #112340 — Paul Crovella — 2020-12-01T17:54:11+00:00

URL: https://externals.io/message/112333#112340 · In reply to: #112339

>
>
> 
> > […]
>
> 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.

---

## #112341 — Aimeos | Norbert Sendetzky — 2020-12-01T17:56:42+00:00

URL: https://externals.io/message/112333#112341 · In reply to: #112339

Am 01.12.20 um 18:47 schrieb G. P. B.:
> > […]
> 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

---

## #112343 — Paul Crovella — 2020-12-01T18:07:31+00:00

URL: https://externals.io/message/112333#112343 · In reply to: #112336

>
> 
>
> > […]
>
> 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.

---

## #112344 — Reindl Harald privat — 2020-12-01T18:21:34+00:00

URL: https://externals.io/message/112333#112344 · In reply to: #112341

Am 01.12.20 um 18:56 schrieb Aimeos | Norbert Sendetzky:
> Am 01.12.20 um 18:47 schrieb G. P. B.:
> > […]
> 
> 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

---

## #112345 — G. P. B. — 2020-12-01T18:23:34+00:00

URL: https://externals.io/message/112333#112345 · In reply to: #112343

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

---

## #112346 — Paul Crovella — 2020-12-01T18:35:22+00:00

URL: https://externals.io/message/112333#112346 · In reply to: #112345

>
> 
> > […]
>
>
> 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.

---

## #112347 — Aimeos | Norbert Sendetzky — 2020-12-01T18:38:51+00:00

URL: https://externals.io/message/112333#112347 · In reply to: #112345

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

---

## #112348 — G. P. B. — 2020-12-01T18:38:54+00:00

URL: https://externals.io/message/112333#112348 · In reply to: #112346

> 
> > […]
> wrote:
> > […]
> wrote:
> > […]
> that
> > […]
> valid
> > […]
> check
> > […]
> sure
> > […]
> differenciate
> > […]
> which
> > […]
> good
> > […]
>
> That's correct, it shouldn't have.
>
> > […]
>
> 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


> > […]
> no sane
> > […]
>
> 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.
>
> > […]
> issue only
> > […]
>
> 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

---

## #112350 — Christoph M. Becker — 2020-12-01T18:58:03+00:00

URL: https://externals.io/message/112333#112350 · In reply to: #112347

> Am 01.12.20 um 19:23 schrieb G. P. B.:
>
> > […]
>
> 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

---

## #112353 — Stanislav Malyshev — 2020-12-01T19:50:13+00:00

URL: https://externals.io/message/112333#112353 · In reply to: #112333

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

---

## #112354 — Nikita Popov — 2020-12-01T19:53:59+00:00

URL: https://externals.io/message/112333#112354 · In reply to: #112350

On Tue, Dec 1, 2020 at 7:58 PM Christoph M. Becker <cmbecker69@gmx.de>
wrote:

> 
>
> > […]
> no
> > […]
>
> 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

---

## #112355 — Stanislav Malyshev — 2020-12-01T19:57:31+00:00

URL: https://externals.io/message/112333#112355 · In reply to: #112345

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

---

## #112356 — Stanislav Malyshev — 2020-12-01T20:06:22+00:00

URL: https://externals.io/message/112333#112356 · In reply to: #112354

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

---

## #112357 — Stanislav Malyshev — 2020-12-01T20:09:51+00:00

URL: https://externals.io/message/112333#112357 · In reply to: #112344

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

---

## #112358 — Reindl Harald privat — 2020-12-01T20:10:28+00:00

URL: https://externals.io/message/112333#112358 · In reply to: #112356

Am 01.12.20 um 21:06 schrieb Stanislav Malyshev:
> Hi!
> 
> > […]
> 
> 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

---

## #112359 — Reindl Harald privat — 2020-12-01T20:13:27+00:00

URL: https://externals.io/message/112333#112359 · In reply to: #112357

Am 01.12.20 um 21:09 schrieb Stanislav Malyshev:
> > […]
> 
> 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

---

## #112360 — Stanislav Malyshev — 2020-12-01T20:21:33+00:00

URL: https://externals.io/message/112333#112360 · In reply to: #112358

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

---

## #112361 — Rowan Tommins — 2020-12-01T20:35:14+00:00

URL: https://externals.io/message/112333#112361 · In reply to: #112356

> > […]
>
> 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]

---

Next page: https://externals.io/message/112333.md?page=2