OWASP A2:2017 – Broken Authentication

If you stumble across this post and are wondering what this is all about, then I recommend reading this post before following this guide. TL; DR, this post is about solving Secure Code Warrior challenges, more specifically PHP Basic challenges. I use challenges like these to aid my learning and keep my skills sharp. That being said, I love challenges especially technical ones. Whenever I see a worthy code auditing challenge, I curl and flex my brain muscles and then I am like:-

Bring it on!

Table of contents

Challenge 1 – Insecure Password Reset Function

Locating Vulnerability

As the challenge name suggests, we’ll be looking for functions that implement password reset feature in an insecure way. As such, if we look at the resetPassword() function in PersonalDataService.php we’ll see that three lines are highlighted for us marked with ⚠️.


public function resetPassword(): bool
    {
        if (!isset($_COOKIE['reset_password'])) {
            return false;
        }

        $sanitizedData = $this->service->sanitize($this->json);
        $password = $this->service->isValidPassword($sanitizedData['password']);
        $passwordConfirm = $this->service->isPasswordEqualsPasswordConfirm(
            $sanitizedData['password'],
            $sanitizedData['confirmed_password']
        );

        if ($password !== null && $password && $passwordConfirm) {
            $passwordHash = Hash::make($sanitizedData['password']);
            $hashKey = Session::getUserToken();

            return User::resetPassword($passwordHash, $hashKey);
        }

        return false;
    }

The above function checks for a cookie with name “reset_password” using the $_COOKIE superglobal variable and if the cookie is not set, then it returns false. If we search the code for “reset_password” parameter, then we’ll find that there is a function createNewResetPasswordToken in Token.php which uses the setcookie() function to create a cookie with the name “reset_password”.

public static function createNewResetPasswordToken()
    {
        setcookie('reset_password', md5(date('Y-m-d')), strtotime('+5 minutes'));
    }
}

Here the token generation method uses a weak algorithm, i.e., MD5, the security of which is considered severely compromised. The md5() function usage is not recommended by PHP and causes the cookie to be predictable by an attacker. The token generated here, i.e the md5 hash of date(‘Y-m-d’) is weak and easily predictable. As such, an attacker can easily forge his own token to reset the password of everyone using the webapp!

Identifying Solution

A session token must be randomly generated using a cryptographically secure algorithm and should be long enough to prevent brute-forcing. As such, the solutions implement base64_encode(), sha1() and md5() functions which are cryptographically weak and easily predictable. One solution gets rid of the createNewResetPasswordToken() function altogether and instead uses the user’s session token(which is generated using a cryptographically safe algorithm) to verify whether an user is authorized or not to carry out the password reset. In this solution, a multi-layered approach is taken to create the session token of the user which is further used as a CSRF token to prevent an attacker to carry out unauthorized operations. Let me break this down for ya.

First, if we look at reset_password.php in /views/cabinet folder, we’ll see a token is created using the setUserToken() function of Session class.

$token = Session::setUserToken();
$request = RequestFactory::getRequest();

Next, if we look at Session.php in /src/Framework/HTTP folder we’ll see that the setUserToken() function uses make() function of RandomString class and Hash class to create a session token and stores it inside $_SESSION supergloblal variable.

public static function setUserToken()
    {
        try {
            return $_SESSION['token'] = Hash::make(RandomString::make(40));
        } catch (\Exception $e) {
            throw new \RuntimeException($e->getMessage());
        }
    }

The Hash.php and RandomString.php files can be found in /application/services/generators/. The RandomString class uses the OWASP recommended random_bytes() function to create a cryptographically secure random string of 40 bytes length(passed in the setUserToken() function above). After a secure random string is created, it is passed to the make() function of Hash class. The hash is then calculated using the PHP password_hash() function using the OWASP recommended Argon2id algorithm.

<?php

namespace App\services\generators;

class Hash
{
    /**
     * @param string $data
     * @return false|string
     */
    public static function make(string $data)
    {
        $hash = password_hash($data, PASSWORD_ARGON2ID, ['memory_cost' => 2048, 'time_cost' => 4, 'threads' => 3]);

        if (!empty($hash)) {
            return $hash;
        }

        return false;
    }
}

