#40671 [NEW]: segfault in ldap_get_entries() & LDAP functions implemented poorly

[cardoe at gentoo dot org]

From: cardoe at gentoo dot org
Operating system: Linux
PHP version: 5.2.1
PHP Bug Type: LDAP related
Bug description: segfault in ldap_get_entries() & LDAP functions implemented poorly

Description:
------------
Referencing Bug #38819

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 [email]tony2001@php.net[/email]'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:

[url]http://www.openldap.org/its/index.cgi/Build?id=4690;selectid=4690[/url]
[url]http://www.openldap.org/software/man.cgi?query=ldap_get_values&sektion=3&apropos=0& manpath=OpenLDAP+2.1-Release[/url]

(Notice I went with OpenLDAP 2.1 docs to quell PHP's urge for backwards
compatibility)

The functions char **ldap_get_values(ld, entry, attr) and struct berval
**ldap_get_values_len(ld, entry, attr) are essentially inter-changeable.
The big difference being that the berval struct provides you with a char *
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 structure
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:
[url]http://bugs.gentoo.org/show_bug.cgi?id=133467[/url]

Reproduce code:
---------------
For reproducing code refer to bug #38819

Actual result:
--------------
For a backtrace see bug #38819.

--
Edit bug report at [url]http://bugs.php.net/?id=40671&edit=1[/url]
--
Try a CVS snapshot (PHP 4.4): [url]http://bugs.php.net/fix.php?id=40671&r=trysnapshot44[/url]
Try a CVS snapshot (PHP 5.2): [url]http://bugs.php.net/fix.php?id=40671&r=trysnapshot52[/url]
Try a CVS snapshot (PHP 6.0): [url]http://bugs.php.net/fix.php?id=40671&r=trysnapshot60[/url]
Fixed in CVS: [url]http://bugs.php.net/fix.php?id=40671&r=fixedcvs[/url]
Fixed in release: [url]http://bugs.php.net/fix.php?id=40671&r=alreadyfixed[/url]
Need backtrace: [url]http://bugs.php.net/fix.php?id=40671&r=needtrace[/url]
Need Reproduce Script: [url]http://bugs.php.net/fix.php?id=40671&r=needscript[/url]
Try newer version: [url]http://bugs.php.net/fix.php?id=40671&r=oldversion[/url]
Not developer issue: [url]http://bugs.php.net/fix.php?id=40671&r=support[/url]
Expected behavior: [url]http://bugs.php.net/fix.php?id=40671&r=notwrong[/url]
Not enough info: [url]http://bugs.php.net/fix.php?id=40671&r=notenoughinfo[/url]
Submitted twice: [url]http://bugs.php.net/fix.php?id=40671&r=submittedtwice[/url]
register_globals: [url]http://bugs.php.net/fix.php?id=40671&r=globals[/url]
PHP 3 support discontinued: [url]http://bugs.php.net/fix.php?id=40671&r=php3[/url]
Daylight Savings: [url]http://bugs.php.net/fix.php?id=40671&r=dst[/url]
IIS Stability: [url]http://bugs.php.net/fix.php?id=40671&r=isapi[/url]
Install GNU Sed: [url]http://bugs.php.net/fix.php?id=40671&r=gnused[/url]
Floating point limitations: [url]http://bugs.php.net/fix.php?id=40671&r=float[/url]
No Zend Extensions: [url]http://bugs.php.net/fix.php?id=40671&r=nozend[/url]
MySQL Configuration Error: [url]http://bugs.php.net/fix.php?id=40671&r=mysqlcfg[/url]

[cardoe at gentoo dot org]

ID: 40671
User updated by: cardoe at gentoo dot org
Reported By: cardoe at gentoo dot org
Status: Open
Bug Type: LDAP related
Operating System: Linux
PHP Version: 5.2.1
New Comment:

diff -Nur php-5.1.6/ext/ldap/ldap.c php-5.1.6-ldap-api/ext/ldap/ldap.c
--- php-5.1.6/ext/ldap/ldap.c 2006-01-01 07:50:08.000000000 -0500
+++ php-5.1.6-ldap-api/ext/ldap/ldap.c 2007-02-28 11:04:05.000000000
-0500
@@ -116,7 +116,8 @@
PHP_FE(ldap_first_attribute, third_arg_force_ref)
PHP_FE(ldap_next_attribute, third_arg_force_ref)
PHP_FE(ldap_get_attributes,
NULL)
- PHP_FE(ldap_get_values,
NULL)
+ PHP_FALIAS(ldap_get_values, ldap_get_values_len,
NULL)
+/* PHP_FE(ldap_get_values,
NULL) */
PHP_FE(ldap_get_values_len,
NULL)
PHP_FE(ldap_get_dn,
NULL)
PHP_FE(ldap_explode_dn,
NULL)
@@ -1033,7 +1034,7 @@ BerElement *ber;
char *attribute;
size_t attr_len;
- char **ldap_value;
+ struct berval **ldap_value;
char *dn;

