# Adding path_len to all stream functions in trunk

- Thread: https://externals.io/message/50191
- Original archive: https://news-web.php.net/php.internals/50191
- Started by Rasmus Lerdorf on 2010-11-14T19:53:37+00:00
- Messages: 25
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #50191 — Rasmus Lerdorf — 2010-11-14T19:53:37+00:00

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

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

---

## #50199 — Andi Gutmans — 2010-11-15T05:35:54+00:00

URL: https://externals.io/message/50191#50199 · In reply to: #50191

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

---

## #50200 — Rasmus Lerdorf — 2010-11-15T05:43:57+00:00

URL: https://externals.io/message/50191#50200 · In reply to: #50199

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

---

## #50202 — Rasmus Lerdorf — 2010-11-15T10:42:22+00:00

URL: https://externals.io/message/50191#50202 · In reply to: #50199

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

---

## #50203 — Pierre Joye — 2010-11-15T14:00:21+00:00

URL: https://externals.io/message/50191#50203 · In reply to: #50202

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



-- 
Pierre

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

---

## #50224 — Stas Malyshev — 2010-11-16T04:21:20+00:00

URL: https://externals.io/message/50191#50224 · In reply to: #50202

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

---

## #50225 — Rasmus Lerdorf — 2010-11-16T04:45:02+00:00

URL: https://externals.io/message/50191#50225 · In reply to: #50224

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

---

## #50226 — Stas Malyshev — 2010-11-16T05:07:22+00:00

URL: https://externals.io/message/50191#50226 · In reply to: #50225

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

---

## #50227 — Andi Gutmans — 2010-11-16T05:16:16+00:00

URL: https://externals.io/message/50191#50227 · In reply to: #50224

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

---

## #50228 — Rasmus Lerdorf — 2010-11-16T05:24:51+00:00

URL: https://externals.io/message/50191#50228 · In reply to: #50227

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

---

## #50229 — Andi Gutmans — 2010-11-16T05:51:55+00:00

URL: https://externals.io/message/50191#50229 · In reply to: #50228

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

---

## #50230 — Stas Malyshev — 2010-11-16T06:12:27+00:00

URL: https://externals.io/message/50191#50230 · In reply to: #50228

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

---

## #50231 — Rasmus Lerdorf — 2010-11-16T06:15:29+00:00

URL: https://externals.io/message/50191#50231 · In reply to: #50230

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

---

## #50239 — Dmitry Stogov — 2010-11-16T08:08:30+00:00

URL: https://externals.io/message/50191#50239 · In reply to: #50231

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

---

## #50240 — Rasmus Lerdorf — 2010-11-16T08:40:05+00:00

URL: https://externals.io/message/50191#50240 · In reply to: #50239

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

---

## #50241 — johannes@schlueters.de — 2010-11-16T09:29:14+00:00

URL: https://externals.io/message/50191#50241 · In reply to: #50202

> 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

---

## #50242 — Derick Rethans — 2010-11-16T09:44:06+00:00

URL: https://externals.io/message/50191#50242 · In reply to: #50202

> 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

---

## #50243 — Rasmus Lerdorf — 2010-11-16T09:57:26+00:00

URL: https://externals.io/message/50191#50243 · In reply to: #50242

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

---

## #50244 — Pierre Joye — 2010-11-16T11:31:48+00:00

URL: https://externals.io/message/50191#50244 · In reply to: #50226

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

---

## #50245 — Pierre Joye — 2010-11-16T11:34:56+00:00

URL: https://externals.io/message/50191#50245 · In reply to: #50231

hi,


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

---

## #50300 — Julien Pauli — 2010-11-17T15:52:17+00:00

URL: https://externals.io/message/50191#50300 · In reply to: #50245

Is this related to http://www.ush.it/2009/02/08/php-filesystem-attack-vectors/ ?

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

J.Pauli


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

---

## #50312 — Pierre Joye — 2010-11-18T00:20:11+00:00

URL: https://externals.io/message/50191#50312 · In reply to: #50202

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



-- 
Pierre

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

---

## #50327 — johannes@schlueters.de — 2010-11-18T09:59:24+00:00

URL: https://externals.io/message/50191#50327 · In reply to: #50241

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

---

## #50353 — Pierre Joye — 2010-11-18T15:27:34+00:00

URL: https://externals.io/message/50191#50353 · In reply to: #50202

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



-- 
Pierre

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

---

## #50387 — Gustavo Lopes — 2010-11-19T14:24:46+00:00

URL: https://externals.io/message/50191#50387 · In reply to: #50312

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