Adding path_len to all stream functions in trunk

15 years ago by Rasmus Lerdorf — view source — reply

unread

I think we need to pass along the string length to all the stream
functions to maintain binary string safety through this code. This
would fix annoying problems like http://bugs.php.net/39863 and a bunch
of similar issues. Obviously not something we can do in 5.3 without
breaking binary compatibility though. Although we might be able to do
something if we assume only chars valid in the current charset is valid
in file paths.

Anybody have any other thoughts on this one?

-Rasmus

15 years ago by Andi Gutmans — view source — reply

unread

Hi Rasmus,

Hope I understood the problem correctly. If not, this answer won't make sense :)
I do not see a major problem in passing path_len but wonder how much it'd actually solve as we end up calling OS APIs that do not accept path_len, no? I assume we don't want to start searching all these strings for invalid chars before we pass them to the OS.

Andi

-----Original Message-----
From: Rasmus Lerdorf [mailto:rasmus@lerdorf.com]
Sent: Sunday, November 14, 2010 11:54 AM
To: internals
Subject: [PHP-DEV] Adding path_len to all stream functions in trunk

I think we need to pass along the string length to all the stream functions to
maintain binary string safety through this code. This would fix annoying
problems like http://bugs.php.net/39863 and a bunch of similar issues.
Obviously not something we can do in 5.3 without breaking binary compatibility
though. Although we might be able to do something if we assume only chars
valid in the current charset is valid in file paths.

Anybody have any other thoughts on this one?

-Rasmus

--
To unsubscribe, visit:
http://www.php.net/unsub.php

15 years ago by Rasmus Lerdorf — view source — reply

unread

Right, but the root of the problem is where to do the invalidation check
of bogus file path strings. Like checking for bogus null bytes in them.
Right now:

file_exists($file . '.txt');
and
file_exists($file);

will check the same file if $file has a \0 stuck onto the end. This can
lead to security problems.

So, either we need to put some sort of check ahead of any and all calls
that pass paths to the filesystem functions, or we need to change those
filesystem functions to do the check natively. For the second option to
work, we obviously need to pass in more than a naked char *.

My worry with the first option is that we then push this out to every
extension and it gets pretty messy. However, doing it this way would
let us do it without breaking binary compatibility in 5.3.

-Rasmus

Hi Rasmus,

Hope I understood the problem correctly. If not, this answer won't make sense :)
I do not see a major problem in passing path_len but wonder how much it'd actually solve as we end up calling OS APIs that do not accept path_len, no? I assume we don't want to start searching all these strings for invalid chars before we pass them to the OS.

Andi

-----Original Message-----
From: Rasmus Lerdorf [mailto:rasmus@lerdorf.com]
Sent: Sunday, November 14, 2010 11:54 AM
To: internals
Subject: [PHP-DEV] Adding path_len to all stream functions in trunk

I think we need to pass along the string length to all the stream functions to
maintain binary string safety through this code. This would fix annoying
problems like http://bugs.php.net/39863 and a bunch of similar issues.
Obviously not something we can do in 5.3 without breaking binary compatibility
though. Although we might be able to do something if we assume only chars
valid in the current charset is valid in file paths.

Anybody have any other thoughts on this one?

-Rasmus

--
To unsubscribe, visit:
http://www.php.net/unsub.php

15 years ago by Rasmus Lerdorf — view source — reply

unread

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

There are quite a few places where we can't solve it centrally, so
perhaps we need to take the same approach in trunk.

This should take care of every issue mentioned here:

http://www.madirish.net/?article=436

along with a number of bug reports.

I think the only outstanding issue with the patch is whether to show an
error message when we hit a null in a filesystem path string. With the
Zend part of the patch, right now the error is slightly misleading for
code like this:

$file = "foo.php\0";
include $file . ".png";

This will output:

PHP Warning: include(): Failed opening 'foo.php' for inclusion
(include_path='.:') in foo on line 3

Without this patch, this code will of course simply include the foo.php
file and ignore the .png extension.

-Rasmus

Hi Rasmus,

Hope I understood the problem correctly. If not, this answer won't make sense :)
I do not see a major problem in passing path_len but wonder how much it'd actually solve as we end up calling OS APIs that do not accept path_len, no? I assume we don't want to start searching all these strings for invalid chars before we pass them to the OS.

