Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » LDAP functions implemented poorly »

LDAP functions implemented poorly

php.internals

Doug Goldstein

19 years ago
Referencing Bug #38819 & Bug #40671 http://bugs.php.net/bug.php?id=38819 http://bugs.php.net/bug.php?id=40671 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

Antony Dovgal

19 years ago
Hello. Thanks for sending the patch here. On 02/28/2007 11:33 PM, Doug Goldstein wrote:
> Referencing Bug #38819 & Bug #40671 > http://bugs.php.net/bug.php?id=38819 > http://bugs.php.net/bug.php?id=40671 > > 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.
> 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.
Wait, I thought the DEPRECATED thing fixed it (I can't test it myself as I don't use LDAP). If not, then what was it all about?
> 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.
Some comments regarding the patch: First of all, why do you add a new function and comment the old one instead of changing/fixing the old function? We definitely don't need dead/commented out code in the sources =) Then, it's a bit too late to change 5.1.6, when 5.2.2 is on it's way, so surely we would like to see the patches for 5.2.2 instead. You can get its sources from CVS using PHP_5_2 branch: cvs -d :pserver:anonymous@cvs.php.net/repository co -r PHP_5_2 php-src
-- Wbr, Antony Dovgal

Doug Goldstein

19 years ago
Antony Dovgal wrote:
> Hello. > Thanks for sending the patch here. > > On 02/28/2007 11:33 PM, Doug Goldstein wrote: >> Referencing Bug #38819 & Bug #40671 >> http://bugs.php.net/bug.php?id=38819 >> http://bugs.php.net/bug.php?id=40671 >> >> 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. > >> 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. > > Wait, I thought the DEPRECATED thing fixed it (I can't test it myself as I > don't use LDAP). > If not, then what was it all about?
The test case still failed for me. It also does not address the fact that the usage of the PHP ldap functions is unsafe because they could return a char * with a non-NULL terminated string and your code will eventually call strlen() on it. With the changes I have made it knows the length from the LDAP server and doesn't need to use strlen(). Since add_index_stringl() doesn't call strlen() since the length is provided.
> >> 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. > > Some comments regarding the patch: > First of all, why do you add a new function and comment the old one > instead of > changing/fixing the old function? > We definitely don't need dead/commented out code in the sources =)
I didn't add a new function. I replaced a function with an alias since the function is pointlessly there. It's duplicated code and the net result is just an alias.
> > Then, it's a bit too late to change 5.1.6, when 5.2.2 is on it's way, so > surely > we would like to see the patches for 5.2.2 instead.
This patch is against 5.1.6 and the 5.2 branch. The code in question has not been touched since before 5.1.0 was released. I tested it against both 5.2 branch and 5.1.6 prior to uploading it.
> You can get its sources from CVS using PHP_5_2 branch: > cvs -d :pserver:anonymous@cvs.php.net/repository co -r PHP_5_2 php-src
You might want to update your instructions to people since the user is "cvsread" and not "anonymous".

Antony Dovgal

19 years ago
On 03/01/2007 12:04 AM, Doug Goldstein wrote:
>> Wait, I thought the DEPRECATED thing fixed it (I can't test it myself as I >> don't use LDAP). >> If not, then what was it all about? > > The test case still failed for me. It also does not address the fact that > the usage of the PHP ldap functions is unsafe because they could return a > char * with a non-NULL terminated string and your code will eventually > call strlen() on it. With the changes I have made it knows the length from > the LDAP server and doesn't need to use strlen(). Since > add_index_stringl() doesn't call strlen() since the length is provided.
Did you really test it with non-NULL terminated strings? Don't you need to add '\0' manually?
>> Some comments regarding the patch: >> First of all, why do you add a new function and comment the old one >> instead of >> changing/fixing the old function? >> We definitely don't need dead/commented out code in the sources =) > > I didn't add a new function. I replaced a function with an alias since the > function is pointlessly there. It's duplicated code and the net result is > just an alias.
No, replace means "add something and delete the original", you added new function and commented out the original one, which made me wonder why you didn't change the original function instead. What was the point in creating new function adding an alias? There is nothing wrong, I'm just curious.
>> Then, it's a bit too late to change 5.1.6, when 5.2.2 is on it's way, so >> surely >> we would like to see the patches for 5.2.2 instead. > > This patch is against 5.1.6 and the 5.2 branch. The code in question has > not been touched since before 5.1.0 was released. I tested it against both > 5.2 branch and 5.1.6 prior to uploading it.
Ok then.
>> You can get its sources from CVS using PHP_5_2 branch: >> cvs -d :pserver:anonymous@cvs.php.net/repository co -r PHP_5_2 php-src > > You might want to update your instructions to people since the user is > "cvsread" and not "anonymous".
Oh, that's just my memory.
-- Wbr, Antony Dovgal

