php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #40671 segfault in ldap_get_entries() & LDAP functions implemented poorly
Submitted: 2007-02-28 19:47 UTC Modified: 2007-03-23 15:24 UTC
Votes:1
Avg. Score:5.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:1 (100.0%)
From: cardoe at gentoo dot org Assigned:
Status: Closed Package: LDAP related
PHP Version: 5.2.1 OS: Linux
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: cardoe at gentoo dot org
New email:
PHP Version: OS:

 

 [2007-02-28 19:47 UTC] 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 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=4690;selectid=4690
http://www.openldap.org/software/man.cgi?query=ldap_get_values&sektion=3&apropos=0&manpath=OpenLDAP+2.1-Release

(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: http://bugs.gentoo.org/show_bug.cgi?id=133467

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

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

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2007-02-28 19:49 UTC] 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 20:03 UTC] tony2001@php.net
Could you please post the same patch in internals@lists.php.net, so we can discuss it there, not in the bug tracking system?
Thanks.
 [2007-03-08 01:00 UTC] php-bugs at lists dot php dot net
No feedback was provided for this bug for over a week, so it is
being suspended automatically. If you are able to provide the
information that was originally requested, please do so and change
the status of the bug back to "Open".
 [2007-03-23 15:24 UTC] cardoe@php.net
Fixed in PHP 5_2 Branch and PHP Head
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Apr 25 09:01:29 2024 UTC