Andi

-----Original Message-----
From: Rasmus Lerdorf [mailto:rasmus@lerdorf.com]
Sent: Sunday, November 14, 2010 11:54 AM
To: internals
Subject: [PHP-DEV] Adding path_len to all stream functions in trunk

I think we need to pass along the string length to all the stream functions to
maintain binary string safety through this code. This would fix annoying
problems like http://bugs.php.net/39863 and a bunch of similar issues.
Obviously not something we can do in 5.3 without breaking binary compatibility
though. Although we might be able to do something if we assume only chars
valid in the current charset is valid in file paths.

Anybody have any other thoughts on this one?

-Rasmus

--
To unsubscribe, visit:
http://www.php.net/unsub.php

15 years ago by Pierre Joye — view source — reply

unread

hi Rasmus,

Thanks for the patch! It is great for 5.3 (this problem has been an
issue for too long already).

However I would like to change the APIs in trunk accordingly to avoid
to have checks in every single place where path are used, and indeed
to avoid to have bugs in new codes. In short, it should be done inside
the respective functions.

NB: by APIs, I mean stream, Zend, TSRM and ext/standard (for the APIs
usable by PHP's extensions).

Cheers,

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

There are quite a few places where we can't solve it centrally, so
perhaps we need to take the same approach in trunk.

This should take care of every issue mentioned here:

http://www.madirish.net/?article=436

along with a number of bug reports.

I think the only outstanding issue with the patch is whether to show an
error message when we hit a null in a filesystem path string. With the
Zend part of the patch, right now the error is slightly misleading for
code like this:

$file = "foo.php\0";
include $file . ".png";

This will output:

PHP Warning: include(): Failed opening 'foo.php' for inclusion
(include_path='.:') in foo on line 3

Without this patch, this code will of course simply include the foo.php
file and ignore the .png extension.

-Rasmus

Hi Rasmus,

Hope I understood the problem correctly. If not, this answer won't make sense :)
I do not see a major problem in passing path_len but wonder how much it'd actually solve as we end up calling OS APIs that do not accept path_len, no? I assume we don't want to start searching all these strings for invalid chars before we pass them to the OS.

Andi

-----Original Message-----
From: Rasmus Lerdorf [mailto:rasmus@lerdorf.com]
Sent: Sunday, November 14, 2010 11:54 AM
To: internals
Subject: [PHP-DEV] Adding path_len to all stream functions in trunk

I think we need to pass along the string length to all the stream functions to
maintain binary string safety through this code. This would fix annoying
problems like http://bugs.php.net/39863 and a bunch of similar issues.
Obviously not something we can do in 5.3 without breaking binary compatibility
though. Although we might be able to do something if we assume only chars
valid in the current charset is valid in file paths.

Anybody have any other thoughts on this one?

-Rasmus

--
To unsubscribe, visit:
http://www.php.net/unsub.php

--

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

15 years ago by Stas Malyshev — view source — reply

unread

Hi!

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

Looking at this patch, I wonder if it wouldn't be cleaner to add new
type (string without nulls) in parameter parsing and have it handled by
parameter parsing in one place instead of doing it in dozens of
different places? There might be some other places not covered but it
would cover most functions - and it's easier to catch/fix those in
extensions.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

15 years ago by Rasmus Lerdorf — view source — reply

unread

Hi!

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

Looking at this patch, I wonder if it wouldn't be cleaner to add new
type (string without nulls) in parameter parsing and have it handled by
parameter parsing in one place instead of doing it in dozens of
different places? There might be some other places not covered but it
would cover most functions - and it's easier to catch/fix those in
extensions.

Yeah, I thought about that too. Still not something we can do without
breaking binary compatibility in the 5.3 branch though and I really
would like to at least get all the core functions to guard themselves
against null-poisoning there.

-Rasmus

15 years ago by Stas Malyshev — view source — reply

unread

Hi!

Yeah, I thought about that too. Still not something we can do without
breaking binary compatibility in the 5.3 branch though and I really
would like to at least get all the core functions to guard themselves
against null-poisoning there.

How adding a new option char to zend_parse_parameters breaks BC?

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

15 years ago by Pierre Joye — view source — reply

unread

Hi!

Yeah, I thought about that too. Still not something we can do without
breaking binary compatibility in the 5.3 branch though and I really
would like to at least get all the core functions to guard themselves
against null-poisoning there.

How adding a new option char to zend_parse_parameters breaks BC?

That's something I like to have as well. For two main reasons:

avoid painful and repetitive path cleaning (VCWD&co)
may save some memory ops (alloc, free, dup) while a file will not be
processed many times by each part of the engine
- exts do a check, then stream and TSRM at the end. That's expensive
  and should be removed

A possible solution would be to return a struct with a smart str ptr
for the given path, the resolved path and some custom OS specific data
(like stats info, permissions, etc.). Doing so each information about
a given path will be requested/generated only once.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

15 years ago by Andi Gutmans — view source — reply

unread

-----Original Message-----
From: Stas Malyshev [mailto:smalyshev@sugarcrm.com]
Sent: Monday, November 15, 2010 8:21 PM
To: Rasmus Lerdorf
Cc: internals
Subject: Re: [PHP-DEV] Adding path_len to all stream functions in trunk

Hi!

Ok, I went through all the 5.3 code. This should fix the null
poisoning problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

Looking at this patch, I wonder if it wouldn't be cleaner to add new type (string
without nulls) in parameter parsing and have it handled by parameter parsing
in one place instead of doing it in dozens of different places? There might be
some other places not covered but it would cover most functions - and it's
easier to catch/fix those in extensions.