Doug Goldstein

19 years ago
Antony Dovgal wrote:
> On 03/01/2007 12:04 AM, Doug Goldstein wrote: >>> Wait, I thought the DEPRECATED thing fixed it (I can't test it myself >>> as I >>> don't use LDAP). >>> If not, then what was it all about? >> >> The test case still failed for me. It also does not address the fact >> that >> the usage of the PHP ldap functions is unsafe because they could return >> a >> char * with a non-NULL terminated string and your code will eventually >> call strlen() on it. With the changes I have made it knows the length >> from >> the LDAP server and doesn't need to use strlen(). Since >> add_index_stringl() doesn't call strlen() since the length is provided. > > Did you really test it with non-NULL terminated strings? > Don't you need to add '\0' manually?
The test is that you run the example code from bug #38819, watch PHP crash. Apply my patch and watch PHP not crash. Fairly simple. My backtrace is identical to the reporter's. If you read the comments by the OpenLDAP developers in the two bugs referenced they have the same reason for using ldap_get_values_len() instead of ldap_get_values() because it's safer incase the data is non-NULL terminated data. In this case PHP's assumption that it's NULL terminated is flawed since it's crashing since it's extending past the end of it's memory segment. (as visible from bug #38819)
> >>> Some comments regarding the patch: >>> First of all, why do you add a new function and comment the old one >>> instead of >>> changing/fixing the old function? >>> We definitely don't need dead/commented out code in the sources =) >> >> I didn't add a new function. I replaced a function with an alias since >> the >> function is pointlessly there. It's duplicated code and the net result >> is >> just an alias. > > No, replace means "add something and delete the original", you added new > function and > commented out the original one, which made me wonder why you didn't change > the original function instead. > What was the point in creating new function adding an alias? > There is nothing wrong, I'm just curious.
I removed PHP_FE(ldap_get_values) because it's a pointless function. It's identical in code to PHP_FE(ldap_get_values_len) so I made it an alias. Less code to maintain for you guys. Less bloat. Everyone should be happy.

Antony Dovgal

