Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:57519
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain ktamura.com from 209.85.210.170 cause and error)
Mime-Version: 1.0 (Apple Message framework v1084)
Content-Type: text/plain; charset=us-ascii
In-Reply-To: <CALKFbxv7FR-kP+PTWyj58hgD78=5E2ihCTAxzdnywQfdtmkz_g@mail.gmail.com>
Date: Wed, 25 Jan 2012 20:26:34 -0800
Cc: internals@lists.php.net
Content-Transfer-Encoding: quoted-printable
Message-ID: <6B45DCFC-12E9-42A9-9DA5-0E49DBA0DC10@ktamura.com>
References: <38EE3732-F134-4C02-8F93-2E9C61FD1E81@ktamura.com> <CALKFbxv7FR-kP+PTWyj58hgD78=5E2ihCTAxzdnywQfdtmkz_g@mail.gmail.com>
To: Chris Stockton <chrisstocktonaz@gmail.com>
Subject: Re: [PHP-DEV] A potential patch for Bug#60668
From: me@ktamura.com (Kiyoto Tamura)

Thanks for the pointers. While I agree with you that application =
developers must be cognizant of input sanitization, I am not sure what's =
the value of allowing CR-LF in the user agent value. Said another way, =
if the user agent contains CR-LF, it was most definitely not meant to be =
there. Can you elaborate on the downside of the stricter check advocated =
by vrana?

Personally, I am ambivalent. I am just curious about the other side of =
the argument.

Thanks,

Kiyoto Tamura

On Jan 25, 2012, at 7:33 PM, Chris Stockton wrote:

> Hello,
>=20
> 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.
>>=20
>> Here is my proposed patch. Feedback is appreciated. Thanks!
>>=20
>> Kiyoto Tamura
>>=20
>> 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_entry **ini_entry TSRMLS_DC)
>>  }
>>  /* }}} */
>>=20
>> +static uint zend_trim_after_carriage_return(char *value, uint =
value_length) /* {{{ */
>> +{
>> +    uint ii;
>> +    char prev_c =3D '\0', curr_c;
>> +    for (ii =3D 0; ii < value_length; ++ii) {
>> +        curr_c =3D *value;
>> +        if (prev_c =3D=3D '\r' && curr_c =3D=3D '\n') {
>> +            return ii - 1;
>> +        }
>> +        prev_c =3D curr_c;
>> +        ++value;
>> +    }
>> +
>> +    return value_length;
>> +}
>> +/* }}} */
>> +
>=20
> 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.
>=20
> -Chris