|
php.net | support | documentation | report a bug | advanced search | search howto | statistics | random bug | login |
[2016-01-10 02:44 UTC] leo at gaspard dot io
Description:
------------
Hello,
Here is the text I put in the commit message of the patch I will attach to this bug report, given I don't find any better way to put it:
==========8<==========8<==========
At the moment, the return value of stream_get_meta_data is composed of
some fields that have a defined value, and some other fields that are
populated with php_stream_populate_meta_data.
This would not be a problem, if php_stream_populate_meta_data was
restricted to writing in fields that are not already set. However, it
turns out, in particular, that a php_stream with ops set to
php_stream_temp_ops fills the metadata with whatever the user supplies.
As a consequence, if we assume $file is attacker-controlled, a program
such as
$uri = stream_get_meta_data(fopen($file, "r"))['uri'];
will yield attacker-controlled data in $uri, and not, as one would
expect, sane PHP-provided data. For instance, $uri will be set to
"eviluri" when $file is set to
data:text/plain;uri=eviluri,
The change to ext/standard/streamsfuncs.c fixes the aforementioned issue
by first filling metadata, and only then filling fields that PHP knows
about. This way, the standardized (and documented) fields will not be
attacker-controlled.
The change made to main/streams/memory.c protects the "mediatype"
variable against the same tampering:
data:text/plain,mediatype=text/x-php,
would set the official mediatype to text/plain, but PHP would interpret
it as text/x-php. This could confuse IDS, IPS or any other application
that reads the same data but does not use the same parser, as these
would not understand the same mediatype as the PHP application does.
==========>8==========>8==========
Hoping this helps,
Leo Gaspard
Test script:
---------------
echo stream_get_meta_data(fopen("data:text/plain;uri=eviluri,", "r"))['uri'];
echo stream_get_meta_data(fopen("data:real/evil;mediatype=text/plain,", "r"))['mediatype'];
Expected result:
----------------
data:text/plain;uri=eviluri,
real/evil
Actual result:
--------------
eviluri
text/plain
PatchesPull RequestsHistoryAllCommentsChangesGit/SVN commits
|
|||||||||||||||||||||||||||
Copyright © 2001-2025 The PHP GroupAll rights reserved. |
Last updated: Sat Oct 25 11:00:01 2025 UTC |
Given it looks like the patch was not attached and I can't attach a patch now (clicking the "Add a Patch" button sends me to a "This bug-report is private"-like page -- maybe this would deserve its own bug report), here is a paste of the entire patch, as generated by git format-patch and applicable by git am -- sorry again for the inconvenience. ==========8<==========8<========== From 1da9a5b048a31f815574a417bb45a813d69875b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Gaspard?= <leo@gaspard.io> Date: Tue, 29 Dec 2015 01:15:13 +0100 Subject: [PATCH] Mitigate return tampering in stream_get_meta_data At the moment, the return value of stream_get_meta_data is composed of some fields that have a defined value, and some other fields that are populated with php_stream_populate_meta_data. This would not be a problem, if php_stream_populate_meta_data was restricted to writing in fields that are not already set. However, it turns out, in particular, that a php_stream with ops set to php_stream_temp_ops fills the metadata with whatever the user supplies. As a consequence, if we assume $file is attacker-controlled, a program such as $uri = stream_get_meta_data(fopen($file, "r"))['uri']; will yield attacker-controlled data in $uri, and not, as one would expect, sane PHP-provided data. For instance, $uri will be set to "eviluri" when $file is set to data:text/plain;uri=eviluri, The change to ext/standard/streamsfuncs.c fixes the aforementioned issue by first filling metadata, and only then filling fields that PHP knows about. This way, the standardized (and documented) fields will not be attacker-controlled. The change made to main/streams/memory.c protects the "mediatype" variable against the same tampering: data:text/plain,mediatype=text/x-php, would set the official mediatype to text/plain, but PHP would interpret it as text/x-php. This could confuse IDS, IPS or any other application that reads the same data but does not use the same parser, as these would not understand the same mediatype as the PHP application does. --- ext/standard/streamsfuncs.c | 12 ++++++------ main/streams/memory.c | 4 +++- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/ext/standard/streamsfuncs.c b/ext/standard/streamsfuncs.c index de0f016..496b990 100644 --- a/ext/standard/streamsfuncs.c +++ b/ext/standard/streamsfuncs.c @@ -507,6 +507,12 @@ PHP_FUNCTION(stream_get_meta_data) array_init(return_value); + if (!php_stream_populate_meta_data(stream, return_value)) { + add_assoc_bool(return_value, "timed_out", 0); + add_assoc_bool(return_value, "blocked", 1); + add_assoc_bool(return_value, "eof", php_stream_eof(stream)); + } + if (!Z_ISUNDEF(stream->wrapperdata)) { Z_ADDREF_P(&stream->wrapperdata); add_assoc_zval(return_value, "wrapper_data", &stream->wrapperdata); @@ -539,12 +545,6 @@ PHP_FUNCTION(stream_get_meta_data) if (stream->orig_path) { add_assoc_string(return_value, "uri", stream->orig_path); } - - if (!php_stream_populate_meta_data(stream, return_value)) { - add_assoc_bool(return_value, "timed_out", 0); - add_assoc_bool(return_value, "blocked", 1); - add_assoc_bool(return_value, "eof", php_stream_eof(stream)); - } } /* }}} */ diff --git a/main/streams/memory.c b/main/streams/memory.c index e2695ff..e36835a 100644 --- a/main/streams/memory.c +++ b/main/streams/memory.c @@ -697,7 +697,9 @@ static php_stream * php_stream_url_wrap_rfc2397(php_stream_wrapper *wrapper, con plen = sep - path; vlen = (semi ? semi - sep : mlen - plen) - 1 /* '=' */; key = estrndup(path, plen); - add_assoc_stringl_ex(&meta, key, plen, sep + 1, vlen); + if (memcmp(key, "mediatype", sizeof("mediatype")-1)) { + add_assoc_stringl_ex(&meta, key, plen, sep + 1, vlen); + } efree(key); plen += vlen + 1; mlen -= plen; -- 2.6.4