The resulting hash is then used to create the $token session token in reset_password.php to verify whether a user is authorized to carry out the password reset or not. The token is also used as the CSRF token to prevent unauthorized operations by any malicious user. As this solution gets rid of the vulnerable function and instead uses cryptographically secure methods to generate session tokens an attacker cannot easily guess the session token and thus cannot conduct password reset of other users.

Challenge 2 – Improper Authentication

Locating Vulnerability

As this challenge involves improper authentication, we should be looking for code where proper check is not applied on the authentication mechanism or user supplied parameters are used to check whether an user is authorized to carry out any action. As such, the dropUser() function in PersonalCabinetService.php is vulnerable as it uses the client’s cookie to determine whether an user should’ve normal or administrative privilege and uses the user’s ID to carry out the function.

/**
 * @param Request $request
 * @return bool
 */
public function dropUser(Request $request): bool
{
    $post = $request->getParsedBody();
    $sanitized = $this->service->sanitize($post);

    if ($sanitized['email'] !== null
        && $_COOKIE['role'] === User::ADMINISTRATOR
        && $this->service->isValidEmail($sanitized['email'])
    ) {
        return User::dropUser($_COOKIE['user'], $sanitized['email']);
    }

    return false;
}

In the above function, on line 34 the $_COOKIE variable’s “role” parameter is used to determine the privileges of the user carrying out the user delete action. The problem here is that the privileges of the user is determined by a client controllable parameter(inside his cookie), as any user can modify his own cookie’s “role” parameter to give himself administrative privileges. Next, on line 37, the $_COOKIE variable’s “user” parameter is used to determine the “id” of the user executing the dropUser() function of User class. The dropUser() function of User class then uses the user’s “id” to determine whether the user is an administrator or a normal user.

    /**
     * @param int $id
     * @param string $email
     * @return bool
     */
    public static function dropUser(int $id, string $email): bool
    {
        $query = self::$database->prepare(
            'SELECT * FROM users WHERE id=:id'
        );
        $query->bindValue(':id', $id);

        if ($query->execute()) {
            $user = $query->fetch(PDO::FETCH_OBJ);

            if ($user->role === self::ADMINISTRATOR) {
                $query = self::$database->prepare(
                    'DELETE FROM users WHERE email=:email'
                );
                $query->bindValue(':email', $email);
                Logger::info('Deleted user');

                return $query->execute();
            }
        }

But the big problem is, the “user” parameter of cookie is also controllable on the client side. Both the parameters, i.e., “role” and “user” are set in isUserAuthorized() function of CheckUserService class.

class CheckUserService
{
    /**
     * @param string $token
     * @return bool
     */
    public static function isUserAuthorized(string $token): bool
    {
        $user = User::getCurrentUser($token);
        setcookie('user', $user->id, time()+3600);
        setcookie('role', $user->role, time()+3600);

        return self::isTokenExpired($user->expires_at);
    }

The above function is vulnerable as we saw in Challenge 1. Instead of using a secure token determined on the server side, user-controllable cookie parameters are used. A malicious user can easily change his own cookie’s “user” and “role” parameter to get administrative privileges on the webapp and delete any user. For example, an attacker can make the following POST request to web application after changing the cookies to impersonate the Administrator user and carry out administrative functions:-

POST /profile/PersonalCabinetService.php HTTP/1.1
Host: example.com
Cookie: "role=ADMINISTRATOR"
Cookie: "user=1"

[request body]

Identifying Solution

In my opinion, identifying the correct solutions of these challenges are always way simpler to determine than locating the vulnerability. Because, once you fully understand why the code is vulnerable it is easier to determine the safest way to make the code secure. As such, only one solution drops the usage of the “user” and “role” cookie parameters and instead uses secure tokens to determine whether the user executing the function has the appropriate privileges or not. In the below code, I’ve highlighted the changes made to the original vulnerable code.

    /**
     * @param string $hashKey
     * @param Request $request
     * @return bool
     */
    public function dropUser(string $hashKey, Request $request): bool
    {
        $post = $request->getParsedBody();
        $sanitized = $this->service->sanitize($post);

        if ($sanitized['email'] !== null
            && $this->service->isValidEmail($sanitized['email'])
            && User::getCurrentUser($hashKey)->role === User::ADMINISTRATOR
        ) {
            return User::dropUser($hashKey, $sanitized['email']);
        }

        return false;
    }

In the above code snippet, the getCurrentUser() function of User class is used to securely determine the role of the user executing the dropUser() function. Also, the dropUser() function of User class uses the $hashkey parameter which contains the authenticated user’s session token, to determine whether the authenticated user is a normal user or an administrator. Here, application uses the session cookie which is determined server side using a secure algorithm. Now, a malicious user cannot bypass the authentication mechanism as it was possible before by manipulating the cookie parameters.

Challenge 3 – Improper Authentication

This challenge is a bit similar to the challenges above and as such you’ll get good practice and can validate the code auditing skills you’ve gained thus far. But, while being a good practice challenge for normal users, for users like me it becomes repetitive and boring. This thing about the Secure Code Warrior free challenges is something I’m not a fan of, as it makes my job easier.

Locating Vulnerability

If you’re doing the challenges in the order I explained above, then this challenge will be a piece of cake for you. As such, there are only two files marked with ⚠️, CheckUserService.php and PersonalDataService.php. In PersonalDataService.php the changePersonalData() function uses improper authenctication mechanism as we saw in the challenges before. The function relies on user controllable cookie parameter, i.e., here “userID” to change personal data of the user.

public function changePersonalData(): bool
    {
        $sanitizedData = $this->service->sanitize($this->json);

        if ($this->service->isValidData([
            $sanitizedData['name'],
            $sanitizedData['surname'],
        ])
        ) {
            $this->personalData->name = $sanitizedData['name'];
            $this->personalData->surname = $sanitizedData['surname'];
        }

        $this->personalData->userID = $_COOKIE['userID'];

        if ($sanitizedData['phone'] !== null && $this->service->isValidPhone($sanitizedData['phone'])) {
            $this->personalData->phone = $sanitizedData['phone'];
        }

        $result = User::changeCredentials($this->personalData);


        if (!empty($result)) {
            return true;
        }

        return false;
    }

Any malicious user can modify the “userID” cookie parameter to change the authentication details of another user(including an administrator) and then ultimately impersonate the user using the changed credentials. If we check in CheckUserService.php we’ll see that the isUserAuthorized() function sets the cookie using the user’s id which according to me, only a noob PHP developer would do.

public static function isUserAuthorized(string $token): bool
    {
        $user = User::getCurrentUser($token);
        setcookie('userID', $user->id, time()+3600);

        return self::isTokenExpired($user->expires_at);
    }

Here, although the user is fetched from the database using his session token, the usage of the insecure cookie makes this a case of improper usage of authentication mechanism or as the challenge name suggests, improper authentication.

Identifying Solution

The solution is pretty straightforward, just remove code doing user validation using the user’s ID, as a parameter value, inside the cookie and instead use the already existing session cookie mechanism in the application to check whether the user trying to change the personal data is authenticated using his credentials(which’ll create the session cookie for that user) or not. As this challenge is pretty much same as Challenge 2 I won’t be explaining this in detail and will leave this is a challenge to solve for myself and anybody else trying it in the future. Thanks for reading.

Conclusion

Authentication is the process of verifying that an individual, entity or website is whom it claims to be. If proper check is not implemented to validate this process, then any malicious user can gain privileged access to a web application and conduct malicious activities with that access. An authentication mechanism should never depend on user-controllable parameters and should always be carried out on the server-side. And as such, this concludes the end of second category of OWASP TOP 10 2017. Only 8 more categories to go. Hope, I can finish them soon.

Leave a Reply

Your email address will not be published. Required fields are marked *