php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Request #76469 ldap_bind should return NULL when called with wrong types
Submitted: 2018-06-13 13:47 UTC Modified: 2018-11-01 08:04 UTC
From: clement dot oudot at worteks dot com Assigned: heiglandreas (profile)
Status: Closed Package: LDAP related
PHP Version: 7.1.18 OS: GNU/Linux
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: clement dot oudot at worteks dot com
New email:
PHP Version: OS:

 

 [2018-06-13 13:47 UTC] clement dot oudot at worteks dot com
Description:
------------
When using an array as password when calling ldap_bind, we have a warning but ldap_errno is not reset, so we keep the value of the previous LDAP operation.

As a lot of PHP code rely on ldap_errno to check if bind is successful, we a major security issue here: sending an array as GET/POST parameter to login age can bypass authentication if the code relies on errno.

Test script:
---------------
<?php
error_reporting(0);

$badpassword = "test";
$goodpassword = "secret";
$bugpassword[] = "a";

$ldap = ldap_connect("ldap://localhost");                                                                                                                              
                                                                                                                                                                       
ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, 3);                                                                                                                  
ldap_set_option($ldap, LDAP_OPT_REFERRALS, 0);                                                                                                                         
                                                                                                                                                                       
$bind = ldap_bind( $ldap, "cn=admin,dc=example,dc=com" , $badpassword );                                                                                               
$errno = ldap_errno($ldap);                                                                                                                                            
echo "Bind 1 returns $errno\n";

$bind = ldap_bind( $ldap, "cn=admin,dc=example,dc=com" , $goodpassword );
$errno = ldap_errno($ldap);
echo "Bind 2 returns $errno\n";

$bind = ldap_bind( $ldap, "cn=admin,dc=example,dc=com" , $bugpassword );
$errno = ldap_errno($ldap);
echo "Bind 3 returns $errno\n";

Expected result:
----------------
Bind 1 returns 49
Bind 2 returns 0
Bind 3 returns 49 # or any error code

Actual result:
--------------
Bind 1 returns 49
Bind 2 returns 0
Bind 3 returns 0

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-06-13 14:21 UTC] cmb@php.net
-Status: Open +Status: Not a bug -Type: Security +Type: Bug -Assigned To: +Assigned To: cmb
 [2018-06-13 14:21 UTC] cmb@php.net
Thank you for taking the time to write to us, but this is not
a bug. Please double-check the documentation available at
http://www.php.net/manual/ and the instructions on how to report
a bug at http://bugs.php.net/how-to-report.php

Passing values of unsupported types to built-in functions is a
userland programming error.  If you want to be extra sure that
this doesn't happen, use `declare(strict_types=1)`.
 [2018-06-13 14:28 UTC] clement dot oudot at worteks dot com
Looking at http://php.net/manual/en/function.ldap-errno.php, we see that errno should be set after each call to an LDAP command. In our case, the last LDAP command fails without setting an errno. Looks like a bug, no?
 [2018-06-13 14:34 UTC] nikic@php.net
Not familiar with ldap_*, but most likely your "Actual result" is missing warnings related to illegal parameters. These come from parameter validation, which works the same for all internal functions and is unrelated to any error-reporting functionality specific to LDAP.

As cmb mentioned, you can turn these into exceptions by setting strict_types=1 and I expect that we will make these always throw in PHP 8, but we're not going to be changing this in PHP 7.x.
 [2018-06-13 15:30 UTC] clement dot oudot at worteks dot com
I understand the point, We need to check ldap_bind return code instead of relying on ldap_errno method.

Maybe this could be added in documentation?
 [2018-06-13 15:46 UTC] cmb@php.net
The documentation states[1]:

| If the parameters given to a function are not what it expects,
| such as passing an array where a string is expected, the return
| value of the function is undefined. In this case it will likely
| return NULL but this is just a convention, and cannot be relied
| upon.

I'm pretty sure that PHP will never return TRUE, so you can check
the return value of ldap_bind().  However, it's better to ensure
that you pass a string (or something compatible in weak type mode)
in the first place.

[1] <http://php.net/manual/en/functions.internal.php>
 [2018-06-13 15:55 UTC] clement dot oudot at worteks dot com
Thanks, We will indeed check the object that is passed as parameter.

Now I see that ldap_bind return FALSE if the call to function was bad (Array instead of string for example), which does not play the operation on LDAP directory, but also when the operation is done on LDAP directory and the return is not success.

There is no way to do the difference in the code, or did I miss something?
 [2018-06-13 16:35 UTC] cmb@php.net
> Now I see that ldap_bind return FALSE if the call to function was
> bad (Array instead of string for example), […]

Indeed! (uncommon, but conforming to the docs)

> There is no way to do the difference in the code, or did I miss
> something?

Besides that you could catch or resolve the case of a wrong
parameter type before even calling ldap_bind(), you could still
call ldap_errno() afterwards, which will return FALSE in the first
case, and an integer in the latter.
 [2018-06-13 17:12 UTC] clement dot oudot at worteks dot com
I agree we can check parameters before calling ldap_bind. But what if we have another error when calling built-in function?

It would be great that ldap_bind returns NULL if there is an error when calling ldap_bind, and 0 if ldap_bind does the LDAP operation and the return is and LDAP error. For the moment, it retuns also NULL. It returns 1 if the LDAP operation returns success.
 [2018-06-13 20:35 UTC] cmb@php.net
-Summary: Bad call to ldap_bind not setting error in ldap_errno +Summary: ldap_bind should return NULL when called with wrong types -Status: Not a bug +Status: Open -Type: Bug +Type: Feature/Change Request -Assigned To: cmb +Assigned To:
 [2018-06-13 20:35 UTC] cmb@php.net
Even though I don't think it is be helpful for anybody, I'm
re-opening this ticket as feature request.
 [2018-11-01 08:04 UTC] heiglandreas@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: heiglandreas
 [2018-11-01 08:04 UTC] heiglandreas@php.net
ldap_bind binds (aka authenticates) a user against the LDAP-server. The method returns - as clearly stated in the docs - true if the bind is succesfull and false in any other cases. As the method expects string-parameters it simply returns false without setting ldap_errno when no strings are given as parameters. It is the responsibility of the user to check for the right types of values. As  it is the responsibility of the user to make sure that ldap_bind is not called with a username but no password at all.

Therefore I'm closing this as it is against the purpose of an authentication function to return something else than "authenticated" and "not authenticated".
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Sat Dec 21 11:01:30 2024 UTC