Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » PHP 8 is_file/is_dir input handling »

PHP 8 is_file/is_dir input handling

php.internals

Aimeos | Norbert Sendetzky

5 years ago
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

Christoph Becker

5 years ago
On 01.12.2020 at 18:18, Aimeos | Norbert Sendetzky 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

Aimeos | Norbert Sendetzky

5 years ago
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

Christoph Becker

5 years ago
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.
> 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

Paul

5 years ago
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.

Girgias

5 years ago
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. 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

Paul

5 years ago
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.
> 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.

Girgias

5 years ago
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

Aimeos | Norbert Sendetzky

5 years ago
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

Christoph Becker

5 years ago
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

Nikita Popov

5 years ago
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

Stas Malyshev

5 years ago
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

Stas Malyshev

5 years ago
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

Björn Larsson

5 years ago
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

Rowan Collins

5 years ago
On 2 December 2020 22:38:13 GMT+00:00, "Björn Larsson" <bjorn.x.larsson@telia.com> wrote:
>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]

Christoph Becker

5 years ago
On 03.12.2020 at 00:06, Rowan Tommins wrote:
> On 2 December 2020 22:38:13 GMT+00:00, "Björn Larsson" <bjorn.x.larsson@telia.com> wrote: > >> 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

Stas Malyshev

5 years ago
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

Paul

5 years ago
On Tue, Dec 1, 2020 at 9:25 AM Christoph M. Becker <cmbecker69@gmx.de> wrote:
> > On 01.12.2020 at 18:18, Aimeos | Norbert Sendetzky 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.

Girgias

5 years ago
On Tue, 1 Dec 2020 at 17:45, Paul Crovella <paul.crovella@gmail.com> wrote:

Girgias

5 years ago
On Tue, 1 Dec 2020 at 17:46, G. P. B. <george.banyard@gmail.com> wrote:
> 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: >> > >> > On 01.12.2020 at 18:18, Aimeos | Norbert Sendetzky 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. >> >> -- >> PHP Internals - PHP Runtime Development Mailing List >> 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

Paul

5 years ago
On Tue, Dec 1, 2020 at 9:48 AM G. P. B. <george.banyard@gmail.com> wrote:
> > > On Tue, 1 Dec 2020 at 17:46, G. P. B. <george.banyard@gmail.com> wrote: >> >> 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: >>> > >>> > On 01.12.2020 at 18:18, Aimeos | Norbert Sendetzky 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. >>> >>> -- >>> PHP Internals - PHP Runtime Development Mailing List >>> 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.

Aimeos | Norbert Sendetzky

5 years ago
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

Benjamin Morel

5 years ago
> 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. >
Here: https://3v4l.org/LHcmN (https://link.getmailspring.com/link/CF1DFC14-E01B-46B3-9719-6DCA72108EE1@getmailspring.com/0?redirect=https%3A%2F%2F3v4l.org%2FLHcmN&recipient=aW50ZXJuYWxzQGxpc3RzLnBocC5uZXQ%3D) On Dec 1 2020, at 6:56 pm, Aimeos | Norbert Sendetzky <norbert@aimeos.com> wrote:

Reindl Harald (privat)

5 years ago
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

Stas Malyshev

5 years ago
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

Reindl Harald (privat)

5 years ago
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

Christian Schneider

5 years ago
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

Aimeos | Norbert Sendetzky

5 years ago
Am 01.12.20 um 18:24 schrieb Christoph M. Becker:
> On 01.12.2020 at 18:18, Aimeos | Norbert Sendetzky 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. :)
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

Nikita Popov

5 years ago
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: > > On 01.12.2020 at 18:18, Aimeos | Norbert Sendetzky 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. :) > > 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

Hans Henrik Bergan

5 years ago
so.. will the imagecreatefromstring() thing be fixed in 8.0.1 or 8.1.0 ? On Wed, 2 Dec 2020 at 10:57, Nikita Popov <nikita.ppv@gmail.com> wrote:

Christoph Becker

5 years ago
On 02.12.2020 at 13:21, Hans Henrik Bergan wrote:
> so.. will the imagecreatefromstring() thing be fixed in 8.0.1 or 8.1.0 ?
This will be fixed in 8.0.1. Christoph

Stas Malyshev

5 years ago
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

Mike Schinkel

5 years ago
> On Dec 1, 2020, at 12:18 PM, Aimeos | Norbert Sendetzky <norbert@aimeos.com> wrote: > > 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: 1. Please consider making is_file() return false for an embedded \0 and no longer throw an exception or generate a warning. 2. 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

Christian Schneider

5 years ago
Am 08.12.2020 um 17:16 schrieb Mike Schinkel <mike@newclarity.net>:
> 1. 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
> 2. 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

Mike Schinkel

5 years ago
> On Dec 8, 2020, at 11:27 AM, Christian Schneider <cschneid@cschneid.com> wrote: > > Am 08.12.2020 um 17:16 schrieb Mike Schinkel <mike@newclarity.net <mailto:mike@newclarity.net>>: >> 1. 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.
> >> 2. 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