5
\$\begingroup\$

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

\$\endgroup\$
7
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for you answer! You given some great suggestions and I will also definitely incorporate your answer into my new code! Could tell me what " ? 10 :" is called so i can look it up in the manual, personally i think this makes the code a little harder to read at first glance but im sure if I understood it, it wouldn't be so bad! Thanks again :) \$\endgroup\$ – Dan Jul 6 '16 at 19:13
  • \$\begingroup\$ That is the ternary conditional operator. It's useful when you want to assign something to a variable, but the something depends on some condition. I hesitate to fully recommend it in PHP, though, since it is weirdly left-associative, unlike in most sane languages. Hence the awkward parentheses. \$\endgroup\$ – 200_success Jul 6 '16 at 19:18
  • \$\begingroup\$ Thanks for sharing a link to it! Im just trying out your code, and it works great, except for when you try to set $info values into the $_SESSION as var_dump($info) = resource(10) of type (ldap result entry) and not an array as I was expecting? Any thoughts ? \$\endgroup\$ – Dan Jul 6 '16 at 20:26
  • \$\begingroup\$ Oops, I think you need to call ldap_next_entry() on it. \$\endgroup\$ – 200_success Jul 6 '16 at 20:40
  • \$\begingroup\$ That didn't do it either, I think it does have to be ldap_get_entries on that one! \$\endgroup\$ – Dan Jul 6 '16 at 20:43
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot this look like some great advise and I will definitely implement most/if not all of what you said! \$\endgroup\$ – Dan Jul 6 '16 at 18:52
  • \$\begingroup\$ Mike (and @Danturnip), as I've dealt with this before, I have to say that suppressing ldap_bind is kind of a mandatory thing because if it yields and error, you'd just want to let the user know that the service is unavailable right now or something happened. The problem with that is that ldap_bind doesn't throw a catchable exception, but rather triggers error on screen. \$\endgroup\$ – Marco Aurélio Deleu Jul 6 '16 at 19:26
  • \$\begingroup\$ @MarcoAurélioDeleu Thanks for your comment. Was not overly familiar with using that set of functions. When you say they error to screen, I am assuming you mean in a case where errors are set to display (which they shouldn't be in any production system or even development systems if the dev team monitors logs closely through development process)? \$\endgroup\$ – Mike Brant Jul 6 '16 at 19:50
  • \$\begingroup\$ @MikeBrant true. My bad. I guess it's just that I dealt with that when I was first starting to develop and I didn't know much about server configuration and production setup and I never forgot about that ugly @ that I had to leave in my code. \$\endgroup\$ – Marco Aurélio Deleu Jul 6 '16 at 19:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.