Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:57518
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.210.170 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <38EE3732-F134-4C02-8F93-2E9C61FD1E81@ktamura.com>
References: <38EE3732-F134-4C02-8F93-2E9C61FD1E81@ktamura.com>
Date: Wed, 25 Jan 2012 20:33:42 -0700
Message-ID: <CALKFbxv7FR-kP+PTWyj58hgD78=5E2ihCTAxzdnywQfdtmkz_g@mail.gmail.com>
To: Kiyoto Tamura <me@ktamura.com>
Cc: internals@lists.php.net
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] A potential patch for Bug#60668
From: chrisstocktonaz@gmail.com (Chris Stockton)

Hello,

On Wed, Jan 25, 2012 at 5:43 PM, Kiyoto Tamura <me@ktamura.com> wrote:
> vrana has raise a good point in a potentially dangerous behavior with ini=
_set() in https://bugs.php.net/bug.php?id=3D60668.
>
> Here is my proposed patch. Feedback is appreciated. Thanks!
>
> Kiyoto Tamura
>
> diff --git a/Zend/zend_ini.c b/Zend/zend_ini.c
> index a7ec5d7..89b1287 100644
> --- a/Zend/zend_ini.c
> +++ b/Zend/zend_ini.c
> @@ -83,6 +83,23 @@ static int zend_restore_ini_entry_wrapper(zend_ini_ent=
ry **ini_entry TSRMLS_DC)
> =A0}
> =A0/* }}} */
>
> +static uint zend_trim_after_carriage_return(char *value, uint value_leng=
th) /* {{{ */
> +{
> + =A0 =A0uint ii;
> + =A0 =A0char prev_c =3D '\0', curr_c;
> + =A0 =A0for (ii =3D 0; ii < value_length; ++ii) {
> + =A0 =A0 =A0 =A0curr_c =3D *value;
> + =A0 =A0 =A0 =A0if (prev_c =3D=3D '\r' && curr_c =3D=3D '\n') {
> + =A0 =A0 =A0 =A0 =A0 =A0return ii - 1;
> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0prev_c =3D curr_c;
> + =A0 =A0 =A0 =A0++value;
> + =A0 =A0}
> +
> + =A0 =A0return value_length;
> +}
> +/* }}} */
> +

To comment specifically on the patch itself; I am not sure you need a
new zend func here, I think it would be more convenient (and safer) to
use php_trim in standard/string.c. Although I must say looking at the
bug, I am not sure this is exactly something that should be "fixed".
It could be a BC break and in my opinion is just a demonstration of
failure to properly sanitize user input as opposed to a security flaw
in php design itself.

-Chris