if (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &link,
&result) == FAILURE) {
@@ -1064,16 +1065,16 @@
attribute = ldap_first_attribute(ldap,
ldap_result_entry, &ber);

while (attribute != NULL) {
- ldap_value = ldap_get_values(ldap,
ldap_result_entry, attribute);
- num_values = ldap_count_values(ldap_value);
+ ldap_value = ldap_get_values_len(ldap,
ldap_result_entry, attribute);
+ num_values =
ldap_count_values_len(ldap_value);

MAKE_STD_ZVAL(tmp2);
array_init(tmp2);
add_assoc_long(tmp2, "count", num_values);
for (i = 0; i < num_values; i++) {
- add_index_string(tmp2, i,
ldap_value[i], 1);
+ add_index_stringl(tmp2, i,
ldap_value[i]->bv_val, ldap_value[i]->bv_len, 1);
}
- ldap_value_free(ldap_value);
+ ldap_value_free_len(ldap_value);

attr_len = strlen(attribute);
zend_hash_update(Z_ARRVAL_P(tmp1),
php_strtolower(attribute, attr_len), attr_len+1, (void *) &tmp2,
sizeof(zval *), NULL);
@@ -1180,7 +1181,7 @@
ldap_linkdata *ld;
ldap_resultentry *resultentry;
char *attribute;
- char **ldap_value;
+ struct berval **ldap_value;
int i, num_values, num_attrib;
BerElement *ber;

@@ -1196,16 +1197,16 @@

attribute = ldap_first_attribute(ld->link, resultentry->data,
&ber);
while (attribute != NULL) {
- ldap_value = ldap_get_values(ld->link,
resultentry->data, attribute);
- num_values = ldap_count_values(ldap_value);
+ ldap_value = ldap_get_values_len(ld->link,
resultentry->data, attribute);
+ num_values = ldap_count_values_len(ldap_value);

MAKE_STD_ZVAL(tmp);
array_init(tmp);
add_assoc_long(tmp, "count", num_values);
for (i = 0; i < num_values; i++) {
- add_index_string(tmp, i, ldap_value[i], 1);
+ add_index_stringl(tmp, i,
ldap_value[i]->bv_val, ldap_value[i]->bv_len, 1);
}
- ldap_value_free(ldap_value);
+ ldap_value_free_len(ldap_value);

zend_hash_update(Z_ARRVAL_P(return_value), attribute,
strlen(attribute)+1, (void *) &tmp, sizeof(zval *), NULL);
add_index_string(return_value, num_attrib, attribute,
1);
@@ -1226,46 +1227,6 @@
}
/* }}} */

-/* {{{ proto array ldap_get_values(resource link, resource
result_entry, string attribute)
- Get all values from a result entry */
-PHP_FUNCTION(ldap_get_values)
-{
- zval **link, **result_entry, **attr;
- ldap_linkdata *ld;
- ldap_resultentry *resultentry;
- char *attribute;
- char **ldap_value;
- int i, num_values;
-
- if (ZEND_NUM_ARGS() != 3 || zend_get_parameters_ex(3, &link,
&result_entry, &attr) == FAILURE) {
- WRONG_PARAM_COUNT;
- }
-
- ZEND_FETCH_RESOURCE(ld, ldap_linkdata *, link, -1, "ldap link",
le_link);
- ZEND_FETCH_RESOURCE(resultentry, ldap_resultentry *,
result_entry, -1, "ldap result entry", le_result_entry);
-
- convert_to_string_ex(attr);
- attribute = Z_STRVAL_PP(attr);
-
- if ((ldap_value = ldap_get_values(ld->link, resultentry->data,
attribute)) == NULL) {
- php_error_docref(NULL TSRMLS_CC, E_WARNING, "Cannot get
the value(s) of attribute %s",
ldap_err2string(_get_lderrno(ld->link)));
- RETURN_FALSE;
- }
-
- num_values = ldap_count_values(ldap_value);
-
- array_init(return_value);
-
- for (i = 0; i<num_values; i++) {
- add_next_index_string(return_value, ldap_value[i], 1);
- }
-
- add_assoc_long(return_value, "count", num_values);
- ldap_value_free(ldap_value);
-
-}
-/* }}} */
-
/* {{{ proto array ldap_get_values_len(resource link, resource
result_entry, string attribute)
Get all values with lengths from a result entry */
PHP_FUNCTION(ldap_get_values_len)


Previous Comments:
------------------------------------------------------------------------

[2007-02-28 19:47:03] cardoe at gentoo dot org

Description:
------------
Referencing Bug #38819

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 [email]tony2001@php.net[/email]'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:

[url]http://www.openldap.org/its/index.cgi/Build?id=4690;selectid=4690[/url]
[url]http://www.openldap.org/software/man.cgi?query=ldap_get_values&sektion=3&apropos=0& manpath=OpenLDAP+2.1-Release[/url]

(Notice I went with OpenLDAP 2.1 docs to quell PHP's urge for backwards
compatibility)

The functions char **ldap_get_values(ld, entry, attr) and struct berval
**ldap_get_values_len(ld, entry, attr) are essentially inter-changeable.
The big difference being that the berval struct provides you with a char
* 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
structure 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:
[url]http://bugs.gentoo.org/show_bug.cgi?id=133467[/url]

Reproduce code:
---------------
For reproducing code refer to bug #38819

Actual result:
--------------
For a backtrace see bug #38819.


------------------------------------------------------------------------


--
Edit this bug report at [url]http://bugs.php.net/?id=40671&edit=1[/url]

[tony2001@php.net]

ID: 40671
Updated by: [email]tony2001@php.net[/email]
Reported By: cardoe at gentoo dot org
-Status: Open
+Status: Feedback
Bug Type: LDAP related
Operating System: Linux
PHP Version: 5.2.1
New Comment:

Could you please post the same patch in [email]internals@lists.php.net[/email], so we
can discuss it there, not in the bug tracking system?
Thanks.


Previous Comments:
------------------------------------------------------------------------

[2007-02-28 19:49:22] cardoe at gentoo dot org

diff -Nur php-5.1.6/ext/ldap/ldap.c php-5.1.6-ldap-api/ext/ldap/ldap.c
--- php-5.1.6/ext/ldap/ldap.c 2006-01-01 07:50:08.000000000 -0500
+++ php-5.1.6-ldap-api/ext/ldap/ldap.c 2007-02-28 11:04:05.000000000
-0500
@@ -116,7 +116,8 @@
PHP_FE(ldap_first_attribute, third_arg_force_ref)
PHP_FE(ldap_next_attribute, third_arg_force_ref)
PHP_FE(ldap_get_attributes,
NULL)
- PHP_FE(ldap_get_values,
NULL)
+ PHP_FALIAS(ldap_get_values, ldap_get_values_len,
NULL)
+/* PHP_FE(ldap_get_values,
NULL) */
PHP_FE(ldap_get_values_len,
NULL)
PHP_FE(ldap_get_dn,
NULL)
PHP_FE(ldap_explode_dn,
NULL)
@@ -1033,7 +1034,7 @@ BerElement *ber;
char *attribute;
size_t attr_len;
- char **ldap_value;
+ struct berval **ldap_value;
char *dn;

if (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &link,
&result) == FAILURE) {
@@ -1064,16 +1065,16 @@
attribute = ldap_first_attribute(ldap,
ldap_result_entry, &ber);

while (attribute != NULL) {
- ldap_value = ldap_get_values(ldap,
ldap_result_entry, attribute);
- num_values = ldap_count_values(ldap_value);
+ ldap_value = ldap_get_values_len(ldap,
ldap_result_entry, attribute);
+ num_values =
ldap_count_values_len(ldap_value);

MAKE_STD_ZVAL(tmp2);
array_init(tmp2);
add_assoc_long(tmp2, "count", num_values);
for (i = 0; i < num_values; i++) {
- add_index_string(tmp2, i,
ldap_value[i], 1);
+ add_index_stringl(tmp2, i,
ldap_value[i]->bv_val, ldap_value[i]->bv_len, 1);
}
- ldap_value_free(ldap_value);
+ ldap_value_free_len(ldap_value);

attr_len = strlen(attribute);
zend_hash_update(Z_ARRVAL_P(tmp1),
php_strtolower(attribute, attr_len), attr_len+1, (void *) &tmp2,
sizeof(zval *), NULL);
@@ -1180,7 +1181,7 @@
ldap_linkdata *ld;
ldap_resultentry *resultentry;
char *attribute;
- char **ldap_value;
+ struct berval **ldap_value;
int i, num_values, num_attrib;
BerElement *ber;

@@ -1196,16 +1197,16 @@

attribute = ldap_first_attribute(ld->link, resultentry->data,
&ber);
while (attribute != NULL) {
- ldap_value = ldap_get_values(ld->link,
resultentry->data, attribute);
- num_values = ldap_count_values(ldap_value);
+ ldap_value = ldap_get_values_len(ld->link,
resultentry->data, attribute);
+ num_values = ldap_count_values_len(ldap_value);

MAKE_STD_ZVAL(tmp);
array_init(tmp);
add_assoc_long(tmp, "count", num_values);
for (i = 0; i < num_values; i++) {
- add_index_string(tmp, i, ldap_value[i], 1);
+ add_index_stringl(tmp, i,
ldap_value[i]->bv_val, ldap_value[i]->bv_len, 1);
}
- ldap_value_free(ldap_value);
+ ldap_value_free_len(ldap_value);

zend_hash_update(Z_ARRVAL_P(return_value), attribute,
strlen(attribute)+1, (void *) &tmp, sizeof(zval *), NULL);
add_index_string(return_value, num_attrib, attribute,
1);
@@ -1226,46 +1227,6 @@
}
/* }}} */

-/* {{{ proto array ldap_get_values(resource link, resource
result_entry, string attribute)
- Get all values from a result entry */
-PHP_FUNCTION(ldap_get_values)
-{
- zval **link, **result_entry, **attr;
- ldap_linkdata *ld;
- ldap_resultentry *resultentry;
- char *attribute;
- char **ldap_value;
- int i, num_values;
-
- if (ZEND_NUM_ARGS() != 3 || zend_get_parameters_ex(3, &link,
&result_entry, &attr) == FAILURE) {
- WRONG_PARAM_COUNT;
- }
-
- ZEND_FETCH_RESOURCE(ld, ldap_linkdata *, link, -1, "ldap link",
le_link);
- ZEND_FETCH_RESOURCE(resultentry, ldap_resultentry *,
result_entry, -1, "ldap result entry", le_result_entry);
-
- convert_to_string_ex(attr);
- attribute = Z_STRVAL_PP(attr);
-
- if ((ldap_value = ldap_get_values(ld->link, resultentry->data,
attribute)) == NULL) {
- php_error_docref(NULL TSRMLS_CC, E_WARNING, "Cannot get
the value(s) of attribute %s",
ldap_err2string(_get_lderrno(ld->link)));
- RETURN_FALSE;
- }
-
- num_values = ldap_count_values(ldap_value);
-
- array_init(return_value);
-
- for (i = 0; i<num_values; i++) {
- add_next_index_string(return_value, ldap_value[i], 1);
- }
-
- add_assoc_long(return_value, "count", num_values);
- ldap_value_free(ldap_value);
-
-}
-/* }}} */
-
/* {{{ proto array ldap_get_values_len(resource link, resource
result_entry, string attribute)
Get all values with lengths from a result entry */
PHP_FUNCTION(ldap_get_values_len)

------------------------------------------------------------------------

[2007-02-28 19:47:03] cardoe at gentoo dot org

Description:
------------
Referencing Bug #38819

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 [email]tony2001@php.net[/email]'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:

[url]http://www.openldap.org/its/index.cgi/Build?id=4690;selectid=4690[/url]
[url]http://www.openldap.org/software/man.cgi?query=ldap_get_values&sektion=3&apropos=0& manpath=OpenLDAP+2.1-Release[/url]

(Notice I went with OpenLDAP 2.1 docs to quell PHP's urge for backwards
compatibility)

The functions char **ldap_get_values(ld, entry, attr) and struct berval
**ldap_get_values_len(ld, entry, attr) are essentially inter-changeable.
The big difference being that the berval struct provides you with a char
* 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
structure 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:
[url]http://bugs.gentoo.org/show_bug.cgi?id=133467[/url]

Reproduce code:
---------------
For reproducing code refer to bug #38819

Actual result:
--------------
For a backtrace see bug #38819.


------------------------------------------------------------------------


--
Edit this bug report at [url]http://bugs.php.net/?id=40671&edit=1[/url]