Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:28283
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain gentoo.org does not designate 205.152.59.69 as permitted sender)
Message-ID: <45EE5C85.5030409@gentoo.org>
Date: Wed, 07 Mar 2007 01:32:37 -0500
User-Agent: Thunderbird 1.5.0.9 (X11/20061222)
MIME-Version: 1.0
To: Wez Furlong <kingwez@gmail.com>
CC:  internals@lists.php.net
References: <40059.216.155.111.10.1172694790.squirrel@webmail.cardoe.com> <4e89b4260703062218v73c3655agdd72b5b7bfdfe41f@mail.gmail.com>
In-Reply-To: <4e89b4260703062218v73c3655agdd72b5b7bfdfe41f@mail.gmail.com>
OpenPGP: id=8B4264CB;
	url=http://dev.gentoo.org/~cardoe/cardoe.asc
Content-Type: multipart/signed; micalg=pgp-sha1;
 protocol="application/pgp-signature";
 boundary="------------enig13A1CDDCD2CB6410FDB31A1D"
Subject: Re: [PHP-DEV] LDAP functions implemented poorly
From: cardoe@gentoo.org (Doug Goldstein)

--------------enig13A1CDDCD2CB6410FDB31A1D
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

Wez Furlong wrote:
> My really really late 2 cents on this.
>=20
> Please make sure that your changes don't make the extension depend on
> OpenLDAP.  Solaris and Windows LDAP implementations are close but not
> totally the same as OpenLDAP.
>=20
> I haven't looked at your patches and probably won't have time to do
> so; I'm merely doing a drive-by peek at the recent traffic on the
> lists, so forgive me if you've already taken this into consideration
> :)

Considering my changes just call ldap_get_values_len() instead of
ldap_get_values(), both of which were implemented on all platforms and
arches (i.e. no IFDEF sections) then I assume if ldap_get_values_len()
which appears to have existed and been used in PHP code since the start
of the extension worked... then using it instead of ldap_get_values()
should still work. as per the IETF spec for LDAP C API, the
ldap_get_values_len() function basically returns you a char * as well as
the length of the data. Instead of just returning you the char * and
relying on your code to call strlen(). Which when it comes to binary
data is not safe.

So it shouldn't affect what LDAP server is being used.


>=20
> --Wez.
>=20
> On 2/28/07, Doug Goldstein <cardoe@gentoo.org> wrote:
>> Referencing Bug #38819 & Bug #40671
>> http://bugs.php.net/bug.php?id=3D38819
>> http://bugs.php.net/bug.php?id=3D40671
>>
>> Essentially I looked through the above mentioned bug, the bugs opened
>> with OpenLDAP developers, and then reviewed ext/ldap/ldap.c and it
>> appears the API calls made by PHP are not necessarily the safest ways =
to
>> write the PHP wrapper functions. Based on tony2001@php.net's comment
>> that the LDAP module is unmaintained I went ahead and made some
>> changes.
>>
>> If you read OpenLDAP's API and comments by OpenLDAP Core Developers,
>> available at:
>>
>> http://www.openldap.org/its/index.cgi/Build?id=3D4690;selectid=3D4690
>> http://www.openldap.org/software/man.cgi?query=3Dldap_get_values&sekti=
on=3D3
>> &apropos=3D0&manpath=3DOpenLDAP+2.1-Release
>>
>> (Notice I went with OpenLDAP 2.1 docs to quell PHP's urge for backward=
s
>> compatibility)
>>
>> The functions char **ldap_get_values(ld, entry, attr) and struct berva=
l
>> **ldap_get_values_len(ld, entry, attr) are essentially inter-changeabl=
e.
>> The big difference being that the berval struct provides you with a ch=
ar
>> * and the size_t of the data. Rather then just a char * that you then
>> have to strlen() which will result in problems if the returned data is=

>> not NULL terminated data. PHP's internal functions make the mistake of=

>> assuming all data will be string data (NULL terminated char *) data,
>> which is the cause of the crash in bug #38819.
>>
>> The patch attached removes all of those assumptions and uses
>> ldap_get_values_len() and uses the length provided back by the structu=
re
>> to feed add_index_stringl() instead of using add_index_string() which
>> will call it's own strlen() on the provided data.
>>
>> This patch also removes ldap_get_values() as a PHP function and makes =
it
>> an alias of ldap_get_values_len() since there's no difference and the
>> same data can be returned, it's just a safer version.
>>
>> The attached patch fixes the test case provided in bug #38819.
>>
>> Referencing for my own purposes:
>> http://bugs.gentoo.org/show_bug.cgi?id=3D133467
>> --=20
>> PHP Internals - PHP Runtime Development Mailing List
>> To unsubscribe, visit: http://www.php.net/unsub.php
>>
>>
>=20


--=20
Doug Goldstein <cardoe@gentoo.org>
http://dev.gentoo.org/~cardoe/


--------------enig13A1CDDCD2CB6410FDB31A1D
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF7lyIoeSe8B0zEfwRAml7AJwI/NEsXeIz9Vt2rsSM7gF0UDzBxwCfSV4Y
1sJmWinlu0wOs0v3eC1yf28=
=xbuU
-----END PGP SIGNATURE-----

--------------enig13A1CDDCD2CB6410FDB31A1D--