That's a nice idea and it also wouldn't break binary compatibility.

Andi

15 years ago by Rasmus Lerdorf — view source — reply

unread

-----Original Message-----
From: Stas Malyshev [mailto:smalyshev@sugarcrm.com]
Sent: Monday, November 15, 2010 8:21 PM
To: Rasmus Lerdorf
Cc: internals
Subject: Re: [PHP-DEV] Adding path_len to all stream functions in trunk

Hi!

Ok, I went through all the 5.3 code. This should fix the null
poisoning problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

Looking at this patch, I wonder if it wouldn't be cleaner to add new type (string
without nulls) in parameter parsing and have it handled by parameter parsing
in one place instead of doing it in dozens of different places? There might be
some other places not covered but it would cover most functions - and it's
easier to catch/fix those in extensions.

That's a nice idea and it also wouldn't break binary compatibility.

Well, it changes the signature of that function, so while we don't break
backward binary compatibility, we break forward compatibility within the
5.3 branch. As in, if I change my extension to use this new NoNull
string flag, it will no longer work on <5.3.3 whereas if I do the
if(strlen(filename) != filename_len) check, this will still work in all
5.3 releases.

-R

15 years ago by Andi Gutmans — view source — reply

unread

-----Original Message-----
From: Rasmus Lerdorf [mailto:rasmus@lerdorf.com]
Sent: Monday, November 15, 2010 9:25 PM
To: Andi Gutmans
Cc: Stas Malyshev; internals
Subject: Re: [PHP-DEV] Adding path_len to all stream functions in trunk

Well, it changes the signature of that function, so while we don't break
backward binary compatibility, we break forward compatibility within the
5.3 branch. As in, if I change my extension to use this new NoNull string flag, it
will no longer work on <5.3.3 whereas if I do the
if(strlen(filename) != filename_len) check, this will still work in all
5.3 releases.

Yes that's correct although I am not sure of chances people would bump into this issue.
In any case, I think it's also fine to fix it the way you suggest in 5.3.x and then do it via API in the next minor/major version.

Andi

15 years ago by Stas Malyshev — view source — reply

unread

Hi!

Well, it changes the signature of that function, so while we don't break
backward binary compatibility, we break forward compatibility within the
5.3 branch. As in, if I change my extension to use this new NoNull
string flag, it will no longer work on<5.3.3 whereas if I do the
if(strlen(filename) != filename_len) check, this will still work in all
5.3 releases.

So if you have such extension, and you need to have it compatible with
previous versions (e.g. PECL one), use the check. That doesn't prevent
us from having the flag in the core code and thus keeping it cleaner.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

15 years ago by Rasmus Lerdorf — view source — reply

unread

Hi!

Well, it changes the signature of that function, so while we don't break
backward binary compatibility, we break forward compatibility within the
5.3 branch. As in, if I change my extension to use this new NoNull
string flag, it will no longer work on<5.3.3 whereas if I do the
if(strlen(filename) != filename_len) check, this will still work in all
5.3 releases.