19 years ago
On 03/01/2007 12:35 AM, Doug Goldstein wrote:
>> Did you really test it with non-NULL terminated strings? >> Don't you need to add '\0' manually? > > The test is that you run the example code from bug #38819, watch PHP > crash. Apply my patch and watch PHP not crash. Fairly simple. My backtrace > is identical to the reporter's.
Well, I can't do it myself since I don't even have a LDAP server installed. That's why I asked you the question.
> If you read the comments by the OpenLDAP developers in the two bugs > referenced they have the same reason for using ldap_get_values_len() > instead of ldap_get_values() because it's safer incase the data is > non-NULL terminated data. In this case PHP's assumption that it's NULL > terminated is flawed since it's crashing since it's extending past the end > of it's memory segment. (as visible from bug #38819)
I have no doubts it's true, but the question was: did you really test [the NEW patched version of] the code with non-NULL terminated strings? As far as I understand, if OpenLDAP returns non-NULL terminated string, we'd most likely need to add the terminator, otherwise we'd end up with non-NULL terminated strings in PHP. Hence the question. Do I make myself clear enough?
>> No, replace means "add something and delete the original", you added new >> function and >> commented out the original one, which made me wonder why you didn't change >> the original function instead. >> What was the point in creating new function adding an alias? >> There is nothing wrong, I'm just curious. > > I removed PHP_FE(ldap_get_values) because it's a pointless function. It's > identical in code to PHP_FE(ldap_get_values_len) so I made it an alias. > Less code to maintain for you guys. Less bloat. Everyone should be happy.
Ok, so we got some misunderstanding here..
> I removed PHP_FE(ldap_get_values) because it's a pointless function.
The point is that the patch should FIX this function, not REMOVE it. And it's still there, but commented out (which makes not sense). Don't get me wrong, I can (and I will) rewrite it myself, but I just wanted to understand what was the initial idea behind it.
-- Wbr, Antony Dovgal

Antony Dovgal

19 years ago
On 03/01/2007 12:46 AM, Antony Dovgal wrote:
>> I removed PHP_FE(ldap_get_values) because it's a pointless function. > > The point is that the patch should FIX this function, not REMOVE it. > And it's still there, but commented out (which makes not sense).
Uhm.. Ignore that, I need to sleep more.
-- Wbr, Antony Dovgal

Doug Goldstein

19 years ago
Antony Dovgal wrote:
> On 03/01/2007 12:35 AM, Doug Goldstein wrote: >>> Did you really test it with non-NULL terminated strings? >>> Don't you need to add '\0' manually? >> >> The test is that you run the example code from bug #38819, watch PHP >> crash. Apply my patch and watch PHP not crash. Fairly simple. My >> backtrace >> is identical to the reporter's. > > Well, I can't do it myself since I don't even have a LDAP server > installed. > That's why I asked you the question. > >> If you read the comments by the OpenLDAP developers in the two bugs >> referenced they have the same reason for using ldap_get_values_len() >> instead of ldap_get_values() because it's safer incase the data is >> non-NULL terminated data. In this case PHP's assumption that it's NULL >> terminated is flawed since it's crashing since it's extending past the >> end >> of it's memory segment. (as visible from bug #38819) > > I have no doubts it's true, but the question was: > did you really test [the NEW patched version of] the code with non-NULL > terminated strings?
If I run the example PHP code from bug #38819, PHP will merrily run off the end of a string into no man's land and crash as per the backtrace in bug #38819. With the patch applied, it does not. That sound clearly like the example PHP code in bug #38819 is testing it with a non-NULL terminated string. I hope this is clear.
> > As far as I understand, if OpenLDAP returns non-NULL terminated string, > we'd most likely need > to add the terminator, otherwise we'd end up with non-NULL terminated > strings in PHP. > Hence the question. > > Do I make myself clear enough?
If you read the PHP API documentation, the API function call add_index_stringl() http://php.interec.es/manual/nl/zend-api.add-index-stringl.php Was created for this explicit purpose. There is no need to tack on a NULL character since it is automatically happening. If it's not happening, then it is properly using the size passed to it to always ensure it doesn't wander off the end of the memory address anywhere. I hope that is clear.
> >>> No, replace means "add something and delete the original", you added >>> new >>> function and >>> commented out the original one, which made me wonder why you didn't >>> change >>> the original function instead. >>> What was the point in creating new function adding an alias? >>> There is nothing wrong, I'm just curious. >> >> I removed PHP_FE(ldap_get_values) because it's a pointless function. >> It's >> identical in code to PHP_FE(ldap_get_values_len) so I made it an alias. >> Less code to maintain for you guys. Less bloat. Everyone should be >> happy. > > Ok, so we got some misunderstanding here.. > >> I removed PHP_FE(ldap_get_values) because it's a pointless function. > > The point is that the patch should FIX this function, not REMOVE it. > And it's still there, but commented out (which makes not sense). > > Don't get me wrong, I can (and I will) rewrite it myself, but I just > wanted to understand what was the initial idea behind it.
The implemented PHP_FUNCTION(ldap_get_values) and PHP_FUNCTION(ldap_get_values_len) differ on in the fact that they call ldap_get_values() and ldap_get_values_len(). Since the premise behind all these fixes is to remove the calls to ldap_get_values() and replace them with ldap_get_values_len() so you ensure that you don't wander off the end of a string and cause a crash.. that makes PHP_FUNCTION(ldap_get_values) and PHP_FUNCTION(ldap_get_values_len) identical in code. Line for line there would be 0 difference. Rather then having a completely duplicated function and duplicated code (duplicated code is bad), I changed PHP_FUNCTION(ldap_get_values) to an alias of PHP_FUNCTION(ldap_get_values_len)

Antony Dovgal

19 years ago
On 03/01/2007 01:11 AM, Doug Goldstein wrote:
> Antony Dovgal wrote: >> On 03/01/2007 12:35 AM, Doug Goldstein wrote: >>>> Did you really test it with non-NULL terminated strings? >>>> Don't you need to add '\0' manually? >>> >>> The test is that you run the example code from bug #38819, watch PHP >>> crash. Apply my patch and watch PHP not crash. Fairly simple. My >>> backtrace >>> is identical to the reporter's. >> >> Well, I can't do it myself since I don't even have a LDAP server >> installed. >> That's why I asked you the question. >> >>> If you read the comments by the OpenLDAP developers in the two bugs >>> referenced they have the same reason for using ldap_get_values_len() >>> instead of ldap_get_values() because it's safer incase the data is >>> non-NULL terminated data. In this case PHP's assumption that it's NULL >>> terminated is flawed since it's crashing since it's extending past the >>> end >>> of it's memory segment. (as visible from bug #38819) >> >> I have no doubts it's true, but the question was: >> did you really test [the NEW patched version of] the code with non-NULL >> terminated strings? > > If I run the example PHP code from bug #38819, PHP will merrily run off > the end of a string into no man's land and crash as per the backtrace in > bug #38819. With the patch applied, it does not. That sound clearly like > the example PHP code in bug #38819 is testing it with a non-NULL > terminated string. I hope this is clear.
Yes, that's perfectly clear, thanks.
-- Wbr, Antony Dovgal

Doug Goldstein

19 years ago
Antony Dovgal wrote:
> On 03/01/2007 01:11 AM, Doug Goldstein wrote: >> Antony Dovgal wrote: >>> On 03/01/2007 12:35 AM, Doug Goldstein wrote: >>>>> Did you really test it with non-NULL terminated strings? >>>>> Don't you need to add '\0' manually? >>>> >>>> The test is that you run the example code from bug #38819, watch PHP >>>> crash. Apply my patch and watch PHP not crash. Fairly simple. My >>>> backtrace >>>> is identical to the reporter's. >>> >>> Well, I can't do it myself since I don't even have a LDAP server >>> installed. >>> That's why I asked you the question. >>> >>>> If you read the comments by the OpenLDAP developers in the two bugs >>>> referenced they have the same reason for using ldap_get_values_len() >>>> instead of ldap_get_values() because it's safer incase the data is >>>> non-NULL terminated data. In this case PHP's assumption that it's NULL >>>> terminated is flawed since it's crashing since it's extending past the >>>> end >>>> of it's memory segment. (as visible from bug #38819) >>> >>> I have no doubts it's true, but the question was: >>> did you really test [the NEW patched version of] the code with non-NULL >>> terminated strings? >> >> If I run the example PHP code from bug #38819, PHP will merrily run off >> the end of a string into no man's land and crash as per the backtrace in >> bug #38819. With the patch applied, it does not. That sound clearly like >> the example PHP code in bug #38819 is testing it with a non-NULL >> terminated string. I hope this is clear. > > Yes, that's perfectly clear, thanks. >
So any word about this getting merged for PHP 5.2.2?
-- Doug Goldstein <cardoe@gentoo.org> http://dev.gentoo.org/~cardoe/

Wez Furlong

19 years ago
My really really late 2 cents on this. 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. 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 :) --Wez. On 2/28/07, Doug Goldstein <cardoe@gentoo.org> wrote:

Doug Goldstein

19 years ago
Wez Furlong wrote:
> My really really late 2 cents on this. > > 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. > > 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.
> > --Wez. > > On 2/28/07, Doug Goldstein <cardoe@gentoo.org> wrote: >> Referencing Bug #38819 & Bug #40671 >> http://bugs.php.net/bug.php?id=38819 >> http://bugs.php.net/bug.php?id=40671 >> >> 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 >> -- >> PHP Internals - PHP Runtime Development Mailing List >> To unsubscribe, visit: http://www.php.net/unsub.php >> >> >
-- Doug Goldstein <cardoe@gentoo.org> http://dev.gentoo.org/~cardoe/

Wez Furlong

19 years ago
What I meant was, make sure that ldap_get_values_len() is also present in the LDAP libraries on Solaris and Windows. I've checked the docs on those platforms, and they both have those APIs. The reason I bring it up is because those 3 implementations have 98% the same API but some differences in some areas. Anyhow, it appears safe; carry on :) --Wez. On 3/7/07, Doug Goldstein <cardoe@gentoo.org> wrote: