LDAP Login Script

Question

This is my first time ever having code peer reviewed! This is just the main function I'm concerned about.

Edit

Apologies for the initial vagueness of my question, this is the "login" part of my login script the classes is initial instantiated on the index.php which handles a $_POST request from login.php I wont paste them here because they are generic input fields.

The class construct looks something like (Removed the not so relevant parts)

public function __construct($DB_con)
{
    // create/read session, absolutely necessary
    session_start();

    //New method of database connection
    $this->db = $DB_con;

    // check the possible login actions:
    // if user tried to log out (happen when user clicks logout button)
    if (isset($_GET["logout"])) {
        $this->doLogout();
    }
    // login via post data (if user just submitted a login form)
    elseif (isset($_POST["login"])) {
        $this->dologinWithPostData();
    }
}

The logic of logging in does as follows;

    private function dologinWithPostData()
    {

        if (empty($_POST['user_name'])) {
            $this->errors[] = "Username field was empty.";
        } elseif (empty($_POST['user_password'])) {
            $this->errors[] = "Password field was empty.";
        } elseif (!empty($_POST['user_name']) && !empty($_POST['user_password'])) {
            //ldap_set_option(NULL, LDAP_OPT_DEBUG_LEVEL, 7);

            $username = $_POST['user_name'];
            $password = $_POST['user_password'];
            $adServer = "ldaps://The Address";

            $ldap = ldap_connect($adServer);

            $ldaprdn = 'MYDN.net' . "\\" . $username;

            ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, 3);
            ldap_set_option($ldap, LDAP_OPT_REFERRALS, 0);

            $bind = @ldap_bind($ldap, $ldaprdn, $password);

            if ($bind) {

                // Get user info by there account login
                $filter="(sAMAccountName=$username)";
                $result = ldap_search($ldap,"dc=MYDC,dc=NET",$filter);
                ldap_sort($ldap,$result,"sn");
                $info = ldap_get_entries($ldap, $result);


                @ldap_close($ldap);

                $fullName = $info[0]["displayname"][0];
                $dn = $info[0]["dn"];
                $groups = array();

                if (array_key_exists("memberof", $info[0])){
                    $x = $info[0]["memberof"];
                    $groups = array();

                    foreach ($x as $key){
                        if(strlen($key)>1){ 
                            array_push($groups, $this->get_string_between($key, "CN=", ",") );
                        }
                    }
                }

                @ldap_close($ldap);
                $_SESSION['userID'] = bin2hex($info[0]["objectguid"][0]);
                $_SESSION['fullName'] = $fullName;
                $_SESSION['userGroups'] = $groups;
                $_SESSION['loggedOnStatus'] = "1";

                if(in_array('Dept_Directors', $groups)){
                    $_SESSION['userLevel'] = 10;
                }elseif(in_array('Dept_Admin', $groups)){
                    $_SESSION['userLevel'] = 7;
                }else{
                    $_SESSION['userLevel'] = 0;
                }

                if (strpos($dn, 'OU=MYOU') !== false) {
                    $_SESSION['userType'] = 2;
                }elseif(strpos($dn, 'OU=MYOU') !== false){
                    $_SESSION['userType'] = 1;
                }




                if($this->checkUserProfileExsists() == false){
                    if(!($this->createUserProfile())){
                        $msg="Error Creating User Profile ";
                        echo $msg;
                    }
                }           
            } else {
                @ldap_close($ldap);     
                $this->errors[] = "Wrong Username or Password";

            }

        } else {
        @ldap_close($ldap);
        echo "Must Enter UserName & Password Try again.";
        }
    }

Now the code does fire of to some other functions that check the database to see if the user's ID (GUID object in AD) has been updated (I'm not a great AD administrator sometimes you gota to what you gota do), or if its a new user who gets authenticated with LDAP and just needs a profile setting up.

My main concern is that script isn't secure, I use HTTPS as standard. But apart from the fact there is no effort made to sanitize the user input (LDAP injection?) is there anything "wrong" or insecure about my code? It is partially based of the The PHP Login Project

Answer 1

Logical inconsistencies and careless mistakes

Let's look at the outline:

if (empty($_POST['user_name'])) {
    $this->errors[] = "Username field was empty.";
} elseif (empty($_POST['user_password'])) {
    $this->errors[] = "Password field was empty.";
} elseif (!empty($_POST['username']) && !empty($_POST['user_password'])) {
    …
    $ldap = ldap_connect(…);
    …
} else {
@ldap_close($ldap);
echo "Wrong password. Try again.";
}

The third condition, if it ever reaches there, is always true. The else clause at the end is, therefore, never reachable. Good thing, too, because $ldap would not be defined if that dead code were ever executed.

Also, stare at these few lines for a while and tell me what is going on:

if (strpos($dn, 'OU=MYOU') !== false) {
    $_SESSION['userType'] = 2;
}elseif(strpos($dn, 'OU=MYOU') !== false){
    $_SESSION['userType'] = 1;
}

This method name is misspelled:

if($this->checkUserProfileExsists() == false){ … }

… and in any case it would be better named userProfileExists().

You called @ldap_close($ldap) too many times:

@ldap_close($ldap);

$fullName = $info[0]["displayname"][0];
$dn = $info[0]["dn"];
$groups = array();

if (array_key_exists("memberof", $info[0])){
    …
}

@ldap_close($ldap);

Suppressing errors using @ is OK with ldap_close(), since you don't really care much if it fails. But you shouldn't do it anywhere else.

Organization

This function never returns a value. It would be nice if it returned true if the login was successful and false if it wasn't.

In this code, and most code in general, there is only one way to succeed, and many ways to fail. Therefore, the idiom should be:

if (failure condition 1) {
    cleanup 1;
    return false;
}
if (failure condition 2) {
    cleanup 2;
    return false;
}
if (failure condition 3) {
    cleanup 3;
    return false;
}
run some infallible code;
return true;

The code would flow better mentally if you rearranged a few statements, such that there are "stanzas". Constructing the $groups array breaks up the flow; I would move that into a helper method. See the suggested implementation below.

LDAP

ldap_sort() is deprecated. In any case, I don't understand why you would need to sort the search results by surname. Shouldn't the "(sAMAccountName=$username)" filter uniquely identify an account? If not, then your directory is a horrible mess.

By the way, be careful when doing string interpolation like that without performing escaping. An attacker might try to confuse your code by posting user_name=*. In this case, you are probably fine, since you only do the search after successfully binding using the given username.

You refer to $info[0] a lot. It's probably worthwhile to reassign the variable to discard the rest of the array.

Suggested implementation

private function doLoginWithPostData() {
    // Input
    $username = $_POST['username'];
    $password = $_POST['username'];

    // Configuration
    $adServer = 'ldaps://The Address';
    $baseDN = "dc=MYDC,dc=NET";
    $ldapRDN = "MYDN.net\\$username";

    // Validate
    if (empty($username) || empty($password)) {
        if (empty($username)) { $this->errors[] = "Username field was empty."; }
        if (empty($password)) { $this->errors[] = "Password field was empty."; }
        return false;
    }

    // Connect
    if (!($ldap = ldap_connect($adServer))) {
        $this->errors[] = "Could not connect to authentication server.";
        return false;
    }
    ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, 3);
    ldap_set_option($ldap, LDAP_OPT_REFERRALS, 0);

    // Bind
    if (!($bind = ldap_bind($ldap, $ldapRDN, $password))) {
        $this->errors[] = "Wrong username or password";
        @ldap_close($ldap);
        return false;
    }

    // Search
    $result = ldap_search($ldap, $baseDN, "(sAMAccountName=$username)");
    $info = ldap_get_entries($ldap, $result);
    $info = $info[0];
    @ldap_close($ldap);

    // Session
    $_SESSION['userID'] = bin2hex($info["objectguid"][0]);
    $_SESSION['fullName'] = $info["displayname"][0];
    $_SESSION['loggedOnStatus'] = "1";
    $_SESSION['userGroups'] = $groups = $this->userGroups($info["memberof"]);
    $_SESSION['userLevel'] = in_array('Dept_Directors', $groups) ? 10 :
                            (in_array('Dept_Admin',     $groups) ?  7
                                                                 :  0);
    if (strpos($info["dn"], 'OU=MYOU') !== false) {
        $_SESSION['userType'] = 2;
    }

    if (!($this->userProfileExists()) && !($this->createUserProfile())) {
        // Succeed with a warning
        $this->errors[] = "Error creating user profile";
    }
    return true;
}

Answer 2

1)

private function dologinWithPostData()
{

I would pass in login credentials to the method so you don't have some arbitrary class method directly accessing raw, unvalidated, uncleansed user input

2)

    if (empty($_POST['user_name'])) {
        $this->errors[] = "Username field was empty.";
    } elseif (empty($_POST['user_password'])) {
        $this->errors[] = "Password field was empty.";
    } elseif (!empty($_POST['user_name']) && !empty($_POST['user_password'])) {

Here I would implement the approach of failing fast. It makes your code much more readable and minimizes if-else nesting.

You also should strongly consider failing more loudly here on bad input. Log an error or throw InvalidArgumentException to let the caller know they did something wrong.

That might look something like:

if(
    empty($username) ||
    empty($password) ||
    !is_string($username) ||
    !is_string($password)
) {
    throw new InvalidArgumentException(
        'Both username and password must be non-zero length strings.'
    );
}

// possibly other validation here

// continue with normal code execution, now free from nesting

3)

        $adServer = "ldaps://The Address";

I would think that, this being a class and all, the server address would not be hardcoded into this specific method.

4)

        $ldap = ldap_connect($adServer);

        $ldaprdn = 'MYDN.net' . "\\" . $username;

        ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, 3);
        ldap_set_option($ldap, LDAP_OPT_REFERRALS, 0);

This seems to assume "happy path" execution. You should handle case where ldap_connect() fails. You do this on several ldap* calls in the following section (ldap_search()). I am not going to comment on them each specifically, but basically you should understand potential return values and make sure your code handles all possible outcomes.

5)

        $bind = @ldap_bind($ldap, $ldaprdn, $password);

Don't suppress errors with @!!! This is terrible coding practice. Typical throughout.

6)

            if($this->checkUserProfileExsists() == false){

Should you be using exact comparison here instead of loose comparison?

7)

                if(!($this->createUserProfile())){
                    $msg="Error Creating User Profile ";
                    echo $msg;
                }

It seems like a bad idea to have this method actually generate output. Let the caller handle output generation based on the provided method response. Unless a method is specifically purposed to output (other than to logs), you should generally avoid this practice.

8) It is unclear to me what the caller to this method should expect to happen. Your method doesn't return anything (perhaps just boolean result would be nice), and as mentioned above, may cause spurious output. How does the caller know that something happened?

9) Probably this method is doing too much (both performing LDAP accesss and setting data in session). You may want to refactor into more focused methods that do one specific activity. The caller could still use this method, but it might for example then call a protected method to to the ldap access, then another protected method to assign the data to session.