So if you have such extension, and you need to have it compatible with
previous versions (e.g. PECL one), use the check. That doesn't prevent
us from having the flag in the core code and thus keeping it cleaner.

It still worries me a bit. Distros love to separate core extensions
into separate packages and if you update one of those without updating
the core package, it will break. Hopefully they have hard dependencies
so you can't install php-curl-5.3.4 on php-5.3.3, for example.

-Rasmus

15 years ago by Dmitry Stogov — view source — reply

unread

hi,

don't we have ext/filter that should check all the dangerous input
strings? It would be useless to perform additional checks for constant
stings known at compile time (e.g. on include "foo.php")

Thanks. Dmitry.

Rasmus Lerdorf wrote:

Hi!

Well, it changes the signature of that function, so while we don't break
backward binary compatibility, we break forward compatibility within the
5.3 branch. As in, if I change my extension to use this new NoNull
string flag, it will no longer work on<5.3.3 whereas if I do the
if(strlen(filename) != filename_len) check, this will still work in all
5.3 releases.
So if you have such extension, and you need to have it compatible with
previous versions (e.g. PECL one), use the check. That doesn't prevent
us from having the flag in the core code and thus keeping it cleaner.

It still worries me a bit. Distros love to separate core extensions
into separate packages and if you update one of those without updating
the core package, it will break. Hopefully they have hard dependencies
so you can't install php-curl-5.3.4 on php-5.3.3, for example.

-Rasmus

15 years ago by Rasmus Lerdorf — view source — reply

unread

Yes, you could say that users should just know to filter, and if we were
able to pass the binary string through and had binary-safe filesystem
calls, I could agree that we wouldn't need to do anything, but the
current default of truncating at the null byte is dangerous.

Compare these:

% echo '/etc/passwd\0.txt'
/etc/passwd.txt
% cat '/etc/passwd\0.txt'
cat: /etc/passwd\0.txt: No such file or directory

The same thing from PHP:

echo "/etc/passwd\0.txt";
readfile("/etc/passwd\0.txt");

In both cases the echo prints out a binary-safe string with a null byte
before the ".txt" part. However, unlike UNIX cat, PHP will show
/etc/passwd there. The cat command will interpret its argument
differently from the echo command because cat knows its argument is a
filesystem path and a literal null makes no sense there.

So, the argument is that PHP should be as smart as cat here.

-Rasmus

hi,

don't we have ext/filter that should check all the dangerous input
strings? It would be useless to perform additional checks for constant
stings known at compile time (e.g. on include "foo.php")

Thanks. Dmitry.

Rasmus Lerdorf wrote:

Hi!

Well, it changes the signature of that function, so while we don't
break
backward binary compatibility, we break forward compatibility within
the
5.3 branch. As in, if I change my extension to use this new NoNull
string flag, it will no longer work on<5.3.3 whereas if I do the
if(strlen(filename) != filename_len) check, this will still work in all
5.3 releases.
So if you have such extension, and you need to have it compatible with
previous versions (e.g. PECL one), use the check. That doesn't prevent
us from having the flag in the core code and thus keeping it cleaner.

It still worries me a bit. Distros love to separate core extensions
into separate packages and if you update one of those without updating
the core package, it will break. Hopefully they have hard dependencies
so you can't install php-curl-5.3.4 on php-5.3.3, for example.

-Rasmus

15 years ago by Pierre Joye — view source — reply

unread

hi,

Hi!

Well, it changes the signature of that function, so while we don't break
backward binary compatibility, we break forward compatibility within the
5.3 branch. As in, if I change my extension to use this new NoNull
string flag, it will no longer work on<5.3.3 whereas if I do the
if(strlen(filename) != filename_len) check, this will still work in all
5.3 releases.

So if you have such extension, and you need to have it compatible with
previous versions (e.g. PECL one), use the check. That doesn't prevent
us from having the flag in the core code and thus keeping it cleaner.

It still worries me a bit. Distros love to separate core extensions
into separate packages and if you update one of those without updating
the core package, it will break. Hopefully they have hard dependencies
so you can't install php-curl-5.3.4 on php-5.3.3, for example.

Same here, it is somehow a API incompatibility. We did similar changes
in the past and forced us to have two versions of extensions (afair it
was <5.2.5 and >=5.2.6). That was really painful and took years to get
rid of 5.2.5 support.

I like the idea of having a (maybe not so perfect) solution for 5.3.x
using Rasmus patch and do it right in trunk.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

15 years ago by Julien Pauli — view source — reply

unread

That's a quiet old bug, I'm happy to listen it's now worked on and has a patch.

J.Pauli

hi,

Hi!

Well, it changes the signature of that function, so while we don't break
backward binary compatibility, we break forward compatibility within the
5.3 branch. As in, if I change my extension to use this new NoNull
string flag, it will no longer work on<5.3.3 whereas if I do the
if(strlen(filename) != filename_len) check, this will still work in all
5.3 releases.

So if you have such extension, and you need to have it compatible with
previous versions (e.g. PECL one), use the check. That doesn't prevent
us from having the flag in the core code and thus keeping it cleaner.

It still worries me a bit. Distros love to separate core extensions
into separate packages and if you update one of those without updating
the core package, it will break. Hopefully they have hard dependencies
so you can't install php-curl-5.3.4 on php-5.3.3, for example.

Same here, it is somehow a API incompatibility. We did similar changes
in the past and forced us to have two versions of extensions (afair it
was <5.2.5 and >=5.2.6). That was really painful and took years to get
rid of 5.2.5 support.

I like the idea of having a (maybe not so perfect) solution for 5.3.x
using Rasmus patch and do it right in trunk.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

15 years ago by johannes@schlueters.de — view source — reply

unread

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

I didn't check it as I'm on vacation traveling right now, but: Does the
data: wrapper allow \0 in the data? Something like
fopen("data:text/plain,foo\0bar" "r");

johannes

15 years ago by johannes@schlueters.de — view source — reply

unread

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

I didn't check it as I'm on vacation traveling right now, but: Does the
data: wrapper allow \0 in the data? Something like
fopen("data:text/plain,foo\0bar" "r");

Answering to myself:

php -r 'var_dump(file_get_contents("data:text/plain,foo\0bar"));'
string(3) "foo"

The data: wrapper looses the stuff already. So I see no harm by the
patch. It might even produce an error in this situation which is a good
thing (while everybody should use base64 with data: anyways ...)

johannes

15 years ago by Derick Rethans — view source — reply

unread

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

There are quite a few places where we can't solve it centrally, so
perhaps we need to take the same approach in trunk.

I've had a look at that patch, and it looks a little bit like a kludge.
I'm saying that because it's quite easy to miss a specific case where a
line like:

if (strlen(filename) != filename_len) {
```
  RETURN_FALSE;
```
}

should/could be added. I prefer a fix that solves this properly, and
that requires breaking BC as I understood it. Perhaps we could just take
care of this in trunk only? Or at last, add a macro for doing the check
from above?

cheers,
Derick

15 years ago by Rasmus Lerdorf — view source — reply

unread

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

There are quite a few places where we can't solve it centrally, so
perhaps we need to take the same approach in trunk.

I've had a look at that patch, and it looks a little bit like a kludge.
I'm saying that because it's quite easy to miss a specific case where a
line like:
if (strlen(filename) != filename_len) {
RETURN_FALSE;
}
should/could be added. I prefer a fix that solves this properly, and
that requires breaking BC as I understood it. Perhaps we could just take
care of this in trunk only? Or at last, add a macro for doing the check
from above?

Yes, I agree it is a bit of a kludge, but no matter how we do it, it
will be easy to miss a case as there simply isn't a central place to do
this check that will catch all cases. We can make the check prettier by
adding this non-null params type, or a pretty macro, but that doesn't
alleviate the need to go through and find all the places where we need
to do the check.

One thing that makes it a bit easier is that in many instances, but not
all, the check is near an open_basedir check.

-Rasmus

15 years ago by Pierre Joye — view source — reply

unread

hi,

If there is no objection I like to apply this patch to 5.3 tomorrow
morning (Europe) so we have it in for 5.3.4RC1. Please raise your
concerns until then. At the very least we can always revert it after
RC1.

Cheers,

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

There are quite a few places where we can't solve it centrally, so
perhaps we need to take the same approach in trunk.

This should take care of every issue mentioned here:

http://www.madirish.net/?article=436

along with a number of bug reports.

I think the only outstanding issue with the patch is whether to show an
error message when we hit a null in a filesystem path string. With the
Zend part of the patch, right now the error is slightly misleading for
code like this:

$file = "foo.php\0";
include $file . ".png";

This will output:

PHP Warning: include(): Failed opening 'foo.php' for inclusion
(include_path='.:') in foo on line 3

Without this patch, this code will of course simply include the foo.php
file and ignore the .png extension.

-Rasmus

Hi Rasmus,

Hope I understood the problem correctly. If not, this answer won't make sense :)
I do not see a major problem in passing path_len but wonder how much it'd actually solve as we end up calling OS APIs that do not accept path_len, no? I assume we don't want to start searching all these strings for invalid chars before we pass them to the OS.

Andi

-----Original Message-----
From: Rasmus Lerdorf [mailto:rasmus@lerdorf.com]
Sent: Sunday, November 14, 2010 11:54 AM
To: internals
Subject: [PHP-DEV] Adding path_len to all stream functions in trunk

I think we need to pass along the string length to all the stream functions to
maintain binary string safety through this code. This would fix annoying
problems like http://bugs.php.net/39863 and a bunch of similar issues.
Obviously not something we can do in 5.3 without breaking binary compatibility
though. Although we might be able to do something if we assume only chars
valid in the current charset is valid in file paths.

Anybody have any other thoughts on this one?

-Rasmus

--
To unsubscribe, visit:
http://www.php.net/unsub.php

--

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

15 years ago by Gustavo Lopes — view source — reply

unread

On Thu, 18 Nov 2010 00:20:11 -0000, Pierre Joye pierre.php@gmail.com
wrote:

If there is no objection I like to apply this patch to 5.3 tomorrow
morning (Europe) so we have it in for 5.3.4RC1. Please raise your
concerns until then. At the very least we can always revert it after
RC1.

Note the patch broke several tests in ext/standard/tests/file/

--
Gustavo Lopes

15 years ago by Pierre Joye — view source — reply

unread

hi,

Patch applied in 5.3. I will do in trunk next week.

Cheers,

Ok, I went through all the 5.3 code. This should fix the null poisoning
problems in 5.3 without breaking binary compatibility:

http://progphp.com/nullpatch.txt

There are quite a few places where we can't solve it centrally, so
perhaps we need to take the same approach in trunk.

This should take care of every issue mentioned here:

http://www.madirish.net/?article=436

along with a number of bug reports.

I think the only outstanding issue with the patch is whether to show an
error message when we hit a null in a filesystem path string. With the
Zend part of the patch, right now the error is slightly misleading for
code like this:

$file = "foo.php\0";
include $file . ".png";

This will output:

PHP Warning: include(): Failed opening 'foo.php' for inclusion
(include_path='.:') in foo on line 3

Without this patch, this code will of course simply include the foo.php
file and ignore the .png extension.

-Rasmus

Hi Rasmus,

Hope I understood the problem correctly. If not, this answer won't make sense :)
I do not see a major problem in passing path_len but wonder how much it'd actually solve as we end up calling OS APIs that do not accept path_len, no? I assume we don't want to start searching all these strings for invalid chars before we pass them to the OS.

Andi

-----Original Message-----
From: Rasmus Lerdorf [mailto:rasmus@lerdorf.com]
Sent: Sunday, November 14, 2010 11:54 AM
To: internals
Subject: [PHP-DEV] Adding path_len to all stream functions in trunk

I think we need to pass along the string length to all the stream functions to
maintain binary string safety through this code. This would fix annoying
problems like http://bugs.php.net/39863 and a bunch of similar issues.
Obviously not something we can do in 5.3 without breaking binary compatibility
though. Although we might be able to do something if we assume only chars
valid in the current charset is valid in file paths.

Anybody have any other thoughts on this one?

-Rasmus

--
To unsubscribe, visit:
http://www.php.net/unsub.php

--

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

Adding path_len to all stream functions in trunk

How adding a new option char to zend_parse_parameters breaks BC?

Cheers,

So if you have such extension, and you need to have it compatible with previous versions (e.g. PECL one), use the check. That doesn't prevent us from having the flag in the core code and thus keeping it cleaner.

Cheers,

Cheers,

So if you have such extension, and you need to have it compatible with
previous versions (e.g. PECL one), use the check. That doesn't prevent
us from having the flag in the core code and thus keeping it